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

Support UI Authentication for OpenID Connect accounts - #7457

Merged
clokep merged 7 commits into
developfrom
clokep/oidc-ui-auth
May 15, 2020
Merged

clokep merged 7 commits into
developfrom
clokep/oidc-ui-auth

Conversation

@clokep

@clokep clokep commented May 8, 2020

Copy link
Copy Markdown
Member

This is similar to #7102 / #7186, but for OpenID (added in #7256).

It also cleans up a little bit of code to make things consistent between the various SSO workflows.

CC @sandhose

@clokep
clokep requested a review from a team May 8, 2020 14:13
@clokep
clokep marked this pull request as ready for review May 8, 2020 14:13

@anoadragon453 anoadragon453 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks pretty sensible, but I'm afraid I'm missing a lot of context around OpenID in order to review this, which makes up the majority of this PR. Ideally @sandhose could take a look at those bits?

@anoadragon453
anoadragon453 requested a review from a team May 13, 2020 16:07
@sandhose

Copy link
Copy Markdown
Member

LGTM. Nothing really changed in the OIDC flow, just that the session cookie has one more key in it (and that part is covered enough by unit tests)

@clokep

clokep commented May 13, 2020

Copy link
Copy Markdown
Member Author

@sandhose Do you think the macaroon changes are reasonable? Sometimes having a particular key there and sometimes now. I was unsure if this is "valid", but it seems to pass verification OK still.

@sandhose

Copy link
Copy Markdown
Member

@sandhose Do you think the macaroon changes are reasonable? Sometimes having a particular key there and sometimes now. I was unsure if this is "valid", but it seems to pass verification OK still.

Wasn't sure either but I tested locally and it works. Seems like caveat verification in macaroons means that each first party caveat in the token must satisfy at least one verifier. It does not mean that each verifier must be used.

@richvdh richvdh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm otherwise

Comment thread synapse/handlers/oidc_handler.py Resolved
@clokep
clokep merged commit a3cf36f into develop May 15, 2020
@clokep
clokep deleted the clokep/oidc-ui-auth branch May 15, 2020 16:26
phil-flex pushed a commit to phil-flex/synapse that referenced this pull request Jun 16, 2020
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.

5 participants