jti, auth_code and auth_req store is using RuntimeStoreProvider - #4172
Merged
Conversation
anushasunkada
requested review from
ThaminduDilshan,
darshanasbg,
rajithacharith and
thiva-k
as code owners
July 20, 2026 11:24
Contributor
|
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:
📝 WalkthroughWalkthroughOAuth authorization, PAR, and JTI persistence move to the runtime store provider. Atomic conditional writes are added across runtime-store backends, service initialization receives shared runtime dependencies, and obsolete transient database objects and cleanup targets are removed. ChangesRuntime storage migration
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant ServerStartup
participant RuntimeStoreProvider
participant DPoPVerifier
participant OAuthService
participant AuthorizationStores
participant PARStore
ServerStartup->>RuntimeStoreProvider: Initialize runtime transient storage
ServerStartup->>DPoPVerifier: Initialize JTI replay protection with runtime store
ServerStartup->>OAuthService: Inject runtime store and transactioner
OAuthService->>AuthorizationStores: Create authorization code and request stores
OAuthService->>PARStore: Create PAR request store
AuthorizationStores->>RuntimeStoreProvider: Store and consume serialized OAuth data
PARStore->>RuntimeStoreProvider: Store and consume serialized PAR data
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 2
🧹 Nitpick comments (2)
backend/internal/oauth/oauth2/par/store.go (1)
47-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale "database"/"DB-backed" comments after runtime-store migration. These doc comments still describe the removed DB/Redis path and can mislead future maintainers of this security-sensitive OAuth code.
backend/internal/oauth/oauth2/par/store.go#L47: update "creates a new DB-backed PAR request store" to reflect the runtime store provider.backend/internal/oauth/oauth2/authz/auth_code_store.go#L49: "inserts a new authorization code into the database" should read as storing into the runtime store.backend/internal/oauth/oauth2/authz/auth_code_store.go#L67-L68: "false if a database error occurs" should reference the runtime store error path.🤖 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 `@backend/internal/oauth/oauth2/par/store.go` at line 47, Update the doc comments for the PAR request store constructor in backend/internal/oauth/oauth2/par/store.go (line 47) and the authorization-code methods in backend/internal/oauth/oauth2/authz/auth_code_store.go (lines 49 and 67-68) to describe the runtime store/provider rather than the removed database or Redis implementation; make no code changes.backend/internal/oauth/oauth2/jti/store.go (1)
27-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused import.
If you apply the proposed logic change below to remove
json.Marshal, this import will no longer be needed.🧹 Proposed fix
- "encoding/json" "fmt"🤖 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 `@backend/internal/oauth/oauth2/jti/store.go` around lines 27 - 28, Remove the unused encoding/json import from the oauth2 JTI store imports, while retaining fmt and all imports still referenced by the implementation.
🤖 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 `@backend/cmd/server/servicemanager.go`:
- Around line 253-255: Update the relevant deployment or migration documentation
for the runtime store initialization introduced around runtimestore.Initialize:
document migration to the RUNTIME_STORE table, removal of AUTHORIZATION_CODE,
AUTHORIZATION_REQUEST, PAR_REQUEST, and JTI_RECORD tables, and the upgrade
impact that in-flight login, recovery, or registration flows may be lost and
must be restarted.
In `@backend/internal/oauth/oauth2/jti/store.go`:
- Around line 74-82: Update the JTI storage flow around ttlSeconds and
storeProvider.Put: return successfully without storing when expiry is already
past, remove the unnecessary json.Marshal/value handling, and calculate the
remaining TTL with a minimum of one second for future expirations, including
sub-second durations. Preserve the existing error wrapping for storage failures.
---
Nitpick comments:
In `@backend/internal/oauth/oauth2/jti/store.go`:
- Around line 27-28: Remove the unused encoding/json import from the oauth2 JTI
store imports, while retaining fmt and all imports still referenced by the
implementation.
In `@backend/internal/oauth/oauth2/par/store.go`:
- Line 47: Update the doc comments for the PAR request store constructor in
backend/internal/oauth/oauth2/par/store.go (line 47) and the authorization-code
methods in backend/internal/oauth/oauth2/authz/auth_code_store.go (lines 49 and
67-68) to describe the runtime store/provider rather than the removed database
or Redis implementation; make no code changes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 358ff234-e12c-4fb5-89c1-145a098d7498
📒 Files selected for processing (35)
backend/cmd/server/servicemanager.gobackend/dbscripts/runtime-transient/postgres-cleanup.sqlbackend/dbscripts/runtime-transient/postgres.sqlbackend/dbscripts/runtime-transient/sqlite.sqlbackend/internal/oauth/init.gobackend/internal/oauth/oauth2/authz/authCodeRedisClient_mock_test.gobackend/internal/oauth/oauth2/authz/authReqRedisClient_mock_test.gobackend/internal/oauth/oauth2/authz/auth_code_redis_store.gobackend/internal/oauth/oauth2/authz/auth_code_redis_store_test.gobackend/internal/oauth/oauth2/authz/auth_code_store.gobackend/internal/oauth/oauth2/authz/auth_code_store_test.gobackend/internal/oauth/oauth2/authz/auth_req_redis_store.gobackend/internal/oauth/oauth2/authz/auth_req_redis_store_test.gobackend/internal/oauth/oauth2/authz/auth_req_store.gobackend/internal/oauth/oauth2/authz/auth_req_store_test.gobackend/internal/oauth/oauth2/authz/init.gobackend/internal/oauth/oauth2/authz/init_test.gobackend/internal/oauth/oauth2/authz/store_constants.gobackend/internal/oauth/oauth2/jti/StoreInterface_mock_test.gobackend/internal/oauth/oauth2/jti/init.gobackend/internal/oauth/oauth2/jti/redisClient_mock_test.gobackend/internal/oauth/oauth2/jti/redis_store.gobackend/internal/oauth/oauth2/jti/redis_store_test.gobackend/internal/oauth/oauth2/jti/store.gobackend/internal/oauth/oauth2/jti/store_constants.gobackend/internal/oauth/oauth2/jti/store_test.gobackend/internal/oauth/oauth2/par/init.gobackend/internal/oauth/oauth2/par/parRedisClient_mock_test.gobackend/internal/oauth/oauth2/par/redis_store.gobackend/internal/oauth/oauth2/par/redis_store_test.gobackend/internal/oauth/oauth2/par/store.gobackend/internal/oauth/oauth2/par/store_constants.gobackend/internal/oauth/oauth2/par/store_test.gobackend/pkg/thunderidengine/engine.gobackend/scripts/cleanup_runtime_transient_db.sh
💤 Files with no reviewable changes (19)
- backend/internal/oauth/oauth2/authz/auth_code_redis_store.go
- backend/internal/oauth/oauth2/par/parRedisClient_mock_test.go
- backend/internal/oauth/oauth2/jti/store_constants.go
- backend/internal/oauth/oauth2/par/store_constants.go
- backend/internal/oauth/oauth2/authz/authCodeRedisClient_mock_test.go
- backend/internal/oauth/oauth2/authz/auth_req_redis_store.go
- backend/dbscripts/runtime-transient/postgres-cleanup.sql
- backend/internal/oauth/oauth2/jti/StoreInterface_mock_test.go
- backend/internal/oauth/oauth2/par/redis_store.go
- backend/internal/oauth/oauth2/authz/auth_req_redis_store_test.go
- backend/internal/oauth/oauth2/par/redis_store_test.go
- backend/internal/oauth/oauth2/jti/redis_store.go
- backend/internal/oauth/oauth2/authz/store_constants.go
- backend/dbscripts/runtime-transient/postgres.sql
- backend/internal/oauth/oauth2/authz/auth_code_redis_store_test.go
- backend/internal/oauth/oauth2/jti/redis_store_test.go
- backend/dbscripts/runtime-transient/sqlite.sql
- backend/internal/oauth/oauth2/jti/redisClient_mock_test.go
- backend/internal/oauth/oauth2/authz/authReqRedisClient_mock_test.go
Comment on lines
253
to
255
| runtimeStoreProvider, transactioner, err := runtimestore.Initialize(runtime.Config.Database.RuntimeTransient.Type, | ||
| runtime.Config.Server.Identifier) | ||
| fatalOnError(ctx, logger, err, "Failed to initialize runtime store") |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🔴 Documentation Required
This PR introduces user-facing changes that are not covered by documentation updates under docs/.
Please update the relevant documentation before merging.
Missing documentation:
- Deployment settings / Migration guide: The migration to the new
RUNTIME_STOREtable and the removal of theAUTHORIZATION_CODE,AUTHORIZATION_REQUEST,PAR_REQUEST, andJTI_RECORDtables must be documented in the deployment or upgrade guides. - User-facing behavior changes: Document the upgrade impact in the release notes or migration guide, explaining that existing in-flight login, recovery, or registration flows may be lost and require users to restart them.
🤖 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 `@backend/cmd/server/servicemanager.go` around lines 253 - 255, Update the
relevant deployment or migration documentation for the runtime store
initialization introduced around runtimestore.Initialize: document migration to
the RUNTIME_STORE table, removal of AUTHORIZATION_CODE, AUTHORIZATION_REQUEST,
PAR_REQUEST, and JTI_RECORD tables, and the upgrade impact that in-flight login,
recovery, or registration flows may be lost and must be restarted.
Source: Path instructions
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
anushasunkada
force-pushed
the
local_main
branch
from
July 20, 2026 12:36
547e12c to
5f70bcb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@backend/internal/oauth/oauth2/authz/auth_code_store.go`:
- Around line 57-58: Update the auth-code persistence flow around ttlSeconds and
acs.storeProvider.Put to handle non-positive TTLs before storage: reject expired
codes or clamp the value to a positive TTL so Put never receives zero or a
negative expiry. Preserve normal TTL calculation and storage for unexpired
authorization codes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e589eb8a-7abe-4b26-a6a0-4d023f406ef7
📒 Files selected for processing (35)
backend/cmd/server/servicemanager.gobackend/dbscripts/runtime-transient/postgres-cleanup.sqlbackend/dbscripts/runtime-transient/postgres.sqlbackend/dbscripts/runtime-transient/sqlite.sqlbackend/internal/oauth/init.gobackend/internal/oauth/oauth2/authz/authCodeRedisClient_mock_test.gobackend/internal/oauth/oauth2/authz/authReqRedisClient_mock_test.gobackend/internal/oauth/oauth2/authz/auth_code_redis_store.gobackend/internal/oauth/oauth2/authz/auth_code_redis_store_test.gobackend/internal/oauth/oauth2/authz/auth_code_store.gobackend/internal/oauth/oauth2/authz/auth_code_store_test.gobackend/internal/oauth/oauth2/authz/auth_req_redis_store.gobackend/internal/oauth/oauth2/authz/auth_req_redis_store_test.gobackend/internal/oauth/oauth2/authz/auth_req_store.gobackend/internal/oauth/oauth2/authz/auth_req_store_test.gobackend/internal/oauth/oauth2/authz/init.gobackend/internal/oauth/oauth2/authz/init_test.gobackend/internal/oauth/oauth2/authz/store_constants.gobackend/internal/oauth/oauth2/jti/StoreInterface_mock_test.gobackend/internal/oauth/oauth2/jti/init.gobackend/internal/oauth/oauth2/jti/redisClient_mock_test.gobackend/internal/oauth/oauth2/jti/redis_store.gobackend/internal/oauth/oauth2/jti/redis_store_test.gobackend/internal/oauth/oauth2/jti/store.gobackend/internal/oauth/oauth2/jti/store_constants.gobackend/internal/oauth/oauth2/jti/store_test.gobackend/internal/oauth/oauth2/par/init.gobackend/internal/oauth/oauth2/par/parRedisClient_mock_test.gobackend/internal/oauth/oauth2/par/redis_store.gobackend/internal/oauth/oauth2/par/redis_store_test.gobackend/internal/oauth/oauth2/par/store.gobackend/internal/oauth/oauth2/par/store_constants.gobackend/internal/oauth/oauth2/par/store_test.gobackend/pkg/thunderidengine/engine.gobackend/scripts/cleanup_runtime_transient_db.sh
💤 Files with no reviewable changes (19)
- backend/internal/oauth/oauth2/par/parRedisClient_mock_test.go
- backend/internal/oauth/oauth2/authz/authCodeRedisClient_mock_test.go
- backend/internal/oauth/oauth2/authz/auth_req_redis_store_test.go
- backend/dbscripts/runtime-transient/postgres-cleanup.sql
- backend/internal/oauth/oauth2/par/redis_store.go
- backend/internal/oauth/oauth2/authz/auth_req_redis_store.go
- backend/internal/oauth/oauth2/jti/StoreInterface_mock_test.go
- backend/internal/oauth/oauth2/par/store_constants.go
- backend/internal/oauth/oauth2/authz/auth_code_redis_store.go
- backend/internal/oauth/oauth2/jti/redisClient_mock_test.go
- backend/internal/oauth/oauth2/authz/store_constants.go
- backend/internal/oauth/oauth2/authz/auth_code_redis_store_test.go
- backend/dbscripts/runtime-transient/postgres.sql
- backend/internal/oauth/oauth2/authz/authReqRedisClient_mock_test.go
- backend/internal/oauth/oauth2/jti/redis_store_test.go
- backend/dbscripts/runtime-transient/sqlite.sql
- backend/internal/oauth/oauth2/par/redis_store_test.go
- backend/internal/oauth/oauth2/jti/redis_store.go
- backend/internal/oauth/oauth2/jti/store_constants.go
🚧 Files skipped from review as they are similar to previous changes (14)
- backend/internal/oauth/init.go
- backend/internal/oauth/oauth2/jti/init.go
- backend/internal/oauth/oauth2/authz/init_test.go
- backend/cmd/server/servicemanager.go
- backend/pkg/thunderidengine/engine.go
- backend/internal/oauth/oauth2/authz/init.go
- backend/internal/oauth/oauth2/par/store_test.go
- backend/internal/oauth/oauth2/jti/store.go
- backend/internal/oauth/oauth2/jti/store_test.go
- backend/scripts/cleanup_runtime_transient_db.sh
- backend/internal/oauth/oauth2/par/store.go
- backend/internal/oauth/oauth2/authz/auth_req_store_test.go
- backend/internal/oauth/oauth2/authz/auth_req_store.go
- backend/internal/oauth/oauth2/authz/auth_code_store_test.go
thiva-k
added
Type/Improvement
breaking change
anushasunkada
force-pushed
the
local_main
branch
from
July 20, 2026 13:08
5f70bcb to
de77ae5
Compare
| return nil | ||
| } | ||
|
|
||
| // ConsumeAuthorizationCode atomically transitions an ACTIVE authorization code to INACTIVE. |
Member
There was a problem hiding this comment.
Shall we update this comment properly?
Member
There was a problem hiding this comment.
@anushasunkada shall we address this? Comment should start with ConsumeAuthorizationCode ...
Ideally this should be caught by the linter
Member
There was a problem hiding this comment.
@coderabbitai why go linter didn't report this? Provide short answer
Contributor
There was a problem hiding this comment.
@ThaminduDilshan, because this is a style convention, not a compiler error. It would only be flagged if the relevant doc-comment linter (for example, revive/golint) is enabled and configured to check it; the current lint configuration likely does not enforce that rule.
| // The runtime store's Put is an unconditional overwrite, so replay detection is done with a | ||
| // Get-then-Put check. This has a benign race under concurrent submissions of the same jti, but | ||
| // that's an acceptable trade-off for a replay cache. | ||
| func (s *jtiStore) RecordJTI( |
Member
There was a problem hiding this comment.
Hope this is the agreed upon approach for this and trade off is acceptable.
cc: @senthalan
Member
There was a problem hiding this comment.
I think it's better if we can introduce a new function in the RuntimeStoreProvider to support this.
PutIfNotExists(ctx context.Context, namespace RuntimeStoreNamespace, key string, value []byte, ttlSeconds int64) (bool, error)
Member
There was a problem hiding this comment.
@senthalan isn't PutIfNotExists give the idea insert only if a record doesn't exists? Here we insert a record if not exists, but updates the existing record if it exist and is not expired
Member
There was a problem hiding this comment.
Seems this is suggested matching redis SETNX semantics. Writes only if no live entry exists for the key; an expired entry is treated as absent. Returns true if the write was performed, false if a live entry blocked it.
So maybe PutIfNotExists is a suitable name here as PUT generally indicates a replace operation
senthalan
reviewed
Jul 20, 2026
Comment on lines
+73
to
+74
| ttlSeconds := int64(expiryTime.Sub(time.Now().UTC()).Seconds()) | ||
| err = authzRS.storeProvider.Put(ctx, providers.NamespaceAuthzReq, key, data, ttlSeconds) |
Member
There was a problem hiding this comment.
Suggested change
| ttlSeconds := int64(expiryTime.Sub(time.Now().UTC()).Seconds()) | |
| err = authzRS.storeProvider.Put(ctx, providers.NamespaceAuthzReq, key, data, ttlSeconds) | |
| err = authzRS.storeProvider.Put(ctx, providers.NamespaceAuthzReq, key, data, authzRS.validityPeriod) |
anushasunkada
force-pushed
the
local_main
branch
from
July 21, 2026 06:38
de77ae5 to
3c1453a
Compare
anushasunkada
force-pushed
the
local_main
branch
from
July 21, 2026 07:23
3c1453a to
aaea435
Compare
anushasunkada
force-pushed
the
local_main
branch
from
July 21, 2026 08:10
aaea435 to
361d060
Compare
ThaminduDilshan
previously approved these changes
Jul 21, 2026
senthalan
previously approved these changes
Jul 21, 2026
senthalan
enabled auto-merge
July 21, 2026 08:27
Signed-off-by: anushasunkada <anushasunkada@gmail.com>
auto-merge was automatically disabled
July 21, 2026 09:53
Head branch was pushed to by a user without write access
anushasunkada
dismissed stale reviews from senthalan and ThaminduDilshan
via
July 21, 2026 09:53
2964725
anushasunkada
force-pushed
the
local_main
branch
from
July 21, 2026 09:53
361d060 to
2964725
Compare
ThaminduDilshan
approved these changes
Jul 21, 2026
ThaminduDilshan
enabled auto-merge
July 21, 2026 09:58
This was referenced Jul 26, 2026
This was referenced Aug 2, 2026
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
4 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.
Purpose
jti, auth_code and auth_req store is updated to use RuntimeStoreProvider
🔧 Summary of Breaking Changes
AUTHORIZATION_CODEAUTHORIZATION_REQUESTPAR_REQUESTJTI_RECORDtables are removed from the runtime database, instead usesRUNTIME_STOREtable.💥 Impact
The data stored in the above listed tables will be lost. This is not a critical impact as it's have only the information related to the flow. The users might need go through the login, recovery or registration flows
🔄 Migration Guide
Create new table in the runtimestore
Approach
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
New Features
Maintenance