Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Widget OpenID reauth implementation #2781

Merged
merged 10 commits into from
Mar 27, 2019
Merged

Widget OpenID reauth implementation #2781

merged 10 commits into from
Mar 27, 2019

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Mar 13, 2019

Fixes element-hq/element-web#7153

The proposal for this is part of https://github.com/matrix-org/matrix-doc/issues/1286. The relevant snippet (and therefore draft) is available here: https://gist.github.com/turt2live/669bbffe882c8541f057daeb29e18b86

As a proof of concept, I've also created a Dimension widget which does nothing more than report your user ID back to you - all without using the scalar_token provided in the query string, and without you having to configure your client to point at a Dimension instance.

Testing procedure:

  1. Create a new room to play with widgets in, or use an existing one
  2. Using /devtools, create an im.vector.modular.widgets state event with state key reauthwidget and content of:
    {
        "type": "customwidget",
        "url": "https://dimension.t2bot.io/widgets/reauth",
        "name": "OpenID Reauth Demo",
        "data": {},
        "id": "reauthwidget",
        "waitForIframeLoad": true,
        "creatorUserId": "@travis:dev.t2host.io"
    }
    You may have to change the creatorUserId to be your user ID.
  3. Close devtools and open the widget
  4. Click the blue "Start OpenID auth" button. This should result in a popup in Riot.
  5. Within the popup, decide if you want to always allow the widget and click either 'Allow' or 'Deny'
  6. If you selected allow, the widget should update to show you your user ID.

To repeat the process, refresh the page. There's currently an unrelated bug that prevents reloading the widget from being enough to unstick the state: the whole page needs to be refreshed. This will be addressed in a different PR (#2799).

Source for the widget: https://github.com/turt2live/matrix-dimension/blob/master/web/app/widget-wrappers/reauth-example/reauth-example.component.ts

image


This is being PR'd as a community member:

Signed-off-by: Travis Ralston <travis@t2bot.io>

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.
@turt2live turt2live changed the title [WIP] Widget OpenID reauth implementation Widget OpenID reauth implementation Mar 16, 2019
@turt2live turt2live marked this pull request as ready for review March 16, 2019 03:59
@turt2live turt2live requested a review from a team March 16, 2019 03:59
@bwindels bwindels requested review from bwindels and removed request for a team March 20, 2019 08:38
Copy link
Contributor

@bwindels bwindels left a comment

Choose a reason for hiding this comment

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

2 comments, otherwise lgtm

src/FromWidgetPostMessageApi.js Outdated Show resolved Hide resolved
src/WidgetMessaging.js Show resolved Hide resolved
@turt2live turt2live requested a review from bwindels March 20, 2019 21:03
@jryans jryans self-requested a review March 22, 2019 14:13
Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

I have read through your proposal snippet and the original issue. This is my first time looking at the code related to widgets, but I am hoping to understand it better through this review. 😄

src/WidgetMessaging.js Outdated Show resolved Hide resolved
src/WidgetMessaging.js Outdated Show resolved Hide resolved

// True to put the toggle in front of the label
// Default false.
toggleInFront: PropTypes.bool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we sometimes want the opposite order?

Copy link
Member Author

Choose a reason for hiding this comment

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

Usually the toggle is at the end of the label, however in a dialog this doesn't make much sense to the user. This is the first instance of needing to do this, and it seems easiest to just put a boolean on it.

Everywhere else:
image

Widget dialog:
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, well, technically it's fine. I'd suggest asking Nad if he would prefer only having one layout of toggles or if he agrees it seems better reversed as you've done it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Summarizing the discussion from earlier today: We'll take care of this in element-hq/element-web#7566

@turt2live turt2live requested a review from jryans March 24, 2019 05:28
Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

I think we are getting close! I'd like to know more about the user widgets first before approving.

src/settings/Settings.js Outdated Show resolved Hide resolved
throw new Error("No matching user widget to form security key");
}

widgetLocation = userWidget.sender;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the sender of a user widget always yourself? (That's what it seems to be for stickerpicker widget, at least...)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, it has to be. They are stored in account data and other places enforce this assumption.

@turt2live turt2live self-assigned this Mar 25, 2019
@turt2live turt2live requested a review from jryans March 26, 2019 03:22
@turt2live turt2live removed their assignment Mar 26, 2019
Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

Great, I think this looks good to me! 😁 Thanks for working through the security details with me.

Copy link
Contributor

@bwindels bwindels left a comment

Choose a reason for hiding this comment

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

lgtm!

@turt2live turt2live merged commit ddcb7a6 into develop Mar 27, 2019
@turt2live turt2live deleted the travis/openid-widget branch March 27, 2019 09:42
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.

Scalar client API to request an OpenID token (read: improve auth dance for widgets)
3 participants