feat: implement session persistence control via JWT claims #66
Conversation
Add support for configuring session cookie expiration based on access token claims. Sessions can now be configured as either persistent (29-day expiration) or session-only (browser close). - Add KSP claim extraction from JWT access tokens in TokenStore - Configure Rails session expiration via TokenManager - Default to persistent sessions for security when claims missing - Add session_persistent? utility method to KindeSdk module - Include comprehensive error handling with secure fallbacks The implementation respects the ksp.persistence claim in access tokens, allowing dynamic session duration control per user session.
WalkthroughAdds Kinde Session Persistence (KSP): TokenStore tracks and serializes a persistent flag (derived from tokens), TokenManager applies Rails session expiry based on that flag, KindeSdk applies persistence during client creation and exposes session_persistent?. Tests covering persistence cases were added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant KindeSdk
participant Client
participant Current
participant TokenManager
participant RailsSession as Rails Session
App->>KindeSdk: client(tokens_hash, ...)
KindeSdk->>Client: instantiate Client
KindeSdk->>Current: read token_store, session
alt Current has token_store and session
KindeSdk->>TokenManager: apply_session_persistence(token_store.persistent, session)
TokenManager->>RailsSession: set options[:expire_after] = 29d (if persistent) or nil
TokenManager-->>KindeSdk: ok / rescued
else Missing token_store or session
KindeSdk-->>KindeSdk: skip persistence apply
end
KindeSdk-->>App: return Client
sequenceDiagram
autonumber
participant TokenStore
participant AccessToken as Access Token (JWT)
participant SessionData as Session Data
Note over TokenStore: On set_tokens / initialize
TokenStore->>AccessToken: inspect KSP claim
TokenStore-->>TokenStore: persistent = extract_ksp_persistence(...) || true
Note over TokenStore,SessionData: Persist/Restore
TokenStore->>SessionData: to_session => includes persistent
SessionData->>TokenStore: from_session(data)
alt data includes persistent
TokenStore-->>TokenStore: restore persistent from data
else no persistent in data
TokenStore->>AccessToken: derive persistence from token
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
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: 4
🧹 Nitpick comments (6)
lib/kinde_sdk/token_manager.rb (1)
90-96: Strengthen guard clause to handle nil session.options.The guard at Line 91 checks
session.options.is_a?(Hash), but ifsession.optionsreturnsnil, the check will passrespond_to?(:options)and then fail on.is_a?(Hash), potentially raising a NoMethodError before the rescue can catch it in some Ruby versions.Apply this diff to ensure nil-safety:
- return unless session.respond_to?(:options) && session.options.is_a?(Hash) + return unless session.respond_to?(:options) && session.options&.is_a?(Hash)lib/kinde_sdk/token_store.rb (1)
35-43: Simplify from_session by using a single key check.Lines 39-40 check both symbol and string keys (
session_data.key?(:persistent) || session_data.key?('persistent')). Sinceset_tokens(Line 14) already transforms keys to symbols viatransform_keys(&:to_sym), andto_session(Line 31) returns a hash with symbol keys, the string key check appears redundant.Consider simplifying to:
- if session_data.key?(:persistent) || session_data.key?('persistent') - store.instance_variable_set(:@persistent, session_data[:persistent] || session_data['persistent']) + if session_data.key?(:persistent) + store.instance_variable_set(:@persistent, session_data[:persistent]) endHowever, retaining both checks provides defense-in-depth if session data is deserialized from different sources (e.g., string-keyed session storage).
lib/kinde_sdk.rb (1)
126-126: Consider making apply_session_persistence public or using a documented pattern.Line 126 uses
TokenManager.send(:apply_session_persistence, ...)to call a private method. This bypasses Ruby's access control and makes the intent unclear.Options:
- Make
apply_session_persistencea public class method if it's part of the intended public API.- Add a public wrapper method in TokenManager if the current design is intentional.
- Document why
send()is necessary here.If this is by design (e.g., to keep the method private from external callers but allow module-level access), add a comment:
if Current.token_store && Current.session + # Call private method to apply session config (internal SDK use only) TokenManager.send(:apply_session_persistence, Current.token_store.persistent, Current.session) endspec/kinde_sdk/ksp_spec.rb (3)
35-37: Suggest using constant for magic number.The hardcoded value
2505600appears in multiple places in the spec. Consider usingKindeSdk::TokenStore::TWENTY_NINE_DAYS_SECONDSto improve maintainability and make the test's intent clearer.Apply this diff:
- it 'sets cookie expiration to 29 days' do - expect(store.cookie_expiration).to be_within(60).of(Time.now + 2505600) - end + it 'sets cookie expiration to 29 days' do + expect(store.cookie_expiration).to be_within(60).of(Time.now + KindeSdk::TokenStore::TWENTY_NINE_DAYS_SECONDS) + end
87-91: Use constant for magic number.Same as previous comment - replace the hardcoded
2505600withKindeSdk::TokenStore::TWENTY_NINE_DAYS_SECONDS.Apply this diff:
it 'sets expire_after to 29 days' do - expect(mock_session.options).to receive(:[]=).with(:expire_after, 2505600) + expect(mock_session.options).to receive(:[]=).with(:expire_after, KindeSdk::TokenStore::TWENTY_NINE_DAYS_SECONDS) token_manager.send(:apply_session_persistence, true, mock_session) end
202-209: Use constant for consistency.Replace the hardcoded
2505600withKindeSdk::TokenStore::TWENTY_NINE_DAYS_SECONDSfor consistency with other tests and the implementation.Apply this diff:
it 'uses 29 days in seconds (2505600) matching other SDKs' do store = KindeSdk::TokenStore.new({ access_token: persistent_token }) - expected_time = Time.now + 2505600 + expected_time = Time.now + KindeSdk::TokenStore::TWENTY_NINE_DAYS_SECONDS expect(store.cookie_expiration).to be_within(1).of(expected_time) end
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/kinde_sdk.rb(2 hunks)lib/kinde_sdk/token_manager.rb(1 hunks)lib/kinde_sdk/token_store.rb(1 hunks)spec/kinde_sdk/ksp_spec.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
lib/kinde_sdk.rb (1)
lib/kinde_sdk/client.rb (1)
tokens_hash(49-51)
lib/kinde_sdk/token_store.rb (2)
lib/kinde_sdk/token_manager.rb (3)
attr_reader(2-98)initialize(5-7)set_tokens(9-14)lib/kinde_sdk/client.rb (1)
initialize(31-39)
spec/kinde_sdk/ksp_spec.rb (2)
lib/kinde_sdk/token_store.rb (3)
cookie_expiration(48-50)to_session(26-33)from_session(35-43)lib/kinde_sdk.rb (2)
session_persistent?(223-225)client(120-130)
🪛 GitHub Actions: Ruby
lib/kinde_sdk.rb
[error] 120-120: NoMethodError: undefined method 'auto_refresh_tokens' for nil in KindeSdk.client.
spec/kinde_sdk/ksp_spec.rb
[error] 132-132: KindeSdk.session_persistent? with non-persistent token store returns false
[warning] 232-232: assigned but unused variable - new_expires_at
[warning] 270-270: assigned but unused variable - client_without_session
🔇 Additional comments (9)
lib/kinde_sdk/token_manager.rb (1)
82-85: LGTM!The integration of session persistence application within
sync_sessionis well-placed and correctly passes the store's persistent flag to configure the Rails session.lib/kinde_sdk/token_store.rb (4)
3-4: Verify TWENTY_NINE_DAYS_SECONDS constant value.The constant is set to 2505600 seconds. Let me verify this matches 29 days:
- 29 days × 24 hours × 60 minutes × 60 seconds = 29 × 86400 = 2,505,600 ✓
The value is correct.
58-70: JWT.decode without signature verification is appropriate for claim extraction.Line 61 calls
JWT.decode(access_token, nil, false)with verification disabled. This is secure in this context because:
- The token has already been validated via
KindeSdk.validate_jwt_tokenbefore being stored- This extraction happens after authentication
- Only reading a non-security-critical claim (ksp.persistence) that controls UX behavior, not authorization
- Fails safely to
true(persistent sessions) on any decode errorThe implementation correctly matches the TypeScript SDK pattern and provides secure defaults.
17-22: LGTM! KSP persistence extraction is correctly integrated.The extraction of the KSP claim from the access token is well-placed in
set_tokens, ensuring persistence is updated whenever tokens are refreshed or set. The logic correctly handles the case when@bearer_tokenis present.
64-66: Verify ksp.persistence claim logic matches specification.Line 66 implements:
ksp_claim.fetch('persistence', true) != falseThis returns
trueunlessksp.persistenceis explicitlyfalse. Confirm this matches the Kinde specification:
- If
ksp.persistenceis missing → defaulttrue✓- If
ksp.persistenceistrue→ returntrue✓- If
ksp.persistenceisfalse→ returnfalse✓- If
ksp.persistenceis any other value → returntrue✓The logic is correct and defensive.
spec/kinde_sdk/ksp_spec.rb (4)
1-24: LGTM! Clean test setup.The token generation setup correctly creates test tokens with varying KSP claims. Using JWT.encode with a simple secret is appropriate for integration specs.
102-110: Good defensive test.Testing graceful handling when session doesn't support options demonstrates good error resilience.
147-179: Excellent coverage of session restoration scenarios.The tests comprehensively cover:
- Symbol and string key variants (important for Rails session compatibility)
- Fallback to token parsing when persistence is missing from session data
- All restoration paths through
from_session
181-200: LGTM! Integration test validates end-to-end flow.This test correctly verifies that KSP configuration is automatically applied during client creation, validating the integration between
KindeSdk.client,TokenManager.apply_session_persistence, and the token store.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/kinde_sdk/token_manager.rb (1)
87-96: Consider logging when session doesn't support options.The method silently returns when
session.respond_to?(:options)is false. While this defensive check is appropriate, consider logging a warning to help diagnose misconfiguration during development.Apply this diff to add optional logging:
def apply_session_persistence(persistent, session) + unless session.respond_to?(:options) && session.options.is_a?(Hash) + Rails.logger&.debug("Session object does not support options configuration") + return + end - return unless session.respond_to?(:options) && session.options.is_a?(Hash) - session.options[:expire_after] = persistent ? TokenStore::TWENTY_NINE_DAYS_SECONDS : nil rescue StandardError => e Rails.logger&.warn("KSP session configuration failed: #{e.message}") end
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/kinde_sdk.rb(3 hunks)lib/kinde_sdk/token_manager.rb(1 hunks)lib/kinde_sdk/token_store.rb(1 hunks)spec/examples.txt(1 hunks)spec/kinde_sdk_spec.rb(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
lib/kinde_sdk/token_store.rb (2)
lib/kinde_sdk/token_manager.rb (3)
attr_reader(2-98)initialize(5-7)set_tokens(9-14)lib/kinde_sdk/client.rb (1)
initialize(31-39)
spec/kinde_sdk_spec.rb (2)
lib/kinde_sdk.rb (2)
refresh_token(181-193)client(120-135)lib/kinde_sdk/current.rb (1)
clear_session(11-14)
lib/kinde_sdk.rb (1)
lib/kinde_sdk/configuration.rb (1)
default(41-43)
🔇 Additional comments (16)
lib/kinde_sdk/token_manager.rb (1)
82-85: LGTM!The integration of
apply_session_persistenceintosync_sessioncorrectly applies KSP settings after syncing token data to the session.spec/kinde_sdk_spec.rb (4)
274-308: LGTM!The test correctly verifies that client creation applies session persistence when a persistent token is present. The use of
sendto invoke the private method is acceptable in specs to verify integration behavior.
310-330: LGTM!The test correctly verifies that client creation applies session persistence with
falsewhen a non-persistent token is present.
332-343: LGTM!The test correctly verifies that session persistence is skipped when no token store is present, exercising the guard condition in the
clientmethod.
345-359: LGTM!The test correctly verifies that session persistence is skipped when no session is present, exercising the second guard condition in the
clientmethod.lib/kinde_sdk.rb (3)
23-24: LGTM! Resolves NoMethodError issue.Initializing
@configtoConfiguration.defaultat module load preventsNoMethodErrorwhenclientis called beforeconfigure. This addresses the pipeline failure from previous reviews.
120-134: LGTM! Past review concerns addressed.The safe config fallback (Lines 121-124) prevents
NoMethodError, and the automatic KSP application (Lines 129-132) is now covered by tests inspec/kinde_sdk_spec.rb(Lines 274-359).
225-231: LGTM! Past review concern resolved.The explicit early return on Line 229 makes the default behavior clear: session is persistent when no token store exists (secure default), otherwise it reflects
Current.token_store.persistent. This addresses the previous concern about|| truealways returning true.lib/kinde_sdk/token_store.rb (8)
3-5: LGTM!The 29-day constant is correctly calculated (29 × 86400 = 2,505,600 seconds) and appropriately scoped to the
TokenStoreclass.
6-6: LGTM!Adding
:persistenttoattr_readercorrectly exposes the persistence state for use byTokenManagerandKindeSdk.session_persistent?.
9-9: LGTM!Defaulting to persistent sessions aligns with the secure default strategy mentioned in the PR description.
17-22: LGTM!The conditional extraction of KSP persistence from the bearer token correctly updates the
@persistentflag duringset_tokens.
30-31: LGTM!Including
persistentin the session hash enables persistence state to be restored across requests.
37-43: LGTM!The restoration logic correctly prioritizes persisted session state over re-extracting from the token, and defensively checks for both symbol and string keys.
45-50: Public utility method for cookie management.The
cookie_expirationmethod provides a clear API for determining session cookie expiration. While not currently used within the codebase, it's a valid public interface for applications to configure cookie persistence.
52-69: LGTM! Secure fallback strategy.The KSP extraction correctly defaults to persistent sessions in all error and missing-claim cases, implementing a secure-by-default strategy. The logic
ksp_claim.fetch('persistence', true) != falseensures only an explicitfalsevalue makes sessions non-persistent.
1 participant
Add support for configuring session cookie expiration based on access token claims. Sessions can now be configured as either persistent (29-day expiration) or session-only (browser close).
The implementation respects the ksp.persistence claim in access tokens, allowing dynamic session duration control per user session.