[Discussion WIP] Refactor API key validation to return API key details#1018
Open
Thushani-Jayasekera wants to merge 2 commits intowso2:mainfrom
Open
[Discussion WIP] Refactor API key validation to return API key details#1018Thushani-Jayasekera wants to merge 2 commits intowso2:mainfrom
Thushani-Jayasekera wants to merge 2 commits intowso2:mainfrom
Conversation
Contributor
WalkthroughValidateAPIKey now returns an additional API key details pointer; SharedContext.AuthContext changed from map[string]string to a typed *AuthContext with explicit fields, and call sites updated to construct and read the new AuthContext. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Thushani-Jayasekera
changed the title
Contributor
|
@Thushani-Jayasekera |
Thushani-Jayasekera
force-pushed
the
cleanup
branch
from
23b0ecb to
5081479
Compare
![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: 2
🧹 Nitpick comments (2)
gateway/system-policies/analytics/analytics.go (1)
34-35: Constant name vs. comment mismatch may cause confusion.
AuthContextKeyUserIDwas previously documented as an "AuthContext key" and is now described as an "Analytics metadata key." The constant is used both as the key inanalyticsMetadata(line 248) and as a lookup key inAuthContext.Properties(line 250). If the Properties lookup on line 250 is intentional (i.e., auth policies are expected to set this key in Properties), the name and documentation should clarify the dual usage. If it's not intentional (per the issue above), the "Analytics metadata key" comment is accurate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/system-policies/analytics/analytics.go` around lines 34 - 35, The constant AuthContextKeyUserID is ambiguously documented as "Analytics metadata key" but is also used to read from AuthContext.Properties and to populate analyticsMetadata; verify intended usage and either (A) update the comment to reflect dual use (e.g., "AuthContext/Analytics metadata key for user ID") so callers of AuthContextKeyUserID and readers of analyticsMetadata are clear, or (B) if the auth property and analytics key should be distinct, split into two explicit constants (e.g., AuthContextKeyUserID and AnalyticsMetadataKeyUserID) and update all references where analyticsMetadata is populated and where AuthContext.Properties is read to use the appropriate constant (refer to usages in analyticsMetadata and AuthContext.Properties).common/apikey/api_key_hash_test.go (1)
72-84: Test assertions for the new return value look good.The nil check on line 79 uses
t.Error(non-fatal), so the subsequent nil guard on line 82 is necessary to avoid a panic — this is handled correctly. Consider usingt.Fatalon line 80 instead, which would both simplify the logic and stop the test early on unexpected nil.♻️ Optional simplification
- if apiKeyDetails == nil { - t.Error("Expected API key details to be returned") - } - if apiKeyDetails != nil && apiKeyDetails.CreatedBy != "test-user" { + if apiKeyDetails == nil { + t.Fatal("Expected API key details to be returned") + } + if apiKeyDetails.CreatedBy != "test-user" { t.Errorf("Expected CreatedBy to be 'test-user', got: %s", apiKeyDetails.CreatedBy) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/apikey/api_key_hash_test.go` around lines 72 - 84, Change the non-fatal nil assertion to a fatal assertion so the test stops immediately on unexpected nil: in the ValidateAPIKey test where you check apiKeyDetails (variable apiKeyDetails returned by ValidateAPIKey), replace the t.Error("Expected API key details to be returned") with t.Fatal(...) so subsequent dereference of apiKeyDetails (CreatedBy check) cannot panic; keep the rest of the assertions the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common/apikey/store.go`:
- Around line 202-203: The ValidateAPIKey method currently returns the internal
*APIKey stored in apiKeysByAPI (variable targetAPIKey), which leaks mutable
state outside the lock; change ValidateAPIKey (in APIkeyStore) to return a
shallow copy of the APIKey instead of the original pointer: while still holding
the lock, copy the struct fields into a new APIKey value, deep-copy ExpiresAt if
necessary (create a new time.Time and take its address) and return a pointer to
that new copy so callers cannot mutate the store's internal object.
In `@gateway/system-policies/analytics/analytics.go`:
- Around line 246-252: The debug log incorrectly reads from
AuthContext.Properties instead of the typed UserID; update the slog.Debug call
in the analytics policy block that fills analyticsMetadata to log
ctx.SharedContext.AuthContext.UserID (the UserID field) rather than
ctx.SharedContext.AuthContext.Properties[AuthContextKeyUserID], keeping the
existing context key AuthContextKeyUserID and leaving analyticsMetadata
assignment as-is so the log accurately reflects the stored value.
---
Nitpick comments:
In `@common/apikey/api_key_hash_test.go`:
- Around line 72-84: Change the non-fatal nil assertion to a fatal assertion so
the test stops immediately on unexpected nil: in the ValidateAPIKey test where
you check apiKeyDetails (variable apiKeyDetails returned by ValidateAPIKey),
replace the t.Error("Expected API key details to be returned") with t.Fatal(...)
so subsequent dereference of apiKeyDetails (CreatedBy check) cannot panic; keep
the rest of the assertions the same.
In `@gateway/system-policies/analytics/analytics.go`:
- Around line 34-35: The constant AuthContextKeyUserID is ambiguously documented
as "Analytics metadata key" but is also used to read from AuthContext.Properties
and to populate analyticsMetadata; verify intended usage and either (A) update
the comment to reflect dual use (e.g., "AuthContext/Analytics metadata key for
user ID") so callers of AuthContextKeyUserID and readers of analyticsMetadata
are clear, or (B) if the auth property and analytics key should be distinct,
split into two explicit constants (e.g., AuthContextKeyUserID and
AnalyticsMetadataKeyUserID) and update all references where analyticsMetadata is
populated and where AuthContext.Properties is read to use the appropriate
constant (refer to usages in analyticsMetadata and AuthContext.Properties).
Comment on lines
+202
to
+203
| // Returns: (isValid bool, apiKey *APIKey, error) | ||
| func (aks *APIkeyStore) ValidateAPIKey(apiId, apiOperation, operationMethod, providedAPIKey string) (bool, *APIKey, error) { |
Contributor
There was a problem hiding this comment.
Returning a pointer to the internal store entry leaks mutable state outside the lock.
targetAPIKey on lines 271/279 is the same *APIKey pointer held in apiKeysByAPI. Callers that mutate the returned object after ValidateAPIKey returns will race with any concurrent store operation (the RWMutex no longer protects it). Consider returning a shallow copy instead.
♻️ Suggested approach
+ // Return a copy so callers cannot mutate internal store state
+ keyCopy := *targetAPIKey
// Check if wildcard is present
for _, op := range operations {
if strings.TrimSpace(op) == "*" {
- return true, targetAPIKey, nil
+ return true, &keyCopy, nil
}
}
// Check if the requested operation is in the allowed operations list
requestedOperation := fmt.Sprintf("%s %s", operationMethod, apiOperation)
for _, op := range operations {
if strings.TrimSpace(op) == requestedOperation {
- return true, targetAPIKey, nil
+ return true, &keyCopy, nil
}
}Note: A shallow copy is sufficient here since the mutable fields (Status, scalar strings, etc.) are value types. The only pointer field is ExpiresAt *time.Time; if callers might mutate it, a deep copy of that field would also be needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@common/apikey/store.go` around lines 202 - 203, The ValidateAPIKey method
currently returns the internal *APIKey stored in apiKeysByAPI (variable
targetAPIKey), which leaks mutable state outside the lock; change ValidateAPIKey
(in APIkeyStore) to return a shallow copy of the APIKey instead of the original
pointer: while still holding the lock, copy the struct fields into a new APIKey
value, deep-copy ExpiresAt if necessary (create a new time.Time and take its
address) and return a pointer to that new copy so callers cannot mutate the
store's internal object.
Comment on lines
+246
to
252
| // Extract user ID from AuthContext if available (set by auth policies) | ||
| if ctx.SharedContext.AuthContext != nil && ctx.SharedContext.AuthContext.UserID != "" { | ||
| analyticsMetadata[AuthContextKeyUserID] = ctx.SharedContext.AuthContext.UserID | ||
| slog.Debug("Analytics system policy: User ID extracted from AuthContext", | ||
| "userID", ctx.SharedContext.AuthContext.Properties[AuthContextKeyUserID], | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
Debug log reads from Properties map instead of the typed UserID field.
Line 248 correctly stores AuthContext.UserID in analytics metadata, but line 250 logs AuthContext.Properties[AuthContextKeyUserID], which may be empty or differ from the typed field. This makes the debug log misleading when troubleshooting.
🐛 Proposed fix
if ctx.SharedContext.AuthContext != nil && ctx.SharedContext.AuthContext.UserID != "" {
analyticsMetadata[AuthContextKeyUserID] = ctx.SharedContext.AuthContext.UserID
slog.Debug("Analytics system policy: User ID extracted from AuthContext",
- "userID", ctx.SharedContext.AuthContext.Properties[AuthContextKeyUserID],
+ "userID", ctx.SharedContext.AuthContext.UserID,
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Extract user ID from AuthContext if available (set by auth policies) | |
| if ctx.SharedContext.AuthContext != nil && ctx.SharedContext.AuthContext.UserID != "" { | |
| analyticsMetadata[AuthContextKeyUserID] = ctx.SharedContext.AuthContext.UserID | |
| slog.Debug("Analytics system policy: User ID extracted from AuthContext", | |
| "userID", ctx.SharedContext.AuthContext.Properties[AuthContextKeyUserID], | |
| ) | |
| } | |
| // Extract user ID from AuthContext if available (set by auth policies) | |
| if ctx.SharedContext.AuthContext != nil && ctx.SharedContext.AuthContext.UserID != "" { | |
| analyticsMetadata[AuthContextKeyUserID] = ctx.SharedContext.AuthContext.UserID | |
| slog.Debug("Analytics system policy: User ID extracted from AuthContext", | |
| "userID", ctx.SharedContext.AuthContext.UserID, | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@gateway/system-policies/analytics/analytics.go` around lines 246 - 252, The
debug log incorrectly reads from AuthContext.Properties instead of the typed
UserID; update the slog.Debug call in the analytics policy block that fills
analyticsMetadata to log ctx.SharedContext.AuthContext.UserID (the UserID field)
rather than ctx.SharedContext.AuthContext.Properties[AuthContextKeyUserID],
keeping the existing context key AuthContextKeyUserID and leaving
analyticsMetadata assignment as-is so the log accurately reflects the stored
value.
Contributor
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)
common/apikey/store.go (1)
172-191:⚠️ Potential issue | 🔴 CriticalCritical compilation errors: three return sites not updated for the new 3-value signature.
Lines 179, 185, and 191 still use the old 2-value return shape and will fail to compile against the new
(bool, *APIKey, error)signature:
- Line 179:
return false, fmt.Errorf("API key is empty")— missingnilfor*APIKey- Line 185:
return false, fmt.Errorf("failed to compute API key hash")— missingnilfor*APIKey- Line 191:
return false, ErrNotFound— missingnilfor*APIKey🐛 Proposed fix
if providedAPIKey == "" { - return false, fmt.Errorf("API key is empty") + return false, nil, fmt.Errorf("API key is empty") } // Compute hash for lookup (hash the full API key value as-is) hash := ComputeAPIKeyHash(providedAPIKey) if hash == "" { - return false, fmt.Errorf("failed to compute API key hash") + return false, nil, fmt.Errorf("failed to compute API key hash") } // Single unified O(1) lookup by hash targetAPIKey, exists := aks.apiKeysByAPI[apiId][hash] if !exists { - return false, ErrNotFound + return false, nil, ErrNotFound }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/apikey/store.go` around lines 172 - 191, The ValidateAPIKey function's return sites were not updated to the new three-value signature (bool, *APIKey, error); update each old two-value return in ValidateAPIKey to return the missing *APIKey nil as the second value (e.g., change returns like `return false, fmt.Errorf(...)` and `return false, ErrNotFound` to `return false, nil, fmt.Errorf(...)` and `return false, nil, ErrNotFound`) so all code paths in ValidateAPIKey provide (bool, *APIKey, error).
🧹 Nitpick comments (3)
common/apikey/store.go (2)
173-174: Write lock onValidateAPIKeyserializes all concurrent validations.The sole write inside this function is
targetAPIKey.Status = Expired(line 206), which marks a key as expired in place. Every API request that hits validation now contends on a single exclusive lock rather than being served by concurrent read locks.Consider dropping the in-place status mutation and restoring
RLock; expiration detection can be purely observational:♻️ Proposed approach
- aks.mu.Lock() - defer aks.mu.Unlock() + aks.mu.RLock() + defer aks.mu.RUnlock() // ... // Check if the API key has expired if targetAPIKey.Status == Expired || (targetAPIKey.ExpiresAt != nil && time.Now().After(*targetAPIKey.ExpiresAt)) { - targetAPIKey.Status = Expired return false, nil, nil }If persisting the
Expiredstatus transition is a requirement, do it via a separate dedicated method (e.g.,MarkExpired) called asynchronously or after the fact rather than inside the validation hot path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/apikey/store.go` around lines 173 - 174, The ValidateAPIKey function currently acquires a full write lock (aks.mu.Lock()) which serializes all validations because it mutates targetAPIKey.Status = Expired; change the locking to a read lock (aks.mu.RLock()/RUnlock()) in ValidateAPIKey and make expiration detection purely observational (do not mutate targetAPIKey.Status in-place inside ValidateAPIKey); if you must persist the Expired transition, add a separate method (e.g., MarkExpired or SetAPIKeyExpired) that acquires the write lock and updates the status outside the hot validation path (call it asynchronously or after validation) so concurrent validations use shared read locks instead of a global write lock.
200-208:targetAPIKey.Status == Expiredcheck on line 205 is unreachable dead code.The preceding guard (lines 200-202) already returns for any status other than
Active. By the time execution reaches line 205,Statusis guaranteed to beActive, making the== Expiredsub-expression always false.♻️ Proposed simplification
- if targetAPIKey.Status == Expired || (targetAPIKey.ExpiresAt != nil && time.Now().After(*targetAPIKey.ExpiresAt)) { + if targetAPIKey.ExpiresAt != nil && time.Now().After(*targetAPIKey.ExpiresAt) { targetAPIKey.Status = Expired return false, nil, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/apikey/store.go` around lines 200 - 208, The check "targetAPIKey.Status == Expired" is unreachable because you already return whenever targetAPIKey.Status != Active; remove that sub-expression and only check expiration via ExpiresAt/time.Now: change the if in the expiration block to test "(targetAPIKey.ExpiresAt != nil && time.Now().After(*targetAPIKey.ExpiresAt))", set targetAPIKey.Status = Expired and return false,nil,nil when expired; ensure references are to targetAPIKey.Status, Active, Expired, targetAPIKey.ExpiresAt and time.Now().gateway/system-policies/analytics/analytics.go (1)
1-1: Reminder: rebuild Docker images after gateway component changes.Both modified files fall under
gateway/**. As per coding guidelines, runcd gateway && make build-localto rebuild the affected Docker images.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway/system-policies/analytics/analytics.go` at line 1, This change touches the gateway component (package analytics in gateway/system-policies/analytics/analytics.go); after committing your edits rebuild the gateway Docker images by running: cd gateway && make build-local so the updated analytics package is included in the rebuilt images and local integration tests pick up your changes.
🤖 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 `@common/apikey/store.go`:
- Around line 172-191: The ValidateAPIKey function's return sites were not
updated to the new three-value signature (bool, *APIKey, error); update each old
two-value return in ValidateAPIKey to return the missing *APIKey nil as the
second value (e.g., change returns like `return false, fmt.Errorf(...)` and
`return false, ErrNotFound` to `return false, nil, fmt.Errorf(...)` and `return
false, nil, ErrNotFound`) so all code paths in ValidateAPIKey provide (bool,
*APIKey, error).
---
Duplicate comments:
In `@common/apikey/store.go`:
- Around line 221-229: ValidateAPIKey currently returns the internal pointer
targetAPIKey from the apiKeysByAPI map, leaking mutable internal state; fix by
making and returning a shallow copy of the APIKey struct (e.g. copy :=
*targetAPIKey; copiedPtr := ©) instead of targetAPIKey, and do this copy
while still holding the store lock in both return paths (the block returning
true, targetAPIKey, nil and the later block returning true, targetAPIKey, nil)
so callers receive an independent struct pointer that cannot be mutated to
affect store internals.
In `@gateway/system-policies/analytics/analytics.go`:
- Around line 246-252: The debug log is still reading from
AuthContext.Properties which can be empty; update the slog.Debug call in the
block that populates analyticsMetadata (the check using
ctx.SharedContext.AuthContext and ctx.SharedContext.AuthContext.UserID) to log
ctx.SharedContext.AuthContext.UserID instead of
ctx.SharedContext.AuthContext.Properties[AuthContextKeyUserID] (keep the
analyticsMetadata assignment to AuthContextKeyUserID unchanged and ensure the
slog.Debug key/value pair uses the typed UserID field).
---
Nitpick comments:
In `@common/apikey/store.go`:
- Around line 173-174: The ValidateAPIKey function currently acquires a full
write lock (aks.mu.Lock()) which serializes all validations because it mutates
targetAPIKey.Status = Expired; change the locking to a read lock
(aks.mu.RLock()/RUnlock()) in ValidateAPIKey and make expiration detection
purely observational (do not mutate targetAPIKey.Status in-place inside
ValidateAPIKey); if you must persist the Expired transition, add a separate
method (e.g., MarkExpired or SetAPIKeyExpired) that acquires the write lock and
updates the status outside the hot validation path (call it asynchronously or
after validation) so concurrent validations use shared read locks instead of a
global write lock.
- Around line 200-208: The check "targetAPIKey.Status == Expired" is unreachable
because you already return whenever targetAPIKey.Status != Active; remove that
sub-expression and only check expiration via ExpiresAt/time.Now: change the if
in the expiration block to test "(targetAPIKey.ExpiresAt != nil &&
time.Now().After(*targetAPIKey.ExpiresAt))", set targetAPIKey.Status = Expired
and return false,nil,nil when expired; ensure references are to
targetAPIKey.Status, Active, Expired, targetAPIKey.ExpiresAt and time.Now().
In `@gateway/system-policies/analytics/analytics.go`:
- Line 1: This change touches the gateway component (package analytics in
gateway/system-policies/analytics/analytics.go); after committing your edits
rebuild the gateway Docker images by running: cd gateway && make build-local so
the updated analytics package is included in the rebuilt images and local
integration tests pick up your changes.
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
2 participants
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.
Relates to : wso2/gateway-controllers#36
#968
Summary
Changes
common/apikey/store.go
of (bool, error)
access metadata like CreatedBy, ExpiresAt, etc.
or nil as appropriate
common/apikey/api_key_hash_test.go
signature
values (e.g., CreatedBy field)
Motivation
This enhancement provides callers with access to the full API key metadata after
successful validation, enabling use cases such as audit logging, usage tracking, and
authorization decisions based on key properties without requiring additional lookups
Summary by CodeRabbit
New Features
Improvements