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

synapse requires unspecified user for m.login.password UI auth #5665

Closed
Tracked by #3506
richvdh opened this issue Jul 11, 2019 · 15 comments · Fixed by #8848
Closed
Tracked by #3506

synapse requires unspecified user for m.login.password UI auth #5665

richvdh opened this issue Jul 11, 2019 · 15 comments · Fixed by #8848
Assignees
@anoadragon453
Labels
A-Spec-Compliance places where synapse does not conform to the spec z-bug (Deprecated Label)

Comments

@richvdh
Copy link
Member

richvdh commented Jul 11, 2019 •
edited
Loading

The spec says that the client should submit a dict including an identifier object. In practice, synapse requires you to instead to just submit a user field, so the dict looks like:

{
  "type": "m.login.password",
  "user": "<user_id or user localpart>",
  "password": "<password>",
  "session": "<session ID>"
}
@richvdh richvdh added the A-Spec-Compliance places where synapse does not conform to the spec label Jul 11, 2019
@richvdh richvdh mentioned this issue Jul 11, 2019
Closed
@richvdh
Copy link
Member Author

richvdh commented Jul 11, 2019

Note that riot-web currently relies on the broken impl working (element-hq/element-web#10312)

@turt2live turt2live mentioned this issue Jul 11, 2019
Closed
14 tasks
@turt2live
Copy link
Member

Also applies to threepidCreds on MSISDN (see matrix-org/matrix-react-sdk#3211)

@richvdh
Copy link
Member Author

richvdh commented Aug 28, 2019

Note that riot-web currently relies on the broken impl working (element-hq/element-web#10312)

This is no longer true: riot-web submits both formats, and should continue to work when we fix this.

@bmarty
Copy link

bmarty commented Nov 21, 2019

FTR RiotX uses the identifier object

@anoadragon453 anoadragon453 self-assigned this Nov 21, 2019
@dbkr
Copy link
Member

dbkr commented Jan 21, 2020

It's obviously easy to do so, but I'm having to work around this in riot web now.

@richvdh richvdh added the z-bug (Deprecated Label) label Feb 5, 2020
@clokep clokep mentioned this issue Apr 16, 2020
Merged
@richvdh richvdh mentioned this issue May 4, 2020
Closed
@jcgruenhage
Copy link
Contributor

So, riot-web has been sending both user and identifier for a while. Can the user parameter be removed as a requirement and just be optional, so that clients correctly implementing the spec can work with synapse?

@jcgruenhage
Copy link
Contributor

(Making this optional will not break clients)

@jcgruenhage
Copy link
Contributor

Also, riot-web is still broken with password changes: https://github.com/matrix-org/matrix-react-sdk/blob/0fcbe40becf8f2c3d0bc684728cf1f4646986e44/src/components/views/settings/ChangePassword.js#L144

@anoadragon453 anoadragon453 mentioned this issue May 6, 2020
Closed
This was referenced May 28, 2020
Closed
Merged
@babolivier babolivier linked a pull request Jun 3, 2020 that will close this issue
Allow for identifier dicts in User Interactive Auth dicts #7438
Closed
anoadragon453 added a commit that referenced this issue Jun 5, 2020
@anoadragon453
Performance improvements and refactor of Ratelimiter (#7595)
f4e6495 
While working on #5665 I found myself digging into the `Ratelimiter` class and seeing that it was both:

* Rather undocumented, and
* causing a *lot* of config checks

This PR attempts to refactor and comment the `Ratelimiter` class, as well as encourage config file accesses to only be done at instantiation. 

Best to be reviewed commit-by-commit.
phil-flex pushed a commit to phil-flex/synapse that referenced this issue Jun 16, 2020
@anoadragon453 @phil-flex
Performance improvements and refactor of Ratelimiter (matrix-org#7595)
be735d7 
While working on matrix-org#5665 I found myself digging into the `Ratelimiter` class and seeing that it was both:

* Rather undocumented, and
* causing a *lot* of config checks

This PR attempts to refactor and comment the `Ratelimiter` class, as well as encourage config file accesses to only be done at instantiation. 

Best to be reviewed commit-by-commit.
@anoadragon453
Copy link
Member

#8182 is one part of this and is currently in review.

@clokep clokep mentioned this issue Oct 15, 2020
Closed
@richvdh richvdh changed the title synapse implements m.login.password UI auth incorrectly synapse requires unspecified user for m.login.password UI auth Nov 27, 2020
@richvdh
Copy link
Member Author

richvdh commented Nov 27, 2020

Also applies to threepidCreds on MSISDN (see matrix-org/matrix-react-sdk#3211)

@turt2live: please can you help me out here: I can't figure out which endpoint is being called with what parameters, and what the correct behaviour is. (it sounds like a separate problem: better to open a separate issue?)

@richvdh richvdh mentioned this issue Nov 27, 2020
Closed
@turt2live
Copy link
Member

Between that PR and the others (matrix-org/matrix-react-sdk#4667 and company) there's a fairly wide range of endpoints. The threepidCreds thing is most apparent on password resets or anything having UIA and email addresses as a flow. The user thing is most obvious on account deactivation, but can also be a case for changing passwords in the app (not reset, changing) as well.

@turt2live
Copy link
Member

The undocumented user object is more or less at the wrong place compared to the spec, so Synapse just needs to change its lookup. The threepidCreds problem is similar in that it's just spelled wrong between the spec and synapse's expectations.

@richvdh
Copy link
Member Author

richvdh commented Nov 27, 2020 •
edited
Loading

@turt2live: ok, to save readers of this issue (including myself) trying to reverse-engineer the problem from react-sdk PRs, please can you try to give some specific examples. The spec says threepidCreds in some places and three_pid_creds in others. What is synapse expecting, and what should it be expecting?

Again I think this is orthogonal to user so I'd encourage you to open a new issue.

@turt2live
Copy link
Member

I don't have the context to be able to write up that issue anymore, sorry.

@richvdh
Copy link
Member Author

richvdh commented Nov 30, 2020 •
edited
Loading

ok, I'm going to fix the user issue and consider threepidCreds an unrelated problem. If you/anyone has more info, please open a new issue.

@richvdh richvdh mentioned this issue Nov 30, 2020
Merged
richvdh added a commit that referenced this issue Dec 1, 2020
@richvdh
Support "identifier" dicts in UIA
af7a5cc 
The spec requires synapse to support `identifier` dicts for `m.login.password`
user-interactive auth, which it did not (instead, it required an undocumented
`user` parameter.)

To fix this properly, we need to pull the code that interprets `identifier`
into `AuthHandler.validate_login` so that it can be called from the UIA code.

Fixes #5665.
richvdh added a commit that referenced this issue Dec 1, 2020
@richvdh
Support "identifier" dicts in UIA (#8848)
4d94965 
The spec requires synapse to support `identifier` dicts for `m.login.password`
user-interactive auth, which it did not (instead, it required an undocumented
`user` parameter.)

To fix this properly, we need to pull the code that interprets `identifier`
into `AuthHandler.validate_login` so that it can be called from the UIA code.

Fixes #5665.
@turt2live turt2live mentioned this issue Apr 29, 2021
Closed
@richvdh richvdh mentioned this issue Apr 16, 2020
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@anoadragon453 anoadragon453

Labels
A-Spec-Compliance places where synapse does not conform to the spec z-bug (Deprecated Label)
Projects
None yet
Milestone
No milestone
6 participants
@dbkr @turt2live @anoadragon453 @richvdh @bmarty @jcgruenhage