From 24a143683a81f78542f2fe8b815c21de377e3b7b Mon Sep 17 00:00:00 2001 From: Juan Marulanda Date: Tue, 12 Dec 2023 15:09:45 -0500 Subject: [PATCH 01/11] Added created service method --- tiled/authn_database/core.py | 9 +++++++++ tiled/server/schemas.py | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/tiled/authn_database/core.py b/tiled/authn_database/core.py index d90b0acfd..5bcc939a7 100644 --- a/tiled/authn_database/core.py +++ b/tiled/authn_database/core.py @@ -113,6 +113,15 @@ async def create_user(db, identity_provider, id): return refreshed_principal +async def create_service(db, role): + role_ = (await db.execute(select(Role).filter(Role.name == role))).scalar() + assert role_ is not None, "User role is missing from Roles table" + principal = Principal(type="service", roles=[role_]) + db.add(principal) + await db.commit() + return principal + + async def lookup_valid_session(db, session_id): if isinstance(session_id, int): # Old versions of tiled used an integer sid. diff --git a/tiled/server/schemas.py b/tiled/server/schemas.py index ab18f0526..40258fa5f 100644 --- a/tiled/server/schemas.py +++ b/tiled/server/schemas.py @@ -288,7 +288,7 @@ class About(pydantic.BaseModel): class PrincipalType(str, enum.Enum): user = "user" - service = "service" # TODO Add support for services. + service = "service" class Identity(pydantic.BaseModel, orm_mode=True): From ba9774fb346bca6aa3ae919165a33f4d0be4f38d Mon Sep 17 00:00:00 2001 From: Thomas Morris Date: Tue, 12 Dec 2023 16:13:39 -0500 Subject: [PATCH 02/11] added function to create new principal --- tiled/authn_database/core.py | 1 + tiled/scopes.py | 3 +++ tiled/server/authentication.py | 35 ++++++++++++++++++++++++++++++++++ 3 files changed, 39 insertions(+) diff --git a/tiled/authn_database/core.py b/tiled/authn_database/core.py index 5bcc939a7..7751646c2 100644 --- a/tiled/authn_database/core.py +++ b/tiled/authn_database/core.py @@ -49,6 +49,7 @@ async def create_default_roles(db): "write:data", "admin:apikeys", "read:principals", + "write:principals", "metrics", ], ), diff --git a/tiled/scopes.py b/tiled/scopes.py index ea62e9dab..36d438c22 100644 --- a/tiled/scopes.py +++ b/tiled/scopes.py @@ -14,4 +14,7 @@ "read:principals": { "description": "Read list of all users and services and their attributes." }, + "write:principals": { + "description": "Edit list of all users and services and their attributes." + }, } diff --git a/tiled/server/authentication.py b/tiled/server/authentication.py index 7760d4b08..919f17994 100644 --- a/tiled/server/authentication.py +++ b/tiled/server/authentication.py @@ -44,6 +44,7 @@ from ..authn_database.connection_pool import get_database_session from ..authn_database.core import ( create_user, + create_service, latest_principal_activity, lookup_valid_api_key, lookup_valid_pending_session_by_device_code, @@ -823,6 +824,40 @@ async def principal_list( return json_or_msgpack(request, principals) +@base_authentication_router.post( + "/principal", + response_model=schemas.Principal, +) +async def create_service_principal( + request: Request, + principal=Security(get_current_principal, scopes=["read:principals"]), + db=Depends(get_database_session), + role: str=Query(...), +): + "Create a principal for a service account." + + principal_orm = await create_service(db, role) + + # Relaod to select Principal and Identiies. + fully_loaded_principal_orm = ( + await db.execute( + select(orm.Principal) + .options( + selectinload(orm.Principal.identities), + selectinload(orm.Principal.roles), + selectinload(orm.Principal.api_keys), + selectinload(orm.Principal.sessions), + ) + .filter(orm.Principal.id == principal_orm.id) + ) + ).scalar() + + principal = schemas.Principal.from_orm(fully_loaded_principal_orm).dict() + request.state.endpoint = "auth" + + return json_or_msgpack(request, principal) + + @base_authentication_router.get( "/principal/{uuid}", response_model=schemas.Principal, From 8912032edecfbf3f00665d4f9ee078903c0e3e92 Mon Sep 17 00:00:00 2001 From: Juan Marulanda Date: Tue, 12 Dec 2023 17:07:29 -0500 Subject: [PATCH 03/11] Refactor authentication test methods --- tiled/_tests/test_authentication.py | 32 ++++++++--------------------- tiled/client/context.py | 28 +++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 23 deletions(-) diff --git a/tiled/_tests/test_authentication.py b/tiled/_tests/test_authentication.py index 50a79b8b0..089509375 100644 --- a/tiled/_tests/test_authentication.py +++ b/tiled/_tests/test_authentication.py @@ -539,9 +539,10 @@ def test_admin_api_key_any_principal( context.authenticate(username="alice") principal_uuid = principals_context["uuid"][username] - api_key = _create_api_key_other_principal( - context=context, uuid=principal_uuid, scopes=scopes + api_key_info = context.admin.create_api_key_other_principal( + principal_uuid, scopes=scopes ) + api_key = api_key_info["secret"] assert api_key context.logout() @@ -564,11 +565,11 @@ def test_admin_api_key_any_principal_exceeds_scopes(enter_password, principals_c principal_uuid = principals_context["uuid"]["bob"] with fail_with_status_code(400) as fail_info: - _create_api_key_other_principal( - context=context, uuid=principal_uuid, scopes=["read:principals"] + context.admin.create_api_key_other_principal( + principal_uuid, scopes=["read:principals"] ) - fail_message = " must be a subset of the principal's scopes " - assert fail_message in fail_info.response.text + fail_message = " must be a subset of the principal's scopes " + assert fail_message in fail_info.value.response.text context.logout() @@ -584,8 +585,8 @@ def test_api_key_any_principal(enter_password, principals_context, username): principal_uuid = principals_context["uuid"][username] with fail_with_status_code(401): - _create_api_key_other_principal( - context=context, uuid=principal_uuid, scopes=["read:metadata"] + context.admin.create_api_key_other_principal( + principal_uuid, scopes=["read:metadata"] ) @@ -619,18 +620,3 @@ def test_api_key_bypass_scopes(enter_password, principals_context): context.http_client.get( resource, params=query_params ).raise_for_status() - - -def _create_api_key_other_principal(context, uuid, scopes=None): - """ - Return api_key or raise error. - """ - response = context.http_client.post( - f"/api/v1/auth/principal/{uuid}/apikey", - json={"expires_in": None, "scopes": scopes or []}, - ) - response.raise_for_status() - api_key_info = response.json() - api_key = api_key_info["secret"] - - return api_key diff --git a/tiled/client/context.py b/tiled/client/context.py index 8a7bbda15..a10259edd 100644 --- a/tiled/client/context.py +++ b/tiled/client/context.py @@ -765,6 +765,34 @@ def show_principal(self, uuid): self.context.http_client.get(f"{self.base_url}/auth/principal/{uuid}") ).json() + def create_api_key_other_principal( + self, uuid, scopes=None, expires_in=None, note=None + ): + """ + Generate a new API for another user or service. + + Parameters + ---------- + uuid : str + Identify the user or service + scopes : Optional[List[str]] + Restrict the access available to the API key by listing specific scopes. + By default, this will have the same access as the user. + expires_in : Optional[int] + Number of seconds until API key expires. If None, + it will never expire or it will have the maximum lifetime + allowed by the server. + note : Optional[str] + Description (for humans). + """ + return handle_error( + self.context.http_client.post( + f"{self.base_url}/auth/principal/{uuid}/apikey", + headers={"Accept": MSGPACK_MIME_TYPE}, + json={"scopes": scopes, "expires_in": expires_in, "note": note}, + ) + ).json() + class CannotPrompt(Exception): pass From 9d13905e862592f0534b7d8fe251e577765e3d47 Mon Sep 17 00:00:00 2001 From: Juan Marulanda Date: Tue, 12 Dec 2023 17:22:34 -0500 Subject: [PATCH 04/11] fixed precommit issues --- tiled/server/authentication.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tiled/server/authentication.py b/tiled/server/authentication.py index 919f17994..eb313ba86 100644 --- a/tiled/server/authentication.py +++ b/tiled/server/authentication.py @@ -43,8 +43,8 @@ from ..authn_database import orm from ..authn_database.connection_pool import get_database_session from ..authn_database.core import ( - create_user, create_service, + create_user, latest_principal_activity, lookup_valid_api_key, lookup_valid_pending_session_by_device_code, @@ -832,7 +832,7 @@ async def create_service_principal( request: Request, principal=Security(get_current_principal, scopes=["read:principals"]), db=Depends(get_database_session), - role: str=Query(...), + role: str = Query(...), ): "Create a principal for a service account." @@ -843,10 +843,10 @@ async def create_service_principal( await db.execute( select(orm.Principal) .options( - selectinload(orm.Principal.identities), - selectinload(orm.Principal.roles), - selectinload(orm.Principal.api_keys), - selectinload(orm.Principal.sessions), + selectinload(orm.Principal.identities), + selectinload(orm.Principal.roles), + selectinload(orm.Principal.api_keys), + selectinload(orm.Principal.sessions), ) .filter(orm.Principal.id == principal_orm.id) ) From 367fdb74b1aca811a15e4b09f6f120b14d832adb Mon Sep 17 00:00:00 2001 From: Thomas Morris Date: Tue, 12 Dec 2023 17:40:07 -0500 Subject: [PATCH 05/11] add service account from context --- tiled/_tests/test_authentication.py | 23 +++++++++++++++++++++++ tiled/client/context.py | 20 ++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/tiled/_tests/test_authentication.py b/tiled/_tests/test_authentication.py index 089509375..2ee98e3ce 100644 --- a/tiled/_tests/test_authentication.py +++ b/tiled/_tests/test_authentication.py @@ -554,6 +554,29 @@ def test_admin_api_key_any_principal( context.http_client.get(resource).raise_for_status() +def test_admin_create_service_principal(enter_password, principals_context): + """ + Admin can create service accounts with API keys. + """ + with principals_context["context"] as context: + # Log in as Alice, create and use API key after logout + with enter_password("secret1"): + context.authenticate(username="alice") + + assert context.whoami()["type"] == "user" + + principal_info = context.admin.create_service_principal(role="user") + principal_uuid = principal_info["uuid"] + + service_api_key_info = context.admin.create_api_key_other_principal( + principal_uuid + ) + context.logout() + + context.api_key = service_api_key_info["secret"] + assert context.whoami()["type"] == "service" + + def test_admin_api_key_any_principal_exceeds_scopes(enter_password, principals_context): """ Admin cannot create API key that exceeds scopes for another principal. diff --git a/tiled/client/context.py b/tiled/client/context.py index a10259edd..9c0d12c0d 100644 --- a/tiled/client/context.py +++ b/tiled/client/context.py @@ -793,6 +793,26 @@ def create_api_key_other_principal( ) ).json() + def create_service_principal( + self, + role, + ): + """ + Generate a new service principal. + + Parameters + ---------- + role : str + Specify the role (e.g. user or admin) + """ + return handle_error( + self.context.http_client.post( + f"{self.base_url}/auth/principal", + headers={"Accept": MSGPACK_MIME_TYPE}, + params={"role": role}, + ) + ).json() + class CannotPrompt(Exception): pass From 447ec7388354a8def673a6c5080221bbd173f030 Mon Sep 17 00:00:00 2001 From: Dan Allan Date: Tue, 12 Dec 2023 18:03:30 -0500 Subject: [PATCH 06/11] Migrate to add 'write:prinicipals' to default admin role. --- tiled/authn_database/core.py | 3 +- ...32e_add_write_principals_scope_to_admin.py | 47 +++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 tiled/authn_database/migrations/versions/769180ce732e_add_write_principals_scope_to_admin.py diff --git a/tiled/authn_database/core.py b/tiled/authn_database/core.py index 7751646c2..ceb5ce23c 100644 --- a/tiled/authn_database/core.py +++ b/tiled/authn_database/core.py @@ -12,9 +12,10 @@ # This is the alembic revision ID of the database revision # required by this version of Tiled. -REQUIRED_REVISION = "c7bd2573716d" +REQUIRED_REVISION = "769180ce732e" # This is list of all valid revisions (from current to oldest). ALL_REVISIONS = [ + "769180ce732e", "c7bd2573716d", "4a9dfaba4a98", "56809bcbfcb0", diff --git a/tiled/authn_database/migrations/versions/769180ce732e_add_write_principals_scope_to_admin.py b/tiled/authn_database/migrations/versions/769180ce732e_add_write_principals_scope_to_admin.py new file mode 100644 index 000000000..0a93cb3cd --- /dev/null +++ b/tiled/authn_database/migrations/versions/769180ce732e_add_write_principals_scope_to_admin.py @@ -0,0 +1,47 @@ +"""Add 'write:principals' scope to admin + +Revision ID: 769180ce732e +Revises: c7bd2573716d +Create Date: 2023-12-12 17:57:56.388145 + +""" +from alembic import op +from sqlalchemy.orm.session import Session + +from tiled.authn_database.orm import Role + +# revision identifiers, used by Alembic. +revision = "769180ce732e" +down_revision = "c7bd2573716d" +branch_labels = None +depends_on = None + + +SCOPE = "write:principals" + + +def upgrade(): + """ + Add 'write:principal' scope to default 'admin' Role. + """ + connection = op.get_bind() + with Session(bind=connection) as db: + role = db.query(Role).filter(Role.name == "admin").first() + scopes = role.scopes.copy() + scopes.append(SCOPE) + role.scopes = scopes + db.commit() + + +def downgrade(): + """ + Remove new scopes from Roles, if present. + """ + connection = op.get_bind() + with Session(bind=connection) as db: + role = db.query(Role).filter(Role.name == "admin").first() + scopes = role.scopes.copy() + if SCOPE in scopes: + scopes.remove(SCOPE) + role.scopes = scopes + db.commit() From 75777a23a35603b972c44ee0a17c45b1b7ee380d Mon Sep 17 00:00:00 2001 From: Padraic Shafer <76011594+padraic-shafer@users.noreply.github.com> Date: Tue, 12 Dec 2023 18:04:18 -0800 Subject: [PATCH 07/11] Apply new "write:principals" scope protection --- .../769180ce732e_add_write_principals_scope_to_admin.py | 2 +- tiled/server/authentication.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tiled/authn_database/migrations/versions/769180ce732e_add_write_principals_scope_to_admin.py b/tiled/authn_database/migrations/versions/769180ce732e_add_write_principals_scope_to_admin.py index 0a93cb3cd..4448297dc 100644 --- a/tiled/authn_database/migrations/versions/769180ce732e_add_write_principals_scope_to_admin.py +++ b/tiled/authn_database/migrations/versions/769180ce732e_add_write_principals_scope_to_admin.py @@ -22,7 +22,7 @@ def upgrade(): """ - Add 'write:principal' scope to default 'admin' Role. + Add 'write:principals' scope to default 'admin' Role. """ connection = op.get_bind() with Session(bind=connection) as db: diff --git a/tiled/server/authentication.py b/tiled/server/authentication.py index eb313ba86..0d30e982b 100644 --- a/tiled/server/authentication.py +++ b/tiled/server/authentication.py @@ -830,7 +830,7 @@ async def principal_list( ) async def create_service_principal( request: Request, - principal=Security(get_current_principal, scopes=["read:principals"]), + principal=Security(get_current_principal, scopes=["write:principals"]), db=Depends(get_database_session), role: str = Query(...), ): From 618ff73235882d0289938d4257fc1bdac28ed9cf Mon Sep 17 00:00:00 2001 From: Dan Allan Date: Wed, 13 Dec 2023 06:24:43 -0500 Subject: [PATCH 08/11] Docstring clarifications Co-authored-by: Padraic Shafer <76011594+padraic-shafer@users.noreply.github.com> --- tiled/client/context.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tiled/client/context.py b/tiled/client/context.py index 9c0d12c0d..536c9dee4 100644 --- a/tiled/client/context.py +++ b/tiled/client/context.py @@ -769,15 +769,15 @@ def create_api_key_other_principal( self, uuid, scopes=None, expires_in=None, note=None ): """ - Generate a new API for another user or service. + Generate a new API key for another user or service. Parameters ---------- uuid : str - Identify the user or service + Identify the principal -- the user or service scopes : Optional[List[str]] Restrict the access available to the API key by listing specific scopes. - By default, this will have the same access as the user. + By default, this will have the same access as the principal. expires_in : Optional[int] Number of seconds until API key expires. If None, it will never expire or it will have the maximum lifetime From 0c99d76bed25cc110b7fb1f75269d44fb803dbd4 Mon Sep 17 00:00:00 2001 From: Dan Allan Date: Wed, 13 Dec 2023 06:26:28 -0500 Subject: [PATCH 09/11] Improve error handling on unknown role. --- tiled/authn_database/core.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tiled/authn_database/core.py b/tiled/authn_database/core.py index ceb5ce23c..24df7a37b 100644 --- a/tiled/authn_database/core.py +++ b/tiled/authn_database/core.py @@ -117,7 +117,8 @@ async def create_user(db, identity_provider, id): async def create_service(db, role): role_ = (await db.execute(select(Role).filter(Role.name == role))).scalar() - assert role_ is not None, "User role is missing from Roles table" + if role_ is not None: + raise ValueError(f"Role named {role!r} is not found") principal = Principal(type="service", roles=[role_]) db.add(principal) await db.commit() From 3ce55d70910b26759423df99e7f6e3008d9fab29 Mon Sep 17 00:00:00 2001 From: Dan Allan Date: Wed, 13 Dec 2023 06:28:59 -0500 Subject: [PATCH 10/11] Use briefer name; rely on namespace to distinguish. --- tiled/_tests/test_authentication.py | 16 ++++------------ tiled/client/context.py | 4 +--- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/tiled/_tests/test_authentication.py b/tiled/_tests/test_authentication.py index 2ee98e3ce..4b0d1d2fe 100644 --- a/tiled/_tests/test_authentication.py +++ b/tiled/_tests/test_authentication.py @@ -539,9 +539,7 @@ def test_admin_api_key_any_principal( context.authenticate(username="alice") principal_uuid = principals_context["uuid"][username] - api_key_info = context.admin.create_api_key_other_principal( - principal_uuid, scopes=scopes - ) + api_key_info = context.admin.create_api_key(principal_uuid, scopes=scopes) api_key = api_key_info["secret"] assert api_key context.logout() @@ -568,9 +566,7 @@ def test_admin_create_service_principal(enter_password, principals_context): principal_info = context.admin.create_service_principal(role="user") principal_uuid = principal_info["uuid"] - service_api_key_info = context.admin.create_api_key_other_principal( - principal_uuid - ) + service_api_key_info = context.admin.create_api_key(principal_uuid) context.logout() context.api_key = service_api_key_info["secret"] @@ -588,9 +584,7 @@ def test_admin_api_key_any_principal_exceeds_scopes(enter_password, principals_c principal_uuid = principals_context["uuid"]["bob"] with fail_with_status_code(400) as fail_info: - context.admin.create_api_key_other_principal( - principal_uuid, scopes=["read:principals"] - ) + context.admin.create_api_key(principal_uuid, scopes=["read:principals"]) fail_message = " must be a subset of the principal's scopes " assert fail_message in fail_info.value.response.text context.logout() @@ -608,9 +602,7 @@ def test_api_key_any_principal(enter_password, principals_context, username): principal_uuid = principals_context["uuid"][username] with fail_with_status_code(401): - context.admin.create_api_key_other_principal( - principal_uuid, scopes=["read:metadata"] - ) + context.admin.create_api_key(principal_uuid, scopes=["read:metadata"]) def test_api_key_bypass_scopes(enter_password, principals_context): diff --git a/tiled/client/context.py b/tiled/client/context.py index 536c9dee4..66288bd61 100644 --- a/tiled/client/context.py +++ b/tiled/client/context.py @@ -765,9 +765,7 @@ def show_principal(self, uuid): self.context.http_client.get(f"{self.base_url}/auth/principal/{uuid}") ).json() - def create_api_key_other_principal( - self, uuid, scopes=None, expires_in=None, note=None - ): + def create_api_key(self, uuid, scopes=None, expires_in=None, note=None): """ Generate a new API key for another user or service. From ff2d2afee19a3e3d8a7ed94d66621565e5abdf12 Mon Sep 17 00:00:00 2001 From: Dan Allan Date: Wed, 13 Dec 2023 06:40:04 -0500 Subject: [PATCH 11/11] Fix sign of error handling --- tiled/authn_database/core.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tiled/authn_database/core.py b/tiled/authn_database/core.py index 24df7a37b..efabb4ae9 100644 --- a/tiled/authn_database/core.py +++ b/tiled/authn_database/core.py @@ -117,7 +117,7 @@ async def create_user(db, identity_provider, id): async def create_service(db, role): role_ = (await db.execute(select(Role).filter(Role.name == role))).scalar() - if role_ is not None: + if role_ is None: raise ValueError(f"Role named {role!r} is not found") principal = Principal(type="service", roles=[role_]) db.add(principal)