-
Notifications
You must be signed in to change notification settings - Fork 379
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
MSC1960: OpenID information exchange with widgets #1960
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good 👍
@turt2live and I had a discussion about this a bit on Matrix, something somewhere needs to spec out a bit more how the client gets the OpenID credentials to pass to the widget. It was suggested something like https://github.com/matrix-org/matrix-doc/pull/1961/files#diff-46763da5dcada9c6bd410e646df8390bR21 could be added. I also wonder if this should be generalized a bit to broader SSO access? I'm not sure how hard that would be, but the current way that clients can login is a generic "SSO" type, not OpenID specifically. |
Co-authored-by: David Baker <[email protected]> Co-authored-by: Andrew Morgan <[email protected]>
@dbkr @anoadragon453 I've ruined your comment threads by making significant formatting changes to the diff, sorry. There's also some clarifications if @richvdh, @clokep, and @jaywink have a chance to review them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all sounds sane to me.
@mscbot fcp merge |
Team member @turt2live has proposed to merge this. The next step is review by the rest of the tagged people: Concerns:
Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for information about what commands tagged team members can give me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor clarifications, but overall looks good!
right, so enough of the widget spec has been specified for this to get the clarifications it needed. Originally the Functionally the MSC is the same with this new wording, though most of it has been replaced with clearer (hopefully) words. @richvdh @dbkr @anoadragon453 please take another read, which may involve skimming parts of the widget spec (see diff for links). For now, I think my concern can be lifted: @mscbot resolve This is blocked on untangling the mess between MSC1236 and reality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sans some small details, I think I'm pretty happy with this as it stands.
|
||
Widgets are currently left with no options to verify the user's ID, making it hard for | ||
personalized and authenticated widgets to exist. The spec says the `$matrix_user_id` | ||
template variable cannot be relied upon due to how easy it is to faslify, which is true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
template variable cannot be relied upon due to how easy it is to faslify, which is true. | |
template variable cannot be relied upon due to how easy it is to falsify, which is true. |
"widgetId": "CCCDDD", | ||
"data": { | ||
"state": "allowed", | ||
"original_request_id": "AAABBB", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mix of snake & camel case is unfortunate but this is probably the least terrible way of combining the two worlds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, it's a mess. It is however consistent with the other API actions (which are also awful).
The request SHOULD result in the user being prompted to confirm that the widget can have | ||
their information. Because of this user interaction, it's not always possible for the user | ||
to complete the approval within the 10 second suggested timeout by the widget spec. As | ||
such, the initial request by the widget can have one of three states: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a bit out-of-order that we're talking about the responses before we've seen what the request is.
``` | ||
|
||
Which then receives a response which has a `state` field alongside potentially the credentials | ||
to be verified. Matching the order of possible responses above, here are examples: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably have just talked about what responses at the same time as the examples rather than having the reader refer back up to the list of possible responses, but ok - the content is fine, probably not worth re-wording.
🔔 This is now entering its final comment period, as per the review above. 🔔 |
The final comment period, with a disposition to merge, as per the review above, is now complete. |
In the case of `state: "request"`, the user is being asked to approve the widget's attempt to | ||
verify their identity. To ensure that future requests are quicker, clients are encouraged to | ||
include a "remember this widget" option to make use of the immediate `state: "allowed"` or | ||
`state: "blocked"` responses above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be redundant to what we have with the capability system. I would propose making this a capability as well. This would
- give the widget developer the choice to ask the user for oidc permissions on first startup.
- the client developer does not need to track oidc permissions + capabilitis but can treat oidc as a capability without any custom logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thinking further capabilites imply a rethinking of this:
to conform to the rest of the widget logic It seems It should be like this:
- there should only be on action:
fromWidget
get_openId
. - there should be a capability
allow_open_id
- it is the widgets responsibility to first aquire the capability. (the client is responsible to prompt the user with a clear explanation) -> then the token request via
get_openid
is instant and does not need two actions: one for asking for the tokenget_openid
(which might already return a token but not necassarly) and another one for sending over the tokenopenid_credentials
.
In other words get_oidc
seems to be equivalent to a request capabilites call with a confusing name.
Rendered
Upstream proposal: MSC1956 - Integrations API
Discussion room: #msc-integrations-api:t2bot.io
This is being contributed under the hat of "author of Dimension":