Feat/token introspection#201
Open
TharunvenkateshN wants to merge 6 commits into
Open
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 38 minutes and 20 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
📝 WalkthroughWalkthroughMigrates JWT signing from HMAC shared secrets to RSA RS256 keypairs across config, token service, and all tests. Adds ChangesRSA JWT Migration + JWKS & OAuth Token Introspection
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant JWKSHandler
participant OAuthHandler
participant OAuthProviderService
participant TokenRepo as OAuthTokenRepository
participant TokenService
participant Redis as CacheService
rect rgba(100, 149, 237, 0.5)
note over Client, JWKSHandler: JWKS Discovery
Client->>Router: GET /.well-known/jwks.json
Router->>JWKSHandler: GetJWKS
JWKSHandler-->>Client: JWKSResponse (RS256, kid, Base64URL N/E)
end
rect rgba(144, 238, 144, 0.5)
note over Client, Redis: Token Introspection (RFC 7662)
Client->>Router: POST /oauth/introspect (token, client_id, client_secret)
Router->>OAuthHandler: Introspect
OAuthHandler->>OAuthHandler: authenticate client (form or Basic Auth)
OAuthHandler->>OAuthProviderService: IntrospectToken(ctx, tokenString)
OAuthProviderService->>TokenRepo: find non-expired OAuthAccessToken
alt opaque token found
TokenRepo-->>OAuthProviderService: OAuthAccessToken
OAuthProviderService-->>OAuthHandler: token, nil claims
OAuthHandler-->>Client: active=true + client_id/sub/scope/exp
else not found, try JWT
OAuthProviderService->>TokenService: ValidateAccessToken
TokenService-->>OAuthProviderService: JWTClaims
OAuthProviderService->>Redis: check blacklist
Redis-->>OAuthProviderService: not blacklisted
OAuthProviderService-->>OAuthHandler: nil token, JWTClaims
OAuthHandler-->>Client: active=true + sub/exp/iat/scope
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (3)
internal/service/oauth_provider_service.go (1)
357-361: 💤 Low valueRedis errors are conflated with blacklisted tokens.
When
IsTokenBlacklistedreturns an error (e.g., Redis connectivity issue), the code returns"token blacklisted"which is misleading. While fail-closed behavior is appropriate for security, consider distinguishing the error message for debugging:💡 Suggested improvement
// Check Redis blacklist for JWT blacklisted, err := s.cacheService.IsTokenBlacklisted(ctx, tokenString) - if err != nil || blacklisted { - return nil, nil, errors.New("token blacklisted") + if err != nil { + return nil, nil, errors.New("failed to verify token status") + } + if blacklisted { + return nil, nil, errors.New("token blacklisted") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/oauth_provider_service.go` around lines 357 - 361, The IsTokenBlacklisted call in the cacheService is conflating Redis errors with actual blacklisted tokens. When cacheService.IsTokenBlacklisted returns an error, you should check for that error condition separately and return a different error message that indicates a cache service error (for debugging and monitoring), rather than claiming the token is blacklisted. Only when there is no error from IsTokenBlacklisted should you check the blacklisted flag and return the "token blacklisted" error. This preserves fail-closed security behavior while allowing developers to distinguish between cache failures and actual blacklisted tokens.internal/dto/oauth_dto.go (1)
1-13: 💤 Low valueDTO defined but handler uses inline
gin.Hmaps.The
IntrospectionResponsestruct is correctly defined per RFC 7662, but theIntrospecthandler (per the context snippet) constructs responses usinggin.H{}inline maps rather than this DTO. Consider either using this struct in the handler for type safety, or documenting that this DTO is intended for Swagger/OpenAPI schema generation only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/dto/oauth_dto.go` around lines 1 - 13, The IntrospectionResponse struct is defined but the Introspect handler is using inline gin.H maps instead of this struct. Locate the Introspect handler and refactor it to use the IntrospectionResponse struct when constructing responses instead of gin.H maps. This ensures type safety and consistency with the DTO definition. Replace all gin.H{} responses in the Introspect handler with proper IntrospectionResponse struct instantiation and marshal it to JSON for the response.internal/config/config.go (1)
197-206: 💤 Low valuePartial key configuration silently falls back to ephemeral keys.
If only one of the two PEM files exists (e.g.,
private.pempresent butpublic.pemmissing), the code generates temporary in-memory keys and ignores the existing file. This could mask a deployment misconfiguration in production.Consider logging which specific file(s) are missing to aid debugging:
💡 Suggested improvement
- if os.IsNotExist(err1) || os.IsNotExist(err2) { - log.Println("RSA keys not found at provided paths, generating temporary in-memory keys for development/testing...") + if os.IsNotExist(err1) || os.IsNotExist(err2) { + if os.IsNotExist(err1) && os.IsNotExist(err2) { + log.Println("RSA keys not found at provided paths, generating temporary in-memory keys for development/testing...") + } else { + log.Printf("Warning: Only partial RSA key configuration found (private missing: %v, public missing: %v). Generating temporary in-memory keys...", os.IsNotExist(err1), os.IsNotExist(err2)) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/config.go` around lines 197 - 206, The current condition checks if either file is missing using os.IsNotExist(err1) || os.IsNotExist(err2), which treats partial file existence (one file found, one missing) the same as both files missing. Fix this by distinguishing three cases: when both files are missing (generate temporary keys), when only one file is missing (log which specific file is missing and fail), and when there are actual read errors. Update the condition to check if both errors are IsNotExist before falling back to temporary keys, and add an else if branch to handle the partial configuration case where exactly one file exists but the other doesn't, logging the specific missing file path to aid debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/swagger.json`:
- Around line 1316-1320: The `/oauth/introspect` endpoint's 401 response schema
reference at line 1316 declares `internal_handler.ErrorResponse` but the actual
handler returns only `error` and optional `error_description` fields without
`code` or `message`. Update the schema reference in the Swagger JSON to match
the actual runtime payload structure (either change the reference to a schema
with `error` and `error_description` fields, or update the handler to return
`internal_handler.ErrorResponse` with the expected `code` and `message` fields),
then run `make swagger` to regenerate the documentation consistently.
- Around line 35-40: The Swagger documentation in swagger.json only documents a
200 response for the JWKS endpoint with JWKSResponse schema, but the handler in
jwks_handler.go can return a 500 status when the public key is not configured.
Add a 500 response definition after the existing 200 response in the
swagger.json file to document this error condition, then run make swagger to
regenerate the documentation according to the coding guidelines.
In `@go.mod`:
- Around line 17-18: Remove the unused github.com/redis/go-redis/v9 v9.20.1
dependency from go.mod. Since the codebase consistently imports and uses
github.com/go-redis/redis/v8 across all files including cache_service.go,
routes.go, redis.go and related test files, the v9 version creates unnecessary
dependency confusion and should be deleted entirely from the module file.
In `@internal/handler/auth_handler_test.go`:
- Around line 277-279: The Redis client created with redis.NewClient in the
OAuth redirect flow test setup is not being cleaned up after the test, causing
resource leakage. After creating the rdb variable with redis.NewClient, add a
t.Cleanup function that calls rdb.Close() to ensure the Redis client connection
is properly released when the test completes.
In `@internal/handler/oauth_handler_test.go`:
- Around line 31-32: Redis clients are created in three places within the test
file using redis.NewClient() at the identified lines but are never closed,
causing resource leaks. Locate all three instances where Redis clients are
instantiated (the rdb variable assignments), and add proper cleanup by either
deferring the Close() method call immediately after creation or ensuring the
clients are closed in the test cleanup/teardown methods. Each redis.NewClient()
call must have a corresponding defer rdb.Close() or equivalent cleanup to
release the connection resources.
- Around line 461-463: The json.Unmarshal calls in the test are ignoring error
return values, which makes subsequent assertions on the unmarshaled resp map
unreliable if unmarshaling fails. Capture the error return value from
json.Unmarshal into a variable and add an assert statement (such as
assert.NoError) to verify the unmarshaling succeeded before proceeding with
field assertions on resp. Apply this fix to both instances where json.Unmarshal
is called on w.Body.Bytes() into the resp map.
- Around line 450-453: The introspection test setup is ignoring errors that
should be validated. In the fixture setup section, capture and assert the error
from bcrypt.GenerateFromPassword instead of using the blank identifier, and also
capture and assert the error returned from clientRepo.Create to ensure the
client persistence succeeds. Use appropriate test assertions (such as
require.NoError) to fail the test immediately if either operation fails,
preventing hidden fixture setup failures.
In `@internal/handler/oauth_handler.go`:
- Around line 484-485: The error responses returned by the Introspect function
are missing the required `code` field in the JSON structure. Modify both
occurrences of the 401 Unauthorized responses that return `invalid_client`
errors to include both the `error` and `code` fields in the JSON response.
Update the gin.H map to include `"code"` with an appropriate error code constant
(matching the codebase error conventions) alongside the existing `"error"` field
to comply with the standardized error response format.
- Line 11: The import statement for
github.com/roshankumar0036singh/auth-server/internal/dto in the oauth_handler.go
file is not used anywhere in the code and is causing lint/compile failures.
Remove this unused import statement from the imports section to resolve the
build issue.
- Around line 490-493: The ResolveClientForToken method accepts public clients
without requiring a secret, allowing token introspection with only a client_id.
Replace the call to ResolveClientForToken with a method that enforces
confidential client authentication (such as ResolveConfidentialClientForToken if
available in the oauthProviderService), or alternatively, after resolving the
client, add validation logic to verify that the client is not public before
allowing the introspection request to proceed.
In `@internal/service/oauth_provider_service_test.go`:
- Around line 16-17: The Redis client created with redis.NewClient in the test
setup is not properly closed after use, which leaks goroutines. Add a defer
statement immediately after creating the rdb client to ensure
redis.Client.Close() is called when the test completes, properly releasing all
resources associated with the Redis connection.
In `@internal/service/oauth_provider_service.go`:
- Around line 345-349: The IntrospectToken method currently only attempts to
find the token using the raw tokenString at lines 345-349, but it should follow
the same pattern as ValidateAccessToken by trying the hashed token first using
utils.HashToken(tokenString) before falling back to the raw token lookup. Modify
the token lookup logic in IntrospectToken to check for the hashed token first
(if found and not expired, return it), and only if that fails, then attempt to
find by the raw tokenString for backward compatibility.
In `@internal/service/security_phase1_test.go`:
- Around line 35-36: The Redis client created with redis.NewClient is not being
closed after the test completes, causing a resource leak. After initializing the
rdb variable with redis.NewClient, add a cleanup mechanism using either a defer
statement to call rdb.Close() or use t.Cleanup() to ensure the Redis client
connection is properly closed when the test finishes.
In `@internal/service/token_service.go`:
- Around line 159-161: The ValidateRefreshToken method in TokenService should
reject purpose-scoped tokens for consistency and security. After calling
s.parseToken(tokenString) and before returning the claims, add a check that
verifies the Purpose field in the returned JWTClaims is empty. If Purpose is not
empty, return an error similar to how ValidateAccessToken handles this case.
This ensures refresh tokens cannot be used when they have a specific purpose
assigned (such as MFA-pending tokens).
In `@internal/testutils/setup.go`:
- Around line 20-36: The GetTestRSAKeys function uses t.Fatalf inside the
sync.Once.Do block, which is problematic because t.Fatalf only exits the current
goroutine via runtime.Goexit, but sync.Once will still mark itself as done. This
means subsequent tests will receive nil keys without any error. Replace the
t.Fatalf call with a panic() statement instead, or alternatively remove the
sync.Once pattern entirely and store any error in a package-level variable that
gets checked on each call to GetTestRSAKeys. Either approach ensures that
subsequent tests will properly fail if key generation fails instead of silently
receiving nil values.
---
Nitpick comments:
In `@internal/config/config.go`:
- Around line 197-206: The current condition checks if either file is missing
using os.IsNotExist(err1) || os.IsNotExist(err2), which treats partial file
existence (one file found, one missing) the same as both files missing. Fix this
by distinguishing three cases: when both files are missing (generate temporary
keys), when only one file is missing (log which specific file is missing and
fail), and when there are actual read errors. Update the condition to check if
both errors are IsNotExist before falling back to temporary keys, and add an
else if branch to handle the partial configuration case where exactly one file
exists but the other doesn't, logging the specific missing file path to aid
debugging.
In `@internal/dto/oauth_dto.go`:
- Around line 1-13: The IntrospectionResponse struct is defined but the
Introspect handler is using inline gin.H maps instead of this struct. Locate the
Introspect handler and refactor it to use the IntrospectionResponse struct when
constructing responses instead of gin.H maps. This ensures type safety and
consistency with the DTO definition. Replace all gin.H{} responses in the
Introspect handler with proper IntrospectionResponse struct instantiation and
marshal it to JSON for the response.
In `@internal/service/oauth_provider_service.go`:
- Around line 357-361: The IsTokenBlacklisted call in the cacheService is
conflating Redis errors with actual blacklisted tokens. When
cacheService.IsTokenBlacklisted returns an error, you should check for that
error condition separately and return a different error message that indicates a
cache service error (for debugging and monitoring), rather than claiming the
token is blacklisted. Only when there is no error from IsTokenBlacklisted should
you check the blacklisted flag and return the "token blacklisted" error. This
preserves fail-closed security behavior while allowing developers to distinguish
between cache failures and actual blacklisted tokens.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7fbfb3be-0c54-41d8-97b2-3cbfa0a0f898
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
docs/docs.godocs/swagger.jsondocs/swagger.yamlgo.modinternal/config/config.gointernal/dto/jwks.gointernal/dto/oauth_dto.gointernal/handler/auth_handler_protected_test.gointernal/handler/auth_handler_test.gointernal/handler/jwks_handler.gointernal/handler/oauth_handler.gointernal/handler/oauth_handler_test.gointernal/middleware/auth_test.gointernal/routes/routes.gointernal/service/oauth_provider_service.gointernal/service/oauth_provider_service_test.gointernal/service/security_phase1_test.gointernal/service/token_service.gointernal/service/token_service_test.gointernal/testutils/setup.go
Contributor
Author
|
I have read the CLA and agree to its terms. |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR implements the standard RFC 7662 Token Introspection endpoint to allow resource servers to validate token state against the auth server in real-time.
Closes #172
Changes Made:
IntrospectionResponsestructure to map RFC 7662 fields (active,client_id,scope,token_type,exp,sub, etc.).CacheServiceintoOAuthProviderServiceand addedIntrospectTokento natively parse, validate, and identify Opaque Database Tokens and JWT access tokens (by validating standard scopes and querying the Redis blacklist).Introspecthandler underPOST /oauth/introspect. Client authentication handles both Form Data and HTTP Basic Auth. Automatically returns{"active": false}for expired, unauthenticated, or revoked tokens seamlessly.miniredistest fixtures acrossauth_handler_test.goandoauth_provider_service_test.goto explicitly create and passv8redis clients via injections where needed, hardening integration tests without compromising other test pipelines.Testing Evidence:
sub,scopeandexp.Dependencies were tidied via
go mod tidyspecifically isolatinggithub.com/go-redis/redis/v8imports.Summary by CodeRabbit
Release Notes
New Features
POST /oauth/introspect) for clients to validate and inspect access tokens with client authentication.GET /.well-known/jwks.json) to publicly expose server signing keys for token verification.Documentation