Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Enable login_via_existing_session by default #15719

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

hughns
Copy link
Member

@hughns hughns commented Jun 5, 2023

Following discussion in #15388 (review) this PR enables the login_via_existing_session capability from MSC3882 by default.

As UIA is required by default and there is a rate limit of 1 request per minute on the new endpoint, I believe this qualifies as "secure by default".

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

Not sure if this PR wanted review; it's a draft but it's also in the queue.

@@ -0,0 +1 @@
Enabled login_via_existing_session by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably promote this to feature and write out in words what this allows, so that it's more noticeable (I expect some admins may want to turn this off, so best not hide it in misc which should not generally contain user-noticeable changes).

Comment on lines +2604 to +2605
To protect against malicious clients abusing this capability, user-interactive authentication
is required unless the `require_ui_auth` sub-option is set to `False`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be missing rationale for why this feature would be useful if you have to authenticate anyway — why don't you 'just authenticate on the device where you're logging in'?

I can imagine some reasons but it might be nice to give a brief summary here.

@clokep
Copy link
Contributor

clokep commented Aug 24, 2023

@hughns Should we put this into the review queue?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants