refactor: rework oidc session storage#913
Open
steveiliop56 wants to merge 14 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughConsolidates OIDC storage into a single oidc_sessions table and refactors the OIDC flow to use in-memory authorization-code caching with session-based token persistence; updates repositories, sqlc queries, migrations, controller wiring, service logic, and tests accordingly. ChangesOIDC Sessions Consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 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 docstrings
🧪 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
dosubot
Bot
added
the
size:XXL
![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: 3
🧹 Nitpick comments (3)
internal/service/oidc_service.go (2)
311-315: 💤 Low valueTypo in variable name:
codeCash→codeCache.Minor naming inconsistency that could cause confusion during future maintenance.
✏️ Suggested fix
// Create caches - codeCash := NewCacheStore[AuthorizeCodeEntry](256) + codeCache := NewCacheStore[AuthorizeCodeEntry](256) usedCode := NewCacheStore[UsedCodeEntry](256) - service.caches.code = codeCash + service.caches.code = codeCache service.caches.usedCode = usedCode🤖 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/oidc_service.go` around lines 311 - 315, The local variable name "codeCash" is a typo; rename it to "codeCache" where it's declared and assigned so it reads: codeCache := NewCacheStore[AuthorizeCodeEntry](256) and then assign service.caches.code = codeCache; ensure any other references in this scope use the new identifier; leave usedCode and service.caches.usedCode unchanged.
571-571: 💤 Low valueRedundant
strings.ReplaceAllfor scope delimiter.
codeEntry.Scopeis already space-separated (set inCreateCodeat line 400 viastrings.Join(..., " ")), so this comma-to-space replacement is unnecessary. The same pattern appears at line 650 inRefreshAccessToken.✏️ Suggested fix
tokenResponse := TokenResponse{ AccessToken: accessToken, RefreshToken: refreshToken, TokenType: "Bearer", ExpiresIn: int64(service.config.Auth.SessionExpiry), IDToken: idToken, - Scope: strings.ReplaceAll(codeEntry.Scope, ",", " "), + Scope: codeEntry.Scope, }And at line 650:
- Scope: strings.ReplaceAll(entry.Scope, ",", " "), + Scope: entry.Scope,🤖 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/oidc_service.go` at line 571, The strings.ReplaceAll(..., ",", " ") call that sets Scope when building the token response is redundant because codeEntry.Scope is already built with spaces in CreateCode (via strings.Join(..., " ")); remove the ReplaceAll and assign Scope directly from codeEntry.Scope in the token creation path (and apply the same change in the RefreshAccessToken code path where the same ReplaceAll is used) so both places use codeEntry.Scope unchanged.internal/assets/migrations/postgres/000002_oidc_rework.up.sql (1)
18-28: 💤 Low valueRedundant
UNIQUEconstraint onsubcolumn.
PRIMARY KEYalready enforces uniqueness, so the explicitUNIQUEconstraint onsubis redundant.Suggested fix
CREATE TABLE IF NOT EXISTS "oidc_sessions" ( - "sub" TEXT NOT NULL UNIQUE PRIMARY KEY, + "sub" TEXT NOT NULL PRIMARY KEY, "access_token_hash" TEXT NOT NULL UNIQUE,🤖 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/assets/migrations/postgres/000002_oidc_rework.up.sql` around lines 18 - 28, The "oidc_sessions" table defines the "sub" column with both UNIQUE and PRIMARY KEY which is redundant; remove the explicit UNIQUE constraint from the "sub" column definition so it remains only as PRIMARY KEY (leave other columns and constraints unchanged) by editing the CREATE TABLE for oidc_sessions and deleting the UNIQUE keyword on "sub".
🤖 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 `@internal/controller/oidc_controller.go`:
- Around line 334-335: The call to controller.oidc.DeleteSessionBySub in the
code-reuse branch currently ignores any returned error; update the handler so
the returned error is checked and logged (using controller.log.App.Warn() or
.Err()) while keeping the same response behavior. Locate the code around
controller.log.App.Warn().Msg("Code reuse detected") and the
controller.oidc.DeleteSessionBySub(c, usedCodeSub) call, capture the error value
from DeleteSessionBySub, and emit an appropriate log entry (including the error
and context like usedCodeSub) so failures to delete sessions are visible for
debugging.
In `@internal/service/oidc_service.go`:
- Around line 686-690: The deletion is calling the wrong query: Replace the call
to DeleteSession(ctx, entry.Sub) (which targets the non-OIDC sessions table
using UUID) with service.queries.DeleteOIDCSessionBySub(ctx, entry.Sub) so the
code deletes from oidc_sessions using the OIDC subject; update the call site in
the code that references DeleteSession to call DeleteOIDCSessionBySub and keep
passing entry.Sub and the same ctx.
In `@sql/sqlite/oidc_schemas.sql`:
- Line 9: The "nonce" column in the SQLite schema is missing a NOT NULL
constraint causing divergence from PostgreSQL; update the schema entry for the
"nonce" column (currently '"nonce" TEXT DEFAULT ""') to include NOT NULL so it
becomes '"nonce" TEXT NOT NULL DEFAULT ""', ensuring SQLite enforces the same
non-null behavior as PostgreSQL.
---
Nitpick comments:
In `@internal/assets/migrations/postgres/000002_oidc_rework.up.sql`:
- Around line 18-28: The "oidc_sessions" table defines the "sub" column with
both UNIQUE and PRIMARY KEY which is redundant; remove the explicit UNIQUE
constraint from the "sub" column definition so it remains only as PRIMARY KEY
(leave other columns and constraints unchanged) by editing the CREATE TABLE for
oidc_sessions and deleting the UNIQUE keyword on "sub".
In `@internal/service/oidc_service.go`:
- Around line 311-315: The local variable name "codeCash" is a typo; rename it
to "codeCache" where it's declared and assigned so it reads: codeCache :=
NewCacheStore[AuthorizeCodeEntry](256) and then assign service.caches.code =
codeCache; ensure any other references in this scope use the new identifier;
leave usedCode and service.caches.usedCode unchanged.
- Line 571: The strings.ReplaceAll(..., ",", " ") call that sets Scope when
building the token response is redundant because codeEntry.Scope is already
built with spaces in CreateCode (via strings.Join(..., " ")); remove the
ReplaceAll and assign Scope directly from codeEntry.Scope in the token creation
path (and apply the same change in the RefreshAccessToken code path where the
same ReplaceAll is used) so both places use codeEntry.Scope unchanged.
🪄 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: 2dcda4c5-94aa-4539-962c-9882d880462a
📒 Files selected for processing (26)
.env.exampleinternal/assets/migrations/postgres/000002_oidc_rework.down.sqlinternal/assets/migrations/postgres/000002_oidc_rework.up.sqlinternal/assets/migrations/sqlite/000010_oidc_rework.down.sqlinternal/assets/migrations/sqlite/000010_oidc_rework.up.sqlinternal/controller/oidc_controller.gointernal/repository/memory/memory_test.gointernal/repository/memory/oidc_queries.gointernal/repository/memory/store.gointernal/repository/models.gointernal/repository/postgres/models.gointernal/repository/postgres/oidc_queries.sql.gointernal/repository/postgres/store.gointernal/repository/sqlite/models.gointernal/repository/sqlite/oidc_queries.sql.gointernal/repository/sqlite/store.gointernal/repository/store.gointernal/service/oauth_extractors.gointernal/service/oauth_service.gointernal/service/oidc_service.gointernal/service/oidc_service_test.gosql/postgres/oidc_queries.sqlsql/postgres/oidc_schemas.sqlsql/sqlite/oidc_queries.sqlsql/sqlite/oidc_schemas.sqlsqlc.yml
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.
Summary by CodeRabbit
New Features
Refactor