From 5463def03b00e8a7bb2fe76932327a39dbe86f07 Mon Sep 17 00:00:00 2001 From: Graham Dumpleton Date: Tue, 20 Aug 2024 18:21:55 +1000 Subject: [PATCH 1/2] Revise HTTP response codes when access denied. --- lookup-service/service/routes/authnz.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lookup-service/service/routes/authnz.py b/lookup-service/service/routes/authnz.py index 2a012c38..85fc903e 100644 --- a/lookup-service/service/routes/authnz.py +++ b/lookup-service/service/routes/authnz.py @@ -75,9 +75,9 @@ async def jwt_token_middleware( token = parts[1] decoded_token = decode_client_token(token) except jwt.ExpiredSignatureError: - return web.Response(text="JWT token has expired", status=403) + return web.Response(text="JWT token has expired", status=401) except jwt.InvalidTokenError: - return web.Response(text="JWT token is invalid", status=403) + return web.Response(text="JWT token is invalid", status=400) # Store the decoded token in the request object for later use. @@ -110,10 +110,10 @@ async def wrapper(request: web.Request) -> web.Response: client = client_database.get_client(decoded_token["sub"]) if not client: - return web.Response(text="Client not found", status=403) + return web.Response(text="Client not found", status=401) if not client.validate_identity(decoded_token["jti"]): - return web.Response(text="Client identity not valid", status=403) + return web.Response(text="Client identity does not match", status=401) # Continue processing the request. @@ -147,7 +147,7 @@ async def wrapper(request: web.Request) -> web.Response: client = client_database.get_client(client_name) if not client: - return web.Response(text="Client not found", status=403) + return web.Response(text="Client not found", status=401) # Check if the client has one of the required roles. From fbb5f99de0d70869dafc9fada83ebd6614e5e4d0 Mon Sep 17 00:00:00 2001 From: Graham Dumpleton Date: Tue, 20 Aug 2024 19:23:51 +1000 Subject: [PATCH 2/2] Review error response codes and correct as necessary. --- lookup-service/service/routes/authnz.py | 26 +++------------ lookup-service/service/routes/clients.py | 2 +- lookup-service/service/routes/clusters.py | 38 +++++++++++----------- lookup-service/service/routes/tenants.py | 20 +++--------- lookup-service/service/routes/workshops.py | 12 +++---- 5 files changed, 34 insertions(+), 64 deletions(-) diff --git a/lookup-service/service/routes/authnz.py b/lookup-service/service/routes/authnz.py index 85fc903e..0de936c8 100644 --- a/lookup-service/service/routes/authnz.py +++ b/lookup-service/service/routes/authnz.py @@ -110,11 +110,13 @@ async def wrapper(request: web.Request) -> web.Response: client = client_database.get_client(decoded_token["sub"]) if not client: - return web.Response(text="Client not found", status=401) + return web.Response(text="Client details not found", status=401) if not client.validate_identity(decoded_token["jti"]): return web.Response(text="Client identity does not match", status=401) + request["remote_client"] = client + # Continue processing the request. return await handler(request) @@ -130,33 +132,15 @@ def roles_accepted( def decorator(handler: Callable[..., web.Response]) -> web.Response: async def wrapper(request: web.Request) -> web.Response: - # Check if the decoded JWT token is present in the request object. - - if "jwt_token" not in request: - return web.Response(text="JWT token not supplied", status=400) - - decoded_token = request["jwt_token"] - - # Lookup the client by the name of the client taken from the JWT - # token subject. - - service_state = request.app["service_state"] - client_database = service_state.client_database - - client_name = decoded_token["sub"] - client = client_database.get_client(client_name) - - if not client: - return web.Response(text="Client not found", status=401) - # Check if the client has one of the required roles. + client = request["remote_client"] + matched_roles = client.has_required_role(*roles) if not matched_roles: return web.Response(text="Client access not permitted", status=403) - request["remote_client"] = client request["client_roles"] = matched_roles # Continue processing the request. diff --git a/lookup-service/service/routes/clients.py b/lookup-service/service/routes/clients.py index 8d91b268..86d5f9e1 100644 --- a/lookup-service/service/routes/clients.py +++ b/lookup-service/service/routes/clients.py @@ -36,7 +36,7 @@ async def api_get_v1_clients_details(request: web.Request) -> web.Response: client = client_database.get_client(client_name) if not client: - return web.Response(text="Client not available", status=403) + return web.Response(text="Client not available", status=404) details = { "name": client.name, diff --git a/lookup-service/service/routes/clusters.py b/lookup-service/service/routes/clusters.py index d61ff934..8f270deb 100644 --- a/lookup-service/service/routes/clusters.py +++ b/lookup-service/service/routes/clusters.py @@ -37,7 +37,7 @@ async def api_get_v1_clusters_details(request: web.Request) -> web.Response: cluster = cluster_database.get_cluster(cluster_name) if not cluster: - return web.Response(text="Cluster not available", status=403) + return web.Response(text="Cluster not available", status=404) details = { "name": cluster.name, @@ -60,7 +60,7 @@ async def api_get_v1_clusters_kubeconfig(request: web.Request) -> web.Response: cluster = cluster_database.get_cluster(cluster_name) if not cluster: - return web.Response(text="Cluster not available", status=403) + return web.Response(text="Cluster not available", status=404) kubeconfig = yaml.dump(cluster.kubeconfig) @@ -80,7 +80,7 @@ async def api_get_v1_clusters_portals(request: web.Request) -> web.Response: cluster = cluster_database.get_cluster(cluster_name) if not cluster: - return web.Response(text="Cluster not available", status=403) + return web.Response(text="Cluster not available", status=404) data = { "portals": [ @@ -116,12 +116,12 @@ async def api_get_v1_clusters_portals_details(request: web.Request) -> web.Respo cluster = cluster_database.get_cluster(cluster_name) if not cluster: - return web.Response(text="Cluster not available", status=403) + return web.Response(text="Cluster not available", status=404) portal = cluster.get_portal(portal_name) if not portal: - return web.Response(text="Portal not available", status=403) + return web.Response(text="Portal not available", status=404) details = { "name": portal.name, @@ -154,12 +154,12 @@ async def api_get_v1_clusters_portals_environments( cluster = cluster_database.get_cluster(cluster_name) if not cluster: - return web.Response(text="Cluster not available", status=403) + return web.Response(text="Cluster not available", status=404) portal = cluster.get_portal(portal_name) if not portal: - return web.Response(text="Portal not available", status=403) + return web.Response(text="Portal not available", status=404) environments = portal.get_environments() @@ -205,17 +205,17 @@ async def api_get_v1_clusters_portals_environments_details( cluster = cluster_database.get_cluster(cluster_name) if not cluster: - return web.Response(text="Cluster not available", status=403) + return web.Response(text="Cluster not available", status=404) portal = cluster.get_portal(portal_name) if not portal: - return web.Response(text="Portal not available", status=403) + return web.Response(text="Portal not available", status=404) environment = portal.get_environment(environment_name) if not environment: - return web.Response(text="Environment not available", status=403) + return web.Response(text="Environment not available", status=404) details = { "name": environment.name, @@ -254,17 +254,17 @@ async def api_get_v1_clusters_portals_environments_sessions( cluster = cluster_database.get_cluster(cluster_name) if not cluster: - return web.Response(text="Cluster not available", status=403) + return web.Response(text="Cluster not available", status=404) portal = cluster.get_portal(portal_name) if not portal: - return web.Response(text="Portal not available", status=403) + return web.Response(text="Portal not available", status=404) environment = portal.get_environment(environment_name) if not environment: - return web.Response(text="Environment not available", status=403) + return web.Response(text="Environment not available", status=404) sessions = environment.get_sessions() @@ -304,17 +304,17 @@ async def api_get_v1_clusters_portals_environments_users( cluster = cluster_database.get_cluster(cluster_name) if not cluster: - return web.Response(text="Cluster not available", status=403) + return web.Response(text="Cluster not available", status=404) portal = cluster.get_portal(portal_name) if not portal: - return web.Response(text="Portal not available", status=403) + return web.Response(text="Portal not available", status=404) environment = portal.get_environment(environment_name) if not environment: - return web.Response(text="Environment not available", status=403) + return web.Response(text="Environment not available", status=404) sessions = environment.get_sessions() @@ -347,17 +347,17 @@ async def api_get_v1_clusters_portals_environments_users_sessions( cluster = cluster_database.get_cluster(cluster_name) if not cluster: - return web.Response(text="Cluster not available", status=403) + return web.Response(text="Cluster not available", status=404) portal = cluster.get_portal(portal_name) if not portal: - return web.Response(text="Portal not available", status=403) + return web.Response(text="Portal not available", status=404) environment = portal.get_environment(environment_name) if not environment: - return web.Response(text="Environment not available", status=403) + return web.Response(text="Environment not available", status=404) sessions = environment.get_sessions() diff --git a/lookup-service/service/routes/tenants.py b/lookup-service/service/routes/tenants.py index 73ebf4ce..9333f5ca 100644 --- a/lookup-service/service/routes/tenants.py +++ b/lookup-service/service/routes/tenants.py @@ -51,7 +51,7 @@ async def api_get_v1_tenants_details(request: web.Request) -> web.Response: tenant = tenant_database.get_tenant(tenant_name) if not tenant: - return web.Response(text="Tenant not available", status=403) + return web.Response(text="Tenant not available", status=404) details = { "name": tenant.name, @@ -68,7 +68,6 @@ async def api_get_v1_tenants_portals(request: web.Request) -> web.Response: service_state = request.app["service_state"] tenant_database = service_state.tenant_database - client_database = service_state.client_database # Grab tenant name from path parameters. If the client has the tenant role # they can only access tenants they are mapped to. @@ -78,7 +77,6 @@ async def api_get_v1_tenants_portals(request: web.Request) -> web.Response: if not tenant_name: return web.Response(text="Missing tenant name", status=400) - client_name = request["client_name"] client_roles = request["client_roles"] # Note that currently "tenant" is not within the allowed roles but leaving @@ -86,10 +84,7 @@ async def api_get_v1_tenants_portals(request: web.Request) -> web.Response: # users with the "tenant" role. if "tenant" in client_roles: - client = client_database.get_client(client_name) - - if not client: - return web.Response(text="Client not found", status=403) + client = request["remote_client"] if not client.allowed_access_to_tenant(tenant_name): return web.Response(text="Client access not permitted", status=403) @@ -99,7 +94,7 @@ async def api_get_v1_tenants_portals(request: web.Request) -> web.Response: tenant = tenant_database.get_tenant(tenant_name) if not tenant: - return web.Response(text="Tenant not available", status=403) + return web.Response(text="Tenant not available", status=404) accessible_portals = tenant.portals_which_are_accessible() @@ -132,7 +127,6 @@ async def api_get_v1_tenants_workshops(request: web.Request) -> web.Response: service_state = request.app["service_state"] tenant_database = service_state.tenant_database - client_database = service_state.client_database # Grab tenant name from path parameters. If the client has the tenant role # they can only access tenants they are mapped to. @@ -142,14 +136,10 @@ async def api_get_v1_tenants_workshops(request: web.Request) -> web.Response: if not tenant_name: return web.Response(text="Missing tenant name", status=400) - client_name = request["client_name"] client_roles = request["client_roles"] if "tenant" in client_roles: - client = client_database.get_client(client_name) - - if not client: - return web.Response(text="Client not found", status=403) + client = request["remote_client"] if not client.allowed_access_to_tenant(tenant_name): return web.Response(text="Client access not permitted", status=403) @@ -159,7 +149,7 @@ async def api_get_v1_tenants_workshops(request: web.Request) -> web.Response: tenant = tenant_database.get_tenant(tenant_name) if not tenant: - return web.Response(text="Tenant not available", status=403) + return web.Response(text="Tenant not available", status=404) accessible_portals = tenant.portals_which_are_accessible() diff --git a/lookup-service/service/routes/workshops.py b/lookup-service/service/routes/workshops.py index eacd303f..41e52d08 100644 --- a/lookup-service/service/routes/workshops.py +++ b/lookup-service/service/routes/workshops.py @@ -18,7 +18,6 @@ async def api_get_v1_workshops(request: web.Request) -> web.Response: service_state = request.app["service_state"] tenant_database = service_state.tenant_database - client_database = service_state.client_database # Get the tenant name from the query parameters. This is required when # the client role is "tenant". @@ -36,13 +35,10 @@ async def api_get_v1_workshops(request: web.Request) -> web.Response: return web.Response(text="Missing tenant name", status=400) - client = client_database.get_client(client_name) - - if not client: - return web.Response(text="Client not found", status=403) + client = request["remote_client"] if not client.allowed_access_to_tenant(tenant_name): - return web.Response(text="Client access not permitted", status=403) + return web.Response(text="Client not allowed access to tenant", status=403) # Work out the set of portals accessible by the specified tenant. @@ -50,7 +46,7 @@ async def api_get_v1_workshops(request: web.Request) -> web.Response: tenant = tenant_database.get_tenant(tenant_name) if not tenant: - return web.Response(text="Tenant not available", status=403) + return web.Response(text="Tenant not available", status=503) accessible_portals = tenant.portals_which_are_accessible() @@ -144,7 +140,7 @@ async def api_post_v1_workshops(request: web.Request) -> web.Response: if not tenant: logger.error("Configuration for tenant %r could not be found", tenant_name) - return web.Response(text="Tenant not available", status=403) + return web.Response(text="Tenant not available", status=503) # Get the list of portals hosting the workshop and calculate the subset # that are accessible to the tenant.