feat: Create API Token Expiration Parameter. BED-7448#2420
feat: Create API Token Expiration Parameter. BED-7448#2420RaymondLaubert wants to merge 11 commits intomainfrom
Conversation
RaymondLaubert
added
enhancement
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThese changes introduce API token expiration functionality by adding a new configuration parameter (auth.api_token_expiration) with a model definition, database migration, and expiration validation during request signature verification. The feature includes a test case validating parameter retrieval. 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)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/api/src/database/parameters_test.go (1)
221-223: Use a non-defaultexpiration_periodto harden this test.Using
expirationPeriod = 90matches the function default, so the period assertion is weaker against fallback behavior.💡 Suggested tweak
- expirationPeriod = 90 + expirationPeriod = 30Also applies to: 235-236
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/database/parameters_test.go` around lines 221 - 223, The test sets expirationPeriod = 90 which equals the production/default value and weakens assertions; change expirationPeriod to a non-default value (e.g., 30 or another distinct integer) in the test so the assertion validates the test input rather than the function default, and update both occurrences referenced (the declaration with apiKeyExpiration/expirationPeriod and the later use at lines 235-236) to use the new non-default value; ensure any assertions that compare against the expected period use this new constant (expirationPeriod) so the test remains consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/api/src/database/parameters_test.go`:
- Around line 233-236: The test assigns to valObtained without declaring it;
change the assignment to declare the variable (e.g., use valObtained :=
appcfg.GetAPITokenExpirationParameter(testCtx, db)) or explicitly declare var
valObtained <ReturnType> then assign, so valObtained is in scope for the
subsequent require.Equal checks that reference it and aligns with the return
type of GetAPITokenExpirationParameter.
In `@cmd/api/src/model/appcfg/parameter.go`:
- Around line 584-601: The GetAPITokenExpirationParameter function must validate
the mapped ExpirationPeriod to prevent zero/negative (and unreasonably large)
values from being used; after cfg.Map(&result) succeeds, check
result.ExpirationPeriod and if it's < 1 or > 3650 (or whatever agreed max)
replace it with the default (90) and emit a slog.WarnContext including the
invalid value and APITokenExpiration key; keep APITokenExpirationParameter,
GetAPITokenExpirationParameter, APITokenExpiration, and ParameterService as the
touch points and perform the validation immediately after cfg.Map so runtime
config cannot set invalid expirations.
---
Nitpick comments:
In `@cmd/api/src/database/parameters_test.go`:
- Around line 221-223: The test sets expirationPeriod = 90 which equals the
production/default value and weakens assertions; change expirationPeriod to a
non-default value (e.g., 30 or another distinct integer) in the test so the
assertion validates the test input rather than the function default, and update
both occurrences referenced (the declaration with
apiKeyExpiration/expirationPeriod and the later use at lines 235-236) to use the
new non-default value; ensure any assertions that compare against the expected
period use this new constant (expirationPeriod) so the test remains consistent.
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/api/src/database/migration/migrations/v8.7.0.sqlcmd/api/src/database/parameters_test.gocmd/api/src/model/appcfg/parameter.go
superlinkx
left a comment
superlinkx
left a comment
There was a problem hiding this comment.
Looks like the tests won't even compile currently due to syntax errors. Aside from that, CodeRabbit is correct in its comments and we should address them.
Additionally, I'm seeing the parameter configuration here, but I'm not seeing how we ensure each API token created after the parameter is enabled is getting the expires_at added.
Finally, we also need invalidation logic to make sure expired tokens are no longer accepted by the auth system.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/api/src/database/parameters_test.go (1)
219-220: Prefer a richer local variable name thandbin new test code.Renaming
dbto something more descriptive (for exampledatabaseInterfaceordatabaseInstance) would align with the repository’s Go naming guideline.Proposed rename
- db = integration.SetupDB(t) + databaseInstance = integration.SetupDB(t) ... - require.Nil(t, db.SetConfigurationParameter(testCtx, appcfg.Parameter{ + require.Nil(t, databaseInstance.SetConfigurationParameter(testCtx, appcfg.Parameter{ ... - valObtained := appcfg.GetAPITokenExpirationParameter(testCtx, db) + valObtained := appcfg.GetAPITokenExpirationParameter(testCtx, databaseInstance)As per coding guidelines: "Prefer rich variable names, for example:
databaseInterfaceinstead ofdiordbi".Also applies to: 233-233
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/database/parameters_test.go` around lines 219 - 220, Rename the local variable named db in the test setup to a more descriptive name (e.g., databaseInstance or databaseInterface) wherever it appears in this file (notably the declaration alongside testCtx in parameters_test.go and the other occurrence at the later test block referenced), and update all references to that variable within the test functions (so uses of db are replaced with the new name) to follow the repository naming guideline for richer variable names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/api/src/database/parameters_test.go`:
- Around line 221-236: The test uses expirationPeriod = 90 which matches the
accessor's default and can yield false positives; change the test to use a
non-default expirationPeriod (e.g. 30) when constructing the JSONB via
types.NewJSONBObject and storing it with db.SetConfigurationParameter for
appcfg.APITokenExpiration, then assert that
appcfg.GetAPITokenExpirationParameter(testCtx, db) (valObtained) returns the new
non-default ExpirationPeriod to ensure DB-backed retrieval is actually
validated.
---
Nitpick comments:
In `@cmd/api/src/database/parameters_test.go`:
- Around line 219-220: Rename the local variable named db in the test setup to a
more descriptive name (e.g., databaseInstance or databaseInterface) wherever it
appears in this file (notably the declaration alongside testCtx in
parameters_test.go and the other occurrence at the later test block referenced),
and update all references to that variable within the test functions (so uses of
db are replaced with the new name) to follow the repository naming guideline for
richer variable names.
2cd90e5 to
9c5703c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/api/src/model/appcfg/parameter.go (1)
592-595: Use a richer variable name thancfgfor readabilityConsider renaming
cfgto something explicit likeconfigurationParameterin this new code path.✏️ Suggested rename
- if cfg, err := service.GetConfigurationParameter(ctx, APITokenExpiration); err != nil { + if configurationParameter, err := service.GetConfigurationParameter(ctx, APITokenExpiration); err != nil { slog.WarnContext(ctx, "Failed to fetch API tokens expiration configuration; returning default values.") - } else if err := cfg.Map(&result); err != nil { + } else if err := configurationParameter.Map(&result); err != nil { slog.WarnContext(ctx, "Invalid API tokens expiration configuration supplied, returning default values.", slog.String("invalid_configuration", err.Error()), slog.String("parameter_key", string(APITokenExpiration))) } else if result.ExpirationPeriod <= 0 || result.ExpirationPeriod > 365 {As per coding guidelines: "Prefer rich variable names, for example:
databaseInterfaceinstead ofdiordbi."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/model/appcfg/parameter.go` around lines 592 - 595, Rename the short variable cfg in the GetConfigurationParameter call to a more descriptive name (e.g., configurationParameter or apiTokenExpirationParameter) to improve readability: update the variable used in the if err := configurationParameter.Map(&result); err != nil { branch and the preceding error check that receives the value from service.GetConfigurationParameter(ctx, APITokenExpiration) so all references (the GetConfigurationParameter result and the subsequent Map call) use the new descriptive identifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/api/src/model/appcfg/parameter.go`:
- Around line 592-603: The cfg.Map(&result) error path logs that defaults will
be returned but doesn't reset the result struct, so partially unmarshalled
fields (e.g., Enabled) may survive; update the error branch in the
APITokenExpiration handling (the cfg.Map error case inside the
GetConfigurationParameter block) to explicitly reset result to its default
values (reinitialize result to the expected defaults) before logging/returning
so callers receive a fully-defaulted result rather than a partially-populated
one.
---
Nitpick comments:
In `@cmd/api/src/model/appcfg/parameter.go`:
- Around line 592-595: Rename the short variable cfg in the
GetConfigurationParameter call to a more descriptive name (e.g.,
configurationParameter or apiTokenExpirationParameter) to improve readability:
update the variable used in the if err := configurationParameter.Map(&result);
err != nil { branch and the preceding error check that receives the value from
service.GetConfigurationParameter(ctx, APITokenExpiration) so all references
(the GetConfigurationParameter result and the subsequent Map call) use the new
descriptive identifier.
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/api/src/database/migration/migrations/v8.7.0.sqlcmd/api/src/database/parameters_test.gocmd/api/src/model/appcfg/parameter.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/src/database/parameters_test.go
RaymondLaubert
force-pushed
the
BED-7448/ApiKey-TokenExpirationParam
branch
from
9c5703c to
b37ca32
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/api/src/api/auth.go`:
- Around line 318-319: The token expiration check fails because NewUserAuthToken
does not set ExpiresAt; update the CreateAuthToken handler to read the
expiration value via GetAPITokenExpirationParameter and set authToken.ExpiresAt
= time.Now().Add(expirationDuration) (or the equivalent parsed value) before
persisting the token, so the stored token has a non-zero ExpiresAt and the
authToken.ExpiresAt.Before(time.Now()) check in auth.go works as intended;
ensure you reference and set the ExpiresAt field on the same token object used
by your persistence call.
- Around line 318-319: authToken.ExpiresAt is a null.Time (sql.NullTime) so you
must check ExpiresAt.Valid before using its Time; replace the direct call to
ExpiresAt.Before(time.Now()) with a validity check and compare ExpiresAt.Time to
serverTime (the injected clock) instead of time.Now(); update the conditional
around authToken.ExpiresAt to first ensure authToken.ExpiresAt.Valid is true,
then use authToken.ExpiresAt.Time.Before(serverTime) to return ErrApiKeyExpired
in the same branch (refer to authToken.ExpiresAt and serverTime in the
surrounding function).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f62e7c9e-bd32-413a-8151-acea14bd9b66
📒 Files selected for processing (4)
cmd/api/src/api/auth.gocmd/api/src/database/migration/migrations/v8.8.0.sqlcmd/api/src/database/parameters_test.gocmd/api/src/model/appcfg/parameter.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/api/src/database/parameters_test.go
- cmd/api/src/model/appcfg/parameter.go
cmd/api/src/api/auth.go
Outdated
| } else if authToken.ExpiresAt.Before(time.Now()) { | ||
| return auth.Context{}, http.StatusUnauthorized, ErrApiKeyExpired |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n-- Token constructor --\n'
sed -n '183,216p' cmd/api/src/auth/model.go
printf '\n-- API token creation endpoint --\n'
sed -n '754,778p' cmd/api/src/api/v2/auth/auth.go
printf '\n-- Relevant matches --\n'
rg -n -C2 'NewUserAuthToken|GetAPITokenExpirationParameter|ExpiresAt' \
cmd/api/src/auth/model.go \
cmd/api/src/api/v2/auth/auth.go \
cmd/api/src/model/appcfg/parameter.goRepository: SpecterOps/BloodHound
Length of output: 6572
API token expiration feature is incomplete - tokens never receive expiration timestamps.
The validation at cmd/api/src/api/auth.go:318-319 checks if ExpiresAt is before current time, but NewUserAuthToken never sets this field. The CreateAuthToken endpoint must apply the expiration from GetAPITokenExpirationParameter before storing the token.
🧰 Tools
🪛 GitHub Actions: Run Go Unit Tests
[error] 318-318: authToken.ExpiresAt.Before undefined (type null.Time has no field or method Before).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/api/src/api/auth.go` around lines 318 - 319, The token expiration check
fails because NewUserAuthToken does not set ExpiresAt; update the
CreateAuthToken handler to read the expiration value via
GetAPITokenExpirationParameter and set authToken.ExpiresAt =
time.Now().Add(expirationDuration) (or the equivalent parsed value) before
persisting the token, so the stored token has a non-zero ExpiresAt and the
authToken.ExpiresAt.Before(time.Now()) check in auth.go works as intended;
ensure you reference and set the ExpiresAt field on the same token object used
by your persistence call.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n-- AuthToken.ExpiresAt declaration --\n'
sed -n '147,157p' cmd/api/src/model/auth.go
printf '\n-- Expiration check in ValidateRequestSignature --\n'
sed -n '314,320p' cmd/api/src/api/auth.goRepository: SpecterOps/BloodHound
Length of output: 1082
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n-- Full ValidateRequestSignature function to find serverTime --\n'
# Search for the function and look for serverTime usage
rg -A 200 'func \(s \*Service\) ValidateRequestSignature' cmd/api/src/api/auth.go | head -100Repository: SpecterOps/BloodHound
Length of output: 129
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n-- Search for ValidateRequestSignature function --\n'
rg 'ValidateRequestSignature' cmd/api/src/api/auth.go -B 2 -A 50 | head -80
printf '\n-- Search for serverTime variable in auth.go --\n'
rg 'serverTime' cmd/api/src/api/auth.goRepository: SpecterOps/BloodHound
Length of output: 4671
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n-- Check null.Time type definition --\n'
# Find where null.Time is imported from
rg 'import.*null' cmd/api/src/model/auth.go | head -5
printf '\n-- Search for null package usage to understand structure --\n'
rg -A 10 'type null\.Time' cmd/api/src/model/ 2>/dev/null || echo "Not found in model, checking imports"Repository: SpecterOps/BloodHound
Length of output: 103
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n-- Check imports in auth.go model file --\n'
head -30 cmd/api/src/model/auth.go
printf '\n-- Check if null.Time can call Before() method --\n'
# Search for any documentation or usage of null.Time with Before
rg 'null\.Time.*Before' cmd/api/src/Repository: SpecterOps/BloodHound
Length of output: 1030
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n-- Check null.Time type definition --\n'
find cmd/api/src/database/types -name "*.go" -type f | head -20
printf '\n-- Look at the null package --\n'
cat cmd/api/src/database/types/null.go | head -100Repository: SpecterOps/BloodHound
Length of output: 949
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n-- Check null.Time definition --\n'
cat cmd/api/src/database/types/null/time.goRepository: SpecterOps/BloodHound
Length of output: 4291
Fix null.Time access with Valid check and use serverTime instead of time.Now().
model.AuthToken.ExpiresAt is a null.Time which embeds sql.NullTime. The type does not have a Before() method—only the underlying time.Time does. Additionally, without checking the Valid field, any token with a NULL expires_at would default to the zero time.Time, incorrectly marking it as expired. Use the serverTime parameter instead of time.Now() to remain consistent with the injected clock throughout the method.
Proposed fix
- } else if authToken.ExpiresAt.Before(time.Now()) {
+ } else if authToken.ExpiresAt.Valid && authToken.ExpiresAt.Time.Before(serverTime) {
return auth.Context{}, http.StatusUnauthorized, ErrApiKeyExpired📝 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.
| } else if authToken.ExpiresAt.Before(time.Now()) { | |
| return auth.Context{}, http.StatusUnauthorized, ErrApiKeyExpired | |
| } else if authToken.ExpiresAt.Valid && authToken.ExpiresAt.Time.Before(serverTime) { | |
| return auth.Context{}, http.StatusUnauthorized, ErrApiKeyExpired |
🧰 Tools
🪛 GitHub Actions: Run Go Unit Tests
[error] 318-318: authToken.ExpiresAt.Before undefined (type null.Time has no field or method Before).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/api/src/api/auth.go` around lines 318 - 319, authToken.ExpiresAt is a
null.Time (sql.NullTime) so you must check ExpiresAt.Valid before using its
Time; replace the direct call to ExpiresAt.Before(time.Now()) with a validity
check and compare ExpiresAt.Time to serverTime (the injected clock) instead of
time.Now(); update the conditional around authToken.ExpiresAt to first ensure
authToken.ExpiresAt.Valid is true, then use
authToken.ExpiresAt.Time.Before(serverTime) to return ErrApiKeyExpired in the
same branch (refer to authToken.ExpiresAt and serverTime in the surrounding
function).
2 participants
Description
Creates the API token expiration parameter which will allow for expiring API tokens to be enabled/disabled.
Motivation and Context
Resolves BED-7448
This change resolves the request for API tokens that are able to be expired automatically after a set time frame.
How Has This Been Tested?
Created new unit tests that tests setting and obtaining the parameter.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Tests