Skip to content

AUT-1462 Add logging to grant-attacher session expiry handling#23

Merged
DmitryMasley merged 2 commits into
masterfrom
AUT-1462-logs
Jul 3, 2026
Merged

AUT-1462 Add logging to grant-attacher session expiry handling#23
DmitryMasley merged 2 commits into
masterfrom
AUT-1462-logs

Conversation

@DmitryMasley

@DmitryMasley DmitryMasley commented Jul 2, 2026

Copy link
Copy Markdown

Summary

  • console.error('Failed to get grant', err) now fires at catch-entry for all errors (including SessionExpiredError), giving a single unified log point for any getGrant failure.
  • console.log('Logged out due to session near maximum lifespan') fires after a successful back-channel logout, confirming the KC session was invalidated.
  • Reformatted .then(next).catch(...) chain to separate lines for readability.
  • Added ESLint indent: ["error", 2] rule to eslint.config.mjs.
  • Extended buildRequest test helper with an originalUrl parameter for future test coverage.

Test plan

  • All unit tests pass: node test/unit/grant-attacher-unit-test.js (20/20)
  • Verify console.error appears in logs when a token refresh fails in staging
  • Verify console.log appears after a session-max-lifespan logout in staging

Relates to AUT-1462

🤖 Generated with Claude Code

- console.error for all getGrant failures (unified at catch entry)
- console.log on successful back-channel logout (session max lifespan)
- Reformat .then(next).catch to separate lines for readability
- Add ESLint indent rule (2 spaces)
- Add buildRequest originalUrl param to test helpers for future use

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
});
});

test('grant-attacher: SessionExpiredError redirect preserves the original deep-linked URL', t => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unhappy that this PR removes the test. For any change, I expect us to update existing tests or, better yet, add new regression tests (which may overlap).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is, redirect no longer happens. For the async request, a different test was added in the previous PR.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread middleware/grant-attacher.js Outdated
Comment on lines 49 to 50
next();
return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we can simplify and remove these two lines because we have the next() right after this block.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reported URL carries error=temporarily_unavailable&error_description=authentication_expired, which is a different, well-documented Keycloak error class: it means Keycloak's own server-side authentication session (the login-flow session tied to the AUTH_SESSION_ID/KC_RESTART cookie) was not found or had expired when the login form was submitted - e.g. the user idled past the auth-session timeout, or a concurrent login in another tab restarted the shared root session. That's not a redirect_uri mismatch, and I found nothing in this codebase or the wider search that generates or handles temporarily_unavailable/authentication_expired - it comes straight from Keycloak.

So: this fix is adjacent (same general area - multi-tab login concurrency) but doesn't address this specific bug report. Stripping stale query params prevents redirect_uri corruption; it does nothing about Keycloak invalidating its own auth session.

…xt()

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@DmitryMasley DmitryMasley merged commit aab4e46 into master Jul 3, 2026
@DmitryMasley DmitryMasley deleted the AUT-1462-logs branch July 3, 2026 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants