Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use existing session id for fetching flows as to not get a new session #1403

Merged
merged 3 commits into from
Jun 12, 2020

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Jun 12, 2020

Fixes element-hq/element-web#13990

Regression caused by bebeec7

@t3chguy t3chguy requested a review from a team June 12, 2020 18:51
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

hmm, I'm fairly sure we're supposed to know the flows by here. Would be good to test with a homeserver like conduit as Synapse doesn't validate the session correctly.

If it works fine though, I guess it's probably okay.

@clokep
Copy link
Member

clokep commented Jun 12, 2020

hmm, I'm fairly sure we're supposed to know the flows by here. Would be good to test with a homeserver like conduit as Synapse doesn't validate the session correctly.

Would love to have more info on what the issues are with how Synapse validates the session! We've put a bunch of work into this recently, so if there are other issues it'd be nice to fix it while the info is still in someone's head!

@t3chguy
Copy link
Member Author

t3chguy commented Jun 12, 2020

hmm, I'm fairly sure we're supposed to know the flows by here

Not if we come in from an email verification apparently as we get the session id from the URL and <InteractiveAuth calls this method after passing that ID to the ctor but it gets thrown away by us using null

@turt2live
Copy link
Member

Would love to have more info on what the issues are with how Synapse validates the session! We've put a bunch of work into this recently, so if there are other issues it'd be nice to fix it while the info is still in someone's head!

@clokep I believe it came down to spec compliance last I looked: Synapse happily accepted {} as an auth dict even when it's not supposed to, and likewise somehow manages to track different session IDs across the same request for the same user (as under some conditions riot-web loses its session ID and just gets a new one in the middle of the flow).

@clokep
Copy link
Member

clokep commented Jun 12, 2020

likewise somehow manages to track different session IDs across the same request for the same user

It definitely doesn't do this. There are some hacks added though for updating sessions and such through the UI Auth flow, which are all...not really speced. :(

I can believe the other bits though.

@t3chguy
Copy link
Member Author

t3chguy commented Jun 12, 2020

Registering on a Conduit worked but then again it didn't have many flows (no email or consent) so maybe coincidental.

@t3chguy t3chguy merged commit 5059155 into develop Jun 12, 2020
@t3chguy t3chguy deleted the t3chguy/attemptAuth-existing-session branch March 1, 2021 10:23
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.

Registration failure: M_MISSING_PARAM password_hash
3 participants