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

[rrayst]

…dding explicit exception into RefreshFlow for invalid tokens instead of NPE. (#2234)

Summary by CodeRabbit

• Bug Fixes

  • Refresh token requests now fail fast for unknown sessions, returning an invalid_grant error to prevent errors during token refresh.

• Chores

  • Session retrieval now updates session activity on access to keep session state current and improve lifecycle reliability during OAuth2 flows.

[coderabbitai]

Walkthrough

Three session getter methods in SessionFinder now call session.touch() before returning sessions to update their activity timestamp. RefreshTokenFlow adds a null-check for the refresh-token session and returns an invalid_grant error if the session is absent before entering the synchronized block.

Changes

Cohort / File(s)	Summary
Session lifecycle management core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/SessionFinder.java	Modified getSessionForCode(), getSessionForToken(), and getSessionForRefreshToken() to invoke session.touch() on retrieved sessions before returning them.
Refresh token flow safety core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/tokenrequest/RefreshTokenFlow.java	Added null-check for session obtained via getSessionForRefreshToken() in processWithParameters() and return invalid_grant when session is null before entering synchronized(session) to avoid NPEs.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant TokenEndpoint
    participant RefreshFlow
    participant SessionFinder
    participant Session

    Client->>TokenEndpoint: POST /token (grant=refresh_token)
    TokenEndpoint->>RefreshFlow: processWithParameters(...)
    RefreshFlow->>SessionFinder: getSessionForRefreshToken(refreshToken)
    alt session found
        SessionFinder->>Session: retrieve & touch()    %% touch on access
        SessionFinder-->>RefreshFlow: session
        RefreshFlow->>RefreshFlow: synchronized(session) { validate & issue token }
        RefreshFlow-->>TokenEndpoint: success response
    else session not found
        SessionFinder-->>RefreshFlow: null
        RefreshFlow-->>TokenEndpoint: return invalid_grant (fail fast)
    end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Extra attention: Session.touch() side-effects (thread-safety, metrics) and the early-return behavior in RefreshTokenFlow.processWithParameters() to confirm callers expect an invalid_grant response for unknown refresh tokens.

Possibly related issues

• SessionFinder access does not set lastUse time of the Sessions. #2233 — Matches changes that ensure session getters update last-use and adjusts refresh-token handling for missing sessions.

Possibly related PRs

• Fixing #2233 by renewing Session time also in SessionFinder access.  #2234 — Edits the same SessionFinder getters and adds the refresh-token null-check; likely the same or companion change.
• Fix #2054 SessionFinder memory leak by removing already time-outed session references during the cleanup. #2055 — Related work on SessionFinder (cleanup and wiring into SessionManager) that touches the same component and lifecycle behavior.

Suggested reviewers

• predic8

Poem

🐇 I nudge the session, give it a touch,

So tokens know they've mattered much.
If a refresh is lost in the wood,
I halt the hop — that's proper and good. 🥕

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 pull request title "Fixing #2233 by renewing Session time also in SessionFinder access. A…" is clearly and specifically related to the main change in the changeset. The title accurately describes the primary modification: adding session.touch() calls to three getter methods in SessionFinder to renew session timeout on access, addressing issue #2233. The title is concise, specific enough that a teammate scanning history would understand the primary change, and directly references the issue being fixed. While the changeset also includes a secondary modification to RefreshTokenFlow (adding null check handling), the title appropriately focuses on the primary change, which aligns with the expectation that titles need not cover every detail.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch precoder-issue-2233-session-finder-renews-delivered-session-timeout-master

---

📜 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 795aa06 and e302fcc.

📒 Files selected for processing (1)

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

🚧 Files skipped from review as they are similar to previous changes (1)

• core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/request/tokenrequest/RefreshTokenFlow.java

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Analyze (java)

---

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.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b014256 and 19146c3.

📒 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 context used

🧬 Code graph analysis (1)

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

core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/OAuth2Util.java (1)

• OAuth2Util (30-89)

🔇 Additional comments (1)

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

63-91: LGTM! Session activity tracking implemented consistently.

The addition of session.touch() calls in all three getter methods (getSessionForCode, getSessionForToken, getSessionForRefreshToken) correctly updates session activity on access, which aligns with the PR objective of renewing session timeouts when sessions are retrieved. The pattern is consistent across all three methods and properly synchronized.

[rrayst]

/ok-to-test

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 19146c3 and 795aa06.

📒 Files selected for processing (1)

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

🧰 Additional context used

🧬 Code graph analysis (1)

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

core/src/main/java/com/predic8/membrane/core/interceptor/oauth2/OAuth2Util.java (1)

• OAuth2Util (30-89)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: Automated tests
• GitHub Check: Analyze (java)

[membrane-ci-server]

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