-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Scalar client API to request an OpenID token (read: improve auth dance for widgets) #7153
Comments
@rxl881 I'm interested in your thoughts on this. |
Hi @turt2live,
I'm sure you are correct, but it would be useful to have a little more background / examples about exactly why this effects mobile and other (specific) riot instances. This could be useful in helping to set prioritisation for the suggested fix. Having said that, we've got zero bandwidth at the moment, so probably won't be able to spend too much time on this in the short term. I like your suggested approach to using the postMessage API and OpenID, and it would be great to see a fleshed-out spec with more detail (maybe a sequence diagram?) for how this would work in practice (especially highlighting the differences from the current flow). In particular, one thing to bear in mind is that we (integration managers in general) need to be able to map the auth tokens to the widget / integrations 'owned' by users for auditing and also access control purposes (the main reason that the auth is tightly coupled to the IM at the moment). So, it would be good to check the spec. proposal doesn't open up any potential for circumventing this. Your suggestion for the UI/UX flow for verifying identity permissions sounds great to me and appears to fit nicely with what I have been thinking of for the more general case of widgets requesting permissions; and yep the permissions probably need to be persisted (probably in user application data, rather than local storage, which is used for the general widget permission at the moment). As mentioned above, we haven't got much time to spend on this at the moment, but I'm keen to see a more detailed proposal, whenever you have it. Please feel free to hit me up in riot PM if you'd like to brainstorm or chat about this in any more detail in the mean time. Thanks! |
I'm happy to work on this in my spare time , so no worries about bandwidth - I'm well aware of the large list of things everyone is trying to get through :) The mobile issue is the most prominent to users: Setting up a sticker picker from Dimension (or non-scalar) means that when someone opens their mobile client they are presented with a 403. This can also apply to riot-web when someone sets up a Scalar (or dimension) sticker picker on one device then moves to another device with the other integration manager in use, presenting the same 403. I'll draw up some sequence diagrams. This is very much in the idea phase, so feedback on loopholes and general problems is greatly appreciated. |
Great. Thanks. Looking forward to seeing progress / updates on this. Any insight that you can give in to mysterious 403 / auth bugs that people are experiencing (for example with this sticker picker) would be greatly received, especially if you have ideas for (low-effort) fixes. I can well believe that a lot of this is related to people trying to use multiple IMs (or multiple deployments of scalar, for that matter). |
They are all related to having multiple versions of scalar/other managers as far as I know. It's mostly that the scalar token isn't valid for other managers (which is good), and that Riot chooses to not provide the scalar token when the widget URL isn't whitelisted (also good). In theory the proposal fixes this, and should be fairly low effort. |
Cool (I hoped/suspected as much). |
It's a really large diagram, but here it is. "IMBackend" is either the Scalar or Dimension backend in this case, where the API between the widget and backend is somewhat up to the backend, so it's a little vague in this area of the diagram.
|
I'm going to triage this as If anyone who doesn't have the power to add labels (but has read all of this issue) wants some other labels attaching, let me know :P |
@rxl881 I know you've been quite busy lately: if you get a chance can you take a look at this? The diagram above should explain in more detail for what I'm thinking. I also want to add the PR to my weekend project list :) |
@turt2live -- Thanks for prodding me on this one and apologies for not getting to it so far (as you suggest, I've been completely swamped trying to get v1 of HHS out the door). I'm hoping that once the remaining high priority items for HHS v1 are out of the way I'll have some time to start looking at / thinking about integration managers again. In the main, I think that the proposed schema looks very sensible and like a good improvement overall. However, there are a couple of items that fairly fundamentally change the way that things work at the moment, and need a little more thought, just to check that we are moving in the right direction for how access to widgets and their content may wish to be used by integration managers in the future. Specifically, (in scalar at least) we currently prevent the widget script (or other resources) from being loaded from the integration manager unless the client has a (scalar) token -- only the base widget HTML can be loaded without the token. This is needed to prevent users from accessing widget applications / content that they have not purchased or otherwise do not have access to. This obviously doesn't apply to any widgets at the moment (and may not be of concern to Dimension), but would definitely be a feature that integration managers would need in the future (e.g. in the style of Google Play or Apple app store type stores protecting application content). Probably the way to do this would be to have some sort of generic widget layer that can be loaded without auth which then provides the capability to bootstrap the full widget content when auth is successful, but that needs speccing out -- your thoughts would be really useful on this. Providing that this sort of bootstrapping layer exists, I guess that we could check and allow access to the full widget code / content at the 'Respond with user ID, and potentially a scalar_token for use' step? |
The bootstrapping layer feels a bit like an implementation detail, given it's the manager's responsibility to enforce whatever restrictions it wants. For Scalar this probably means changing the server-side to allow a js script to load that tries to acquire a token (either from the query string, cookie, localstorage, or by asking the client). That script would then set the appropriate values so the server can verify the token itself as it does today. Dimension has a similar approach where it tries in several ways to acquire a token before asking the server to verify it - it does however load the entire webapp before it does that (as it's not a concern). Something I don't think I mentioned was that I think the request for auth should be able to happen at any time. It's very possible that the widget can load just fine without auth, but when the user clicks a particular button or something then it wants to verify the user's identity. This is also admittedly to help fix a bug in Dimension where it tries to avoid communicating upstream to scalar unless required, which means that when the user adds something like matrix.org's IRC bridge it ends up breaking the user's current token by accident because it can't verify that they have the required auth to talk to scalar (and scalar rightfully complains about it) - a reauth prompt at this point would mean that Dimension can set up the appropriate tokens and get the right level of access to the user. |
@turt2live - I'm hoping that @jaywink is going to be able to work on this in the near future (based on your suggestions). |
yay! That's good to hear. just as a head's up: I'm working on standardizing how integrations interact with the Matrix ecosystem as part of https://github.com/matrix-org/matrix-doc/issues/1286. I haven't reached the point of formally proposing this in that work, and it may be suited as its own proposal. |
@jaywink is this still a work in progress for you? If not, I'm probably going to take it on this week in the evenings - the use cases are starting to stack up :( |
@turt2live unfortunately near future hasn't happened yet due to other priorities :/ It also looks like I wont be able to work on this in the very near near future, so I'm unassigning it from myself for now. The proposed solution sounds like a good one. However it does require lots of changes not just for widgets but also all the clients using them, so a lot of strings to hold pull in the rights direction. Currently there just isn't bandwitdh for that, sorry :/ Interested in following and reviewing the work though, if you happen to work on any parts of this. |
Covers the minimum of element-hq/element-web#7153 This does not handling automatically accepting/blocking widgets yet, however. This could lead to dialog irritation.
@jaywink no problem, thanks for the feedback. I've started a minimal example in Riot (matrix-org/matrix-react-sdk#2781) with accompanying widget in Dimension (see PR links) which demonstrates this in action. There's a bit of work to do still, but the groundwork is there. |
* Add unread indicator to the timelineCard header icon ([\element-hq#7156](matrix-org/matrix-react-sdk#7156)). Fixes element-hq#19635. * Only show core navigation elements (call/chat/notification/info) when a widget is maximised ([\element-hq#7114](matrix-org/matrix-react-sdk#7114)). Fixes element-hq#19632. * Improve ThreadPanel ctx menu accessibility ([\element-hq#7217](matrix-org/matrix-react-sdk#7217)). Fixes element-hq#19885. * Allow filtering room list during treeview navigation ([\element-hq#7219](matrix-org/matrix-react-sdk#7219)). Fixes element-hq#14702. * Add right panel chat timeline ([\element-hq#7112](matrix-org/matrix-react-sdk#7112)). Fixes element-hq#19633. * Hide server options hint when disable_custom_urls is true ([\element-hq#7215](matrix-org/matrix-react-sdk#7215)). Fixes element-hq#19919. * Improve right panel resize handle usability ([\element-hq#7204](matrix-org/matrix-react-sdk#7204)). Fixes element-hq#15145. Contributed by @weeman1337. * Spaces quick settings ([\element-hq#7196](matrix-org/matrix-react-sdk#7196)). * Maximised widgets always force a call to be shown in PIP mode ([\element-hq#7163](matrix-org/matrix-react-sdk#7163)). Fixes element-hq#19637. * Group Labs flags ([\#7190](matrix-org/matrix-react-sdk#7190)). * Show room context details in forward dialog ([\element-hq#7162](matrix-org/matrix-react-sdk#7162)). Fixes element-hq#19793. * Remove chevrons from RoomSummaryCard_Button ([\element-hq#7137](matrix-org/matrix-react-sdk#7137)). Fixes element-hq#19644. * Disable op/deop commands where user has no permissions ([\element-hq#7161](matrix-org/matrix-react-sdk#7161)). Fixes element-hq#15390. * Add option to change the size of images/videos in the timeline ([\element-hq#7017](matrix-org/matrix-react-sdk#7017)). Fixes element-hq/element-meta#49 element-hq#1520 and element-hq#19498. * Fix left panel glow in Safari ([\element-hq#7236](matrix-org/matrix-react-sdk#7236)). Fixes element-hq#19863. * Fix newline on edit messages with quotes ([\element-hq#7227](matrix-org/matrix-react-sdk#7227)). Fixes element-hq#12535. Contributed by @renancleyson-dev. * Guard against null refs in findSiblingElement ([\#7228](matrix-org/matrix-react-sdk#7228)). * Tweak bottom of space panel buttons in expanded state ([\element-hq#7213](matrix-org/matrix-react-sdk#7213)). Fixes element-hq#19921. * Fix multiline paragraph rendering as single line ([\element-hq#7210](matrix-org/matrix-react-sdk#7210)). Fixes element-hq#8786. Contributed by @renancleyson-dev. * Improve room list message previews ([\element-hq#7224](matrix-org/matrix-react-sdk#7224)). Fixes element-hq#17101 and element-hq#16169. * Fix EmojiPicker lazy loaded rendering bug ([\element-hq#7225](matrix-org/matrix-react-sdk#7225)). Fixes element-hq#15341. * Prevent default avatar in UserInfo having pointer cursor ([\element-hq#7218](matrix-org/matrix-react-sdk#7218)). Fixes element-hq#13872. * Prevent duplicate avatars in Event List Summaries ([\element-hq#7222](matrix-org/matrix-react-sdk#7222)). Fixes element-hq#17706. * Respect the home page as a context for the Home space ([\element-hq#7216](matrix-org/matrix-react-sdk#7216)). Fixes element-hq#19554. * Fix RoomUpgradeWarningBar exploding ([\element-hq#7214](matrix-org/matrix-react-sdk#7214)). Fixes element-hq#19920. * Polish threads misalignments and UI diversion ([\element-hq#7209](matrix-org/matrix-react-sdk#7209)). Fixes element-hq#19772, element-hq#19710 element-hq#19629 and element-hq#19711. * Fix Manage Restricted Join Rule Dialog for Spaces ([\element-hq#7208](matrix-org/matrix-react-sdk#7208)). Fixes element-hq#19610. * Fix wrongly showing unpin in pinned messages tile with no perms ([\element-hq#7197](matrix-org/matrix-react-sdk#7197)). Fixes element-hq#19886. * Make image size constrained by height when using the ImageSize.Large option ([\element-hq#7171](matrix-org/matrix-react-sdk#7171)). Fixes element-hq#19788. * Prevent programmatic scrolling within truncated room sublists ([\element-hq#7191](matrix-org/matrix-react-sdk#7191)). * Remove leading slash from /addwidget Jitsi confs ([\element-hq#7175](matrix-org/matrix-react-sdk#7175)). Fixes element-hq#19839. Contributed by @AndrewFerr. * Fix automatic composer focus, regressed by threads work ([\element-hq#7167](matrix-org/matrix-react-sdk#7167)). Fixes element-hq#19479. * Show space members when not invited even if summary didn't fail ([\element-hq#7153](matrix-org/matrix-react-sdk#7153)). Fixes element-hq#19781. * Prevent custom power levels from breaking roles & permissions tab ([\element-hq#7160](matrix-org/matrix-react-sdk#7160)). Fixes element-hq#19812. * Room Context Menu should respond to tag changes ([\element-hq#7154](matrix-org/matrix-react-sdk#7154)). Fixes element-hq#19776. * Fix an edge case when trying to join an upgraded room ([\element-hq#7159](matrix-org/matrix-react-sdk#7159)).
The auth dance for widgets like the sticker picker currently involves the integration manager being an integral part of your client, as Riot rightly refuses to provide a scalar token to the widget unless the widget belongs to the integration manager (as per config). This means that sticker pickers sometimes don't work on mobile or other Riot instances.
The widget could instead send an action over the postMessage API to request an OpenID token that it can then pass along to it's backend for verification. Given this is how the scalar token is sourced, this seems to be a reasonable thing to do. It's also hard to lie about your identity with this approach.
UX-wise, it might be a good idea to shove a popup to the user saying "Your sticker picker wants to verify your identity". The widget would probably have to be aware of this UX and not time out it's own request for asking for identity. A rate limit might need to be introduced, and the act of approving/denying the widget should probably persist instead of notifying the user every time they happen to reload the page.
The text was updated successfully, but these errors were encountered: