diff --git a/README.md b/README.md index 1f9be08..1fc4d66 100644 --- a/README.md +++ b/README.md @@ -53,7 +53,7 @@ campus grant grant --grantee-type user --grantee-id someone@school.edu \ # campus.audit: mint a student's trace-reading key (operator, #54) CAMPUS_AUDIT_API_KEY= campus audit login campus audit keys new --name "Alice" --owner alice@school.edu \ - --scope traces:read --rate-limit 60 + --role traces-viewer --rate-limit 60 campus audit keys list --owner alice@school.edu # …and the queries that key unlocks (from any machine the student logs in on) @@ -199,14 +199,22 @@ Two roles use the group: ```bash campus audit keys new --name "Alice" --owner alice@school.edu \ - --scope traces:read --rate-limit 60 --expires-at 2026-12-31T23:59:59Z + --role traces-viewer --rate-limit 60 --expires-at 2026-12-31T23:59:59Z campus audit keys list --owner alice@school.edu campus audit keys get --key-id - campus audit keys update --key-id --scope traces:read --rate-limit 120 + campus audit keys update --key-id --role traces-viewer --rate-limit 120 campus audit keys regenerate --key-id # rotation; old value dies campus audit keys revoke --key-id # record kept for audit ``` + **Roles vs scopes.** `--role` names one of the service's roles — a + bundle of scopes it owns (`traces-viewer`, `traces-searcher`, + `traces-producer`, `key-operator`), so the holder gets a reviewed + privilege set instead of a hand-assembled one. `--scope` is for + anything the roles do not cover. The two are mutually exclusive, and + the service owns the catalogue: an unknown role is refused with the + names of the roles it does have. + Plaintext key material is shown exactly once, at creation and at regeneration (the server keeps only the hash) — pass it to the student, who stores it with `campus audit login`. `owner_id` is a diff --git a/campus_cli/audit/keys.py b/campus_cli/audit/keys.py index 2a07a77..c45a877 100644 --- a/campus_cli/audit/keys.py +++ b/campus_cli/audit/keys.py @@ -53,6 +53,15 @@ " (the server requires at least one)" ), ) +RoleOption = typer.Option( + None, + "--role", + help=( + "Named role the key gets, e.g. traces-viewer. The role is a" + " bundle of scopes the service owns; --scope and --role are" + " mutually exclusive" + ), +) def _print_reveal_key(key_value: str) -> None: @@ -192,7 +201,15 @@ def keys_list( def keys_new( name: str = typer.Option(..., "--name", help="Key name, e.g. the student's name"), owner: str = OwnerOption, - scope: list[str] = ScopeOption, # noqa: B008 + scope: Optional[list[str]] = typer.Option( # noqa: B008 + None, + "--scope", + help=( + "Scope the key grants, e.g. traces:read; repeat for multiple" + " (mutually exclusive with --role)" + ), + ), + role: Optional[str] = RoleOption, rate_limit: Optional[int] = typer.Option( None, "--rate-limit", help="Requests per minute (optional)" ), @@ -205,16 +222,34 @@ def keys_new( """ Create an audit API key (mint a student key). + Give the key a privilege: --role for one of the service's + named roles (a bundle of scopes it owns), or --scope for a + hand-assembled scope list. The two are mutually exclusive — the + server refuses both together, because it leaves it ambiguous which + was meant. + The plaintext key is shown exactly once, at creation — the server keeps only its hash. Pass it to the student; they store it with 'campus audit login --api-key'. """ + if role and scope: + print_error("Give either --role or --scope, not both.") + raise typer.Exit(1) + if not role and not scope: + print_error( + "Give the key a privilege: --role or --scope ." + ) + raise typer.Exit(1) + if dry_run: new_kwargs = [ f" name={name!r},", f" owner_id={owner!r},", - f" scopes={list(scope)!r},", ] + if role is not None: + new_kwargs.append(f" role={role!r},") + else: + new_kwargs.append(f" scopes={list(scope)!r},") if rate_limit is not None: new_kwargs.append(f" rate_limit={rate_limit!r},") if expires_at is not None: @@ -236,7 +271,8 @@ def keys_new( record = api.apikeys.new( name=name, owner_id=owner, - scopes=list(scope), + scopes=list(scope) if scope else None, + role=role, rate_limit=rate_limit, expires_at=expires_at, ) @@ -297,9 +333,10 @@ def keys_update( "--scope", help=( "Replacement scope set; repeat for multiple. Omit to keep" - " the current scopes" + " the current scopes (mutually exclusive with --role)" ), ), + role: Optional[str] = RoleOption, rate_limit: Optional[int] = typer.Option( None, "--rate-limit", help="Requests per minute" ), @@ -307,14 +344,21 @@ def keys_update( dry_run: bool = dry_run_option(), ) -> None: """ - Update an audit API key's name, scopes, or rate limit. + Update an audit API key's name, scopes, role, or rate limit. Only the fields passed are changed. Expiry and revocation are immutable — mint a fresh key instead. + + --role and --scope are mutually exclusive: the server refuses both + together, because it leaves it ambiguous which was meant. """ - if name is None and scope is None and rate_limit is None: + if role and scope: + print_error("Give either --role or --scope, not both.") + raise typer.Exit(1) + if name is None and scope is None and role is None and rate_limit is None: print_error( - "Nothing to update: pass --name, --scope, or --rate-limit." + "Nothing to update: pass --name, --scope, --role, or" + " --rate-limit." ) raise typer.Exit(1) @@ -322,7 +366,9 @@ def keys_update( update_kwargs = [] if name is not None: update_kwargs.append(f" name={name!r},") - if scope is not None: + if role is not None: + update_kwargs.append(f" role={role!r},") + elif scope is not None: update_kwargs.append(f" scopes={list(scope)!r},") if rate_limit is not None: update_kwargs.append(f" rate_limit={rate_limit!r},") @@ -343,6 +389,7 @@ def keys_update( record = api.apikeys[key_id].update( name=name, scopes=list(scope) if scope is not None else None, + role=role, rate_limit=rate_limit, ) diff --git a/poetry.lock b/poetry.lock index 5e78c04..e67faaf 100644 --- a/poetry.lock +++ b/poetry.lock @@ -176,7 +176,7 @@ flask = "^3.0.0" type = "git" url = "https://github.com/nyjc-computing/campus-api-python.git" reference = "main" -resolved_reference = "dad044252ec9256cf99e5a0f7d9b29ecf81e88fc" +resolved_reference = "26678465e55f737526fc28cc7ff9979dbfcd640a" [[package]] name = "campus-suite" diff --git a/tests/unit/test_audit.py b/tests/unit/test_audit.py index a9b1db6..08f0639 100644 --- a/tests/unit/test_audit.py +++ b/tests/unit/test_audit.py @@ -766,3 +766,93 @@ def test_absent_journey_is_none(self): assert result.exit_code == 0, result.output assert api.traces.search.call_args.kwargs["journey_id"] is None + + +class TestKeysRole: + """`campus audit keys new/update --role` grants a named role (#67). + + The role is the service's own vocabulary (campus#918); the CLI + passes the name through and lets the server refuse an unknown one. + """ + + def _invoke_new(self, args): + api = mock.Mock() + api.apikeys.new.return_value = {"id": "k1", "api_key": "audit_v1_x"} + with ( + mock.patch("campus_cli.audit.keys.get_audit_client", return_value=api), + mock.patch.dict("os.environ", {"CAMPUS_AUDIT_URL": AUDIT_URL}), + ): + result = CliRunner().invoke(app, ["audit", "keys", "new", *args]) + return result, api + + def test_role_is_passed_and_no_scopes(self): + result, api = self._invoke_new([ + "--name", "Alice", "--owner", "alice@school.edu", + "--role", "traces-viewer", + ]) + + assert result.exit_code == 0, result.output + kwargs = api.apikeys.new.call_args.kwargs + assert kwargs["role"] == "traces-viewer" + assert kwargs["scopes"] is None + + def test_scopes_still_work(self): + result, api = self._invoke_new([ + "--name", "Alice", "--owner", "alice@school.edu", + "--scope", "traces:read", + ]) + + assert result.exit_code == 0, result.output + kwargs = api.apikeys.new.call_args.kwargs + assert kwargs["scopes"] == ["traces:read"] + assert kwargs["role"] is None + + def test_role_and_scopes_together_are_refused(self): + result, api = self._invoke_new([ + "--name", "Alice", "--owner", "alice@school.edu", + "--role", "traces-viewer", "--scope", "traces:read", + ]) + + assert result.exit_code == 1 + assert "not both" in result.output + api.apikeys.new.assert_not_called() + + def test_neither_role_nor_scopes_is_refused(self): + result, api = self._invoke_new([ + "--name", "Alice", "--owner", "alice@school.edu", + ]) + + assert result.exit_code == 1 + assert "--role" in result.output + api.apikeys.new.assert_not_called() + + def test_update_accepts_a_role(self): + api = mock.MagicMock() + api.apikeys.__getitem__.return_value.update.return_value = {"id": "k1"} + with ( + mock.patch("campus_cli.audit.keys.get_audit_client", return_value=api), + mock.patch.dict("os.environ", {"CAMPUS_AUDIT_URL": AUDIT_URL}), + ): + result = CliRunner().invoke(app, [ + "audit", "keys", "update", "--key-id", "k1", + "--role", "traces-searcher", + ]) + + assert result.exit_code == 0, result.output + kwargs = api.apikeys.__getitem__.return_value.update.call_args.kwargs + assert kwargs["role"] == "traces-searcher" + assert kwargs["scopes"] is None + + def test_update_refuses_role_and_scopes_together(self): + api = mock.MagicMock() + with ( + mock.patch("campus_cli.audit.keys.get_audit_client", return_value=api), + mock.patch.dict("os.environ", {"CAMPUS_AUDIT_URL": AUDIT_URL}), + ): + result = CliRunner().invoke(app, [ + "audit", "keys", "update", "--key-id", "k1", + "--role", "traces-viewer", "--scope", "traces:read", + ]) + + assert result.exit_code == 1 + assert "not both" in result.output