Fixing #2233 by renewing Session time also in SessionFinder access.

[precoder]

Renewing Session time also in SessionFinder access and adding explicit exception into RefreshFlow for invalid tokens instead of NPE.
Now RefreshFlow can renew the Session to prevent log off after initial Session time.

Summary by CodeRabbit

Release Notes

• Bug Fixes
  • Session access timestamps are now consistently updated when authentication codes, tokens, and refresh tokens are retrieved, improving the accuracy of session tracking and activity monitoring.
  • Invalid or unknown refresh token requests now return descriptive error responses, providing better feedback for authentication failures.

[coderabbitai]

Walkthrough

Enhanced OAuth2 session management by updating SessionFinder retrieval methods to call session.touch() when returning sessions, ensuring access timestamps are maintained. Added null-check validation in RefreshTokenFlow to handle invalid refresh tokens with proper error responses.

Changes

Cohort / File(s)	Summary
Session Retrieval Enhancement core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/SessionFinder.java	Modified session retrieval methods to invoke session.touch() on non-null sessions, updating access timestamps for authorization codes, access tokens, and refresh tokens.
Refresh Token Validation core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/tokenrequest/RefreshTokenFlow.java	Added null-check for session retrieved via getSessionForRefreshToken(). Returns JSON error response with "invalid_request" code when session is null, preventing further processing of invalid refresh tokens.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Client
    participant RTA as RefreshTokenFlow
    participant SF as SessionFinder
    participant Session as Session

    Note over Client,Session: Previous Flow (without validation)
    Client->>RTA: POST refresh_token
    RTA->>SF: getSessionForRefreshToken()
    SF->>Session: retrieve session
    Session-->>SF: returns session or null
    SF-->>RTA: session (possibly null)
    RTA->>RTA: Enter synchronized block<br/>(potential null error)

    Note over Client,Session: New Flow (with validation)
    Client->>RTA: POST refresh_token
    RTA->>SF: getSessionForRefreshToken()
    SF->>Session: retrieve session
    Session-->>SF: returns session or null
    SF->>Session: session.touch()
    Session-->>SF: ✓ updated timestamp
    SF-->>RTA: session
    rect rgb(200, 220, 255)
        RTA->>RTA: Check if session is null
        alt session is null
            RTA-->>Client: error: invalid_request (no sync block)
        else session is valid
            RTA->>RTA: Enter synchronized block<br/>(safe processing)
        end
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Changes involve straightforward null-check addition and session lifecycle method calls across multiple retrieval methods, but require verification of consistent touch() invocation patterns and validation of error handling semantics in the refresh token flow.

Possibly related issues

• SessionFinder access does not set lastUse time of the Sessions. #2233: Directly addresses the core issue of SessionFinder not updating session access timestamps via the touch() mechanism introduced in these changes.

Possibly related PRs

• Fix #2054 SessionFinder memory leak by removing already time-outed session references during the cleanup. #2055: Related modification to SessionFinder for session lifecycle handling with cleanupSessions and SessionManager integration.

Suggested reviewers

• rrayst

Poem

🐰 Hops with glee o'er tokens fresh,
Sessions touched, timestamps mesh,
Null checks guard the flow so clean,
Refresh tokens living—keen! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title "Fixing #2233 by renewing Session time also in SessionFinder access" is partially related to the changeset. It clearly and specifically describes the primary change in SessionFinder.java—calling session.touch() to renew access timestamps for codes, tokens, and refresh tokens. However, the title does not mention the RefreshTokenFlow.java changes, which include adding a null check for invalid refresh tokens and returning an explicit error response instead of allowing an NPE. Both changes are presented in the PR objectives as part of the fix for issue #2233, but the title only captures the SessionFinder aspect.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 23d66ee and 0cd44c7.

📒 Files selected for processing (2)

• core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/SessionFinder.java (1 hunks)
• core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/tokenrequest/RefreshTokenFlow.java (1 hunks)

🔇 Additional comments (4)

core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/SessionFinder.java (3)

63-71: LGTM: Session timestamp properly updated on code retrieval.

The addition of session.touch() ensures the session's access timestamp is updated when retrieving by authorization code, preventing premature session expiration. The call is correctly placed inside the synchronized block.

---

73-81: LGTM: Session timestamp properly updated on token retrieval.

The addition of session.touch() ensures the session's access timestamp is updated when retrieving by access token. The implementation correctly mirrors the pattern in getSessionForCode.

---

83-91: LGTM: Session timestamp properly updated on refresh token retrieval.

The addition of session.touch() ensures the session's access timestamp is updated when retrieving by refresh token. This is particularly important for the RefreshTokenFlow to prevent session expiration during token refresh operations.

core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/tokenrequest/RefreshTokenFlow.java (1)

91-95: Critical NPE fix, but consider using "invalid_grant" error code.

The null check correctly prevents a NullPointerException when getSessionForRefreshToken returns null. However, per OAuth 2.0 RFC 6749 section 5.2, the error code "invalid_grant" is more appropriate for invalid, expired, or unknown refresh tokens, whereas "invalid_request" is intended for malformed requests or missing parameters.

Consider this change for better OAuth2 spec compliance:

 if(session == null) {
     // client sends unknown refresh token
-    return OAuth2Util.createParameterizedJsonErrorResponse(exc, jsonGen,"error", "invalid_request");
+    return OAuth2Util.createParameterizedJsonErrorResponse(exc, jsonGen,"error", "invalid_grant");
 }

Note: Line 58 uses "invalid_request" for token processing failures, which may warrant similar consideration for consistency across the codebase.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[membrane-ci-server]

This pull request needs "/ok-to-test" from an authorized committer.

[predic8]

@coderabbitai plz review

[predic8]

/ok-to-test

[coderabbitai]

@predic8: I'll review the changes in this pull request now.

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[predic8]

@rrayst should we also merge this into main branch?