fix: allow platform paths without tenant claim in JWT#1400
Conversation
Platform paths like /v1/tenants are bootstrap endpoints where the caller discovers available tenants. Requiring a tenant claim to list tenants is circular — the user needs to know which tenants exist before they can scope requests to one. Export IsPlatformPath from shared/platform/gateway and use it in TenantAuthorizationMiddleware to bypass tenant authorization for authenticated users hitting platform endpoints. The endpoint itself handles access control based on the caller's identity. Fixes the demo login flow where Dex OIDC tokens (which lack custom tenant claims) could not call ListTenants to populate the tenant picker.
📝 WalkthroughWalkthroughThe changes refactor JWT tenant authorization handling in the API gateway by extracting logic for processing requests without tenant claims into a dedicated method. The platform path checking utility is simultaneously promoted from unexported to exported to support this new middleware logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
Claude Code ReviewCommit: SummaryClean, well-scoped fix for a real chicken-and-egg problem: OIDC tokens from Dex lack tenant claims, so users cannot call The Risk Assessment
Findings
AnalysisSecurity model: The bypass is gated behind three conditions: (1) the user is authenticated (passed Refactoring correctness: The Test coverage: Two new tests cover both REST ( Multi-tenant isolation: No risk — platform paths are tenant-agnostic by design. No tenant data is exposed or leaked by this change. Bot Review NotesNo unresolved bot threads found on this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shared/platform/gateway/tenant_resolver.go (1)
84-90:⚠️ Potential issue | 🟠 MajorTighten platform-path matching to avoid unintended auth bypass.
Line 86 currently does raw prefix matching, so
/v1/tenantshipwould be treated as a platform path. That can unintentionally bypass tenant resolution/authorization for non-platform routes.Proposed fix
func IsPlatformPath(path string) bool { for _, prefix := range platformPaths { - if strings.HasPrefix(path, prefix) { + base := strings.TrimSuffix(prefix, "/") + if path == base || strings.HasPrefix(path, base+"/") { return true } } return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/platform/gateway/tenant_resolver.go` around lines 84 - 90, IsPlatformPath currently treats any path starting with a platform prefix as a match (e.g., "/v1/tenantship"), which can bypass tenant checks; change the logic in IsPlatformPath to only return true when the path is exactly the prefix or the prefix followed by a slash (i.e., path == prefix || strings.HasPrefix(path, prefix + "/")) for each entry in platformPaths, so only the intended route segments (and their subpaths) are considered platform paths.
🧹 Nitpick comments (1)
shared/platform/gateway/tenant_resolver_test.go (1)
931-946: Add a boundary regression assertion for non-platform near-prefix paths.Consider adding a negative case like
assert.False(t, IsPlatformPath("/v1/tenantship"))to lock down prefix-boundary behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/platform/gateway/tenant_resolver_test.go` around lines 931 - 946, Add a negative boundary test to ensure prefix matches are exact: update TestIsPlatformPath to include an assertion that a near-prefix path like "/v1/tenantship" (and optionally "/meridian.tenant.v1x.TenantService/List") returns false; locate the test function TestIsPlatformPath and add assert.False(t, IsPlatformPath("/v1/tenantship")) to lock down prefix-boundary behavior for IsPlatformPath.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@shared/platform/gateway/tenant_resolver.go`:
- Around line 84-90: IsPlatformPath currently treats any path starting with a
platform prefix as a match (e.g., "/v1/tenantship"), which can bypass tenant
checks; change the logic in IsPlatformPath to only return true when the path is
exactly the prefix or the prefix followed by a slash (i.e., path == prefix ||
strings.HasPrefix(path, prefix + "/")) for each entry in platformPaths, so only
the intended route segments (and their subpaths) are considered platform paths.
---
Nitpick comments:
In `@shared/platform/gateway/tenant_resolver_test.go`:
- Around line 931-946: Add a negative boundary test to ensure prefix matches are
exact: update TestIsPlatformPath to include an assertion that a near-prefix path
like "/v1/tenantship" (and optionally "/meridian.tenant.v1x.TenantService/List")
returns false; locate the test function TestIsPlatformPath and add
assert.False(t, IsPlatformPath("/v1/tenantship")) to lock down prefix-boundary
behavior for IsPlatformPath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 78a695e3-589f-48fe-a00d-20858cc0375f
📒 Files selected for processing (4)
services/api-gateway/auth/combined_middleware.goservices/api-gateway/auth/combined_middleware_test.goshared/platform/gateway/tenant_resolver.goshared/platform/gateway/tenant_resolver_test.go
|
This PR contains breaking changes to protobuf definitions. Please review the changes carefully and consider:
Run 💡 Tip: If this is an intentional breaking change, add the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean fix for the tenant-claim chicken-and-egg problem. Security model is sound — platform path bypass requires authentication, and the endpoints handle their own access control. Refactoring preserves existing behavior while adding the new platform-path fallback. Tests cover both REST and gRPC paths. No multi-tenant isolation risk. See summary comment for full analysis.
1 participant
Summary
IsPlatformPathfromshared/platform/gatewayfor reuse across middleware/v1/tenants,TenantService/*) without a tenant claim in their JWTauthorizeWithoutTenantClaimhelper to reduce cognitive complexity inTenantAuthorizationMiddleware.HandlerContext
Standard OIDC providers like Dex issue identity-only tokens without custom tenant claims. The frontend login flow calls
ListTenantsto discover available tenants and populate the tenant picker. This endpoint was blocked with 403 "missing tenant claim in token" becauseTenantAuthorizationMiddlewarerequired a tenant claim even for platform-level bootstrap endpoints.This is a chicken-and-egg problem: you need to list tenants to know which tenant to scope to, but listing tenants required tenant scope.
The fix adds a third fallback path in the empty-tenant-claim handler: after checking for platform-admin role and resolved tenant from subdomain, check if the request targets a platform path. Platform paths are bootstrap endpoints that handle their own access control.
Test plan
TestTenantAuthorizationMiddlewaretests pass (including 2 new platform path tests)TestIsPlatformPathpasses (exported function)./services/api-gateway/...test suite passes