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

Clarifications to SSO login/UIA #2608

Merged
merged 3 commits into from Jun 5, 2020
Merged

Clarifications to SSO login/UIA #2608

merged 3 commits into from Jun 5, 2020

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Jun 4, 2020

including a bunch of text about security.

including a bunch of text about security
@richvdh richvdh requested a review from a team June 4, 2020 21:54
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.

Generally lgtm - just some questions and minor clarifications

specification/client_server_api.rst Show resolved Hide resolved

An overview of the process is as follows:

0. The Matrix client calls |GET /login|_ to find the supported login
Copy link
Member

Choose a reason for hiding this comment

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

Please can we start numbered lists at one?

Copy link
Member Author

Choose a reason for hiding this comment

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

we can, though in this case step zero felt very much like a preliminary step.

Copy link
Member

Choose a reason for hiding this comment

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

It's just a bit awkward to have things start at zero, as it implies that it's not a real step (at least imo).

Copy link
Member Author

Choose a reason for hiding this comment

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

that implication is correct, imo :)

specification/modules/sso_login.rst Show resolved Hide resolved
specification/modules/sso_login.rst Outdated Show resolved Hide resolved
specification/modules/sso_login.rst Outdated Show resolved Hide resolved
specification/modules/sso_login.rst Show resolved Hide resolved
specification/modules/sso_login.rst Outdated Show resolved Hide resolved
<../index.html#mapping-from-other-character-sets>`_ may be useful.
This confirmation could take place before redirecting to the SSO
authentication page (when handling the
``/_matrix/client/%CLIENT_MAJOR_VERSION%/auth/m.login.sso/fallback/web``
Copy link
Member

Choose a reason for hiding this comment

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

fwiw we have a history of shortening URLs to after the version (ie: /auth/m.login.sso/fallback/web), largely for legibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

mmhmm. There was some inconsistency here and I went this way: do you think I should go the other?

Copy link
Member

Choose a reason for hiding this comment

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

I don't personally have a preference, just mentioning it in the event it felt annoying to have to do it every time.

Co-authored-by: Travis Ralston <travpc@gmail.com>
@richvdh richvdh requested a review from turt2live June 5, 2020 15:56
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.

Thanks!

@richvdh richvdh merged commit da740bf into master Jun 5, 2020
Comment on lines +62 to +64
1. To initiate the ``m.login.sso`` login type, the Matrix client instructs the
user's browser to navigate to the |/login/sso/redirect|_ endpoint on the
user's homeserver.
Copy link
Member

Choose a reason for hiding this comment

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

I know it's taken from the current text but it's a bit too specific, in my opinion. It's not really necessary to assume that SSO requires a web view which is generally, but not necessarily the case. The flow between (1) and (4) only requires HTTP(S). (3) doesn't need even that: interaction with the Auth server can be driven by API (voice authentication through a SIP call; local agent providing SSO; you name it). The wording here also implies that the user's browser is somewhat separate from the client while the text later suggests using an embedded web view for native applications.

Suggested change
1. To initiate the ``m.login.sso`` login type, the Matrix client instructs the
user's browser to navigate to the |/login/sso/redirect|_ endpoint on the
user's homeserver.
1. To initiate the ``m.login.sso`` login type, the Matrix client dispatches
a call to the |/login/sso/redirect|_ endpoint on the user's homeserver.
Depending on the client architecture, the call and further flow can
either be performed by the client itself or the client may use a suitable
embedded view or external application (e.g. a web browser if the flow is
web-based). This client-side party of the authentication process (the Matrix
client itself or the view/application) is further referred to as
"authenticating agent".

Further proposed changes below.


1. To initiate the ``m.login.sso`` login type, the Matrix client instructs the
user's browser to navigate to the |/login/sso/redirect|_ endpoint on the
user's homeserver.

2. The homeserver responds with an HTTP redirect to the SSO user interface,
which the browser follows.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
which the browser follows.
which the authenticating agent follows.

Comment on lines +69 to +71
3. The authentication server and the homeserver interact to verify the user's
identity and other authentication information, potentially using a number of
redirects.
Copy link
Member

Choose a reason for hiding this comment

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

Surely there was a typo? Interaction is not with the homeserver at this stage.

Suggested change
3. The authentication server and the homeserver interact to verify the user's
identity and other authentication information, potentially using a number of
redirects.
3. The authentication server and the authenticating agent interact
to verify the user's identity and other authentication information,
potentially using a number of redirects.

Comment on lines +73 to 74
4. The browser is directed to the ``redirectUrl`` provided by the client with
a ``loginToken`` query parameter for the client to log in with.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
4. The browser is directed to the ``redirectUrl`` provided by the client with
a ``loginToken`` query parameter for the client to log in with.
4. Upon successful completion of (3) the authentication server directs
the authenticating agent to the ``redirectUrl`` provided by the client at step (1),
adding a ``loginToken`` query parameter for the client to log in with.

Comment on lines +79 to +80
For native applications, typically steps 1 to 4 are carried out by opening an
embedded web view.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For native applications, typically steps 1 to 4 are carried out by opening an
embedded web view.
For native applications and web-based authentication flows, typically
steps 1 to 4 are carried out by opening an embedded web view.


Instead, the state could be stored in `localStorage
<https://developer.mozilla.org/en-US/docs/Web/API/Window/localStorage>`_ or
in a cookie.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
in a cookie.
in a cookie; native clients should also keep state local to them.

and the REST endpoints which consume the ticket. A good reference for how CAS could
be implemented is available in the older `r0.4.0 version <https://matrix.org/docs/spec/client_server/r0.4.0.html#cas-based-client-login>`_
of this specification.
The server should handle
Copy link
Member

Choose a reason for hiding this comment

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

Just for the sake of clarity

Suggested change
The server should handle
The homeserver should handle

| | | |
| |<========(3) Authentication process================>|
| | | |
| |<--(4) redirect to redirectUrl--| |
Copy link
Member

Choose a reason for hiding this comment

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

If it's really the homeserver invoking the redirect then how does it figure out that the auth flow has been successful? The sequence has a gap between 3 and 4.

#. The homeserver should generate a short-term login token. This is an opaque
token, suitable for use with the ``m.login.token`` type of the |/login|_
API. The lifetime of this token SHOULD be limited to around five
seconds.
Copy link
Member

Choose a reason for hiding this comment

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

This seems to assume relatively quick roundtrip between the client and the homeserver?

parameters. If it already includes one or more ``loginToken`` parameters,
they should be removed before adding the new one.)

#. The homeserver redirects the user's browser to the URI thus built.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#. The homeserver redirects the user's browser to the URI thus built.
#. The homeserver redirects the client to the URI thus built.

@afranke afranke deleted the rav/clarify_openid branch September 22, 2021 10:37
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.

None yet

3 participants