Skip to content

Conversation

AndrewFerr
Copy link
Member

No description provided.

@AndrewFerr AndrewFerr requested a review from sandhose July 14, 2025 06:41
Copy link

cloudflare-workers-and-pages bot commented Jul 14, 2025

Deploying matrix-authentication-service-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6183cae
Status: ✅  Deploy successful!
Preview URL: https://60a93d42.matrix-authentication-service-docs.pages.dev
Branch Preview URL: https://af-user-locked-errors.matrix-authentication-service-docs.pages.dev

View logs

Copy link
Member

@sandhose sandhose left a comment

Choose a reason for hiding this comment

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

Nothing major, thanks for this!

browser_session.id = %browser_session_id,
"Attempt to exchange login token but browser session is not active"
);
return Err(RouteError::InvalidLoginToken);
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be more readable to check if the user is locked earlier

if browser_session.user.locked_at.is_some() {
  return Err(RouteError::UserLocked);
}

if !browser_session.active() || !browser_session.user.is_valid() {
  // ...
}

It changes a little bit what error we show in what condition, but it's an edge-case anyway, as usually we would show a user comprehensible error earlier in the UI

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought, I've reverted this with 05827d1 (the commit message explains why). The logic is more precise this time, though.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, works for me!

@AndrewFerr AndrewFerr force-pushed the af/user-locked-errors branch from d31af23 to 82e3ea6 Compare July 15, 2025 15:19
When a user is both locked and deactivated, give precedence to
deactivation errors over locked errors, as a locked error suggests that
unlocking the user would make it available.
@AndrewFerr AndrewFerr requested a review from sandhose July 16, 2025 18:18
@sandhose sandhose merged commit b8480b1 into main Jul 17, 2025
20 checks passed
@sandhose sandhose deleted the af/user-locked-errors branch July 17, 2025 07:17
@sandhose sandhose added A-Compatibility-Layer Related to the legacy Matrix authentication compatibility layer T-Enhancement New feature of request labels Jul 17, 2025
@sandhose sandhose changed the title Support M_USER_LOCKED error for compat sessions Return M_USER_LOCKED error when logging in with a locked account on the compatibility endpoints Jul 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Compatibility-Layer Related to the legacy Matrix authentication compatibility layer T-Enhancement New feature of request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants