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

UIA-over-SSO leaves you marooned in a "please close this tab" tab #16219

Closed
richvdh opened this issue Jan 20, 2021 · 3 comments · Fixed by matrix-org/matrix-react-sdk#5578
Closed
Assignees
Labels
A-Authentication A-SSO P1 S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect

Comments

@richvdh
Copy link
Member

richvdh commented Jan 20, 2021

Once UIA-over-SSO completes, you end up stuck in a tab which looks like this:

image

@t3chguy
Copy link
Member

t3chguy commented Jan 20, 2021

Current SSO UIA flow doesn't listen to postMessage.

Current flow:

image

Clicking the affirmative button here does two things,

  • opens a new tab for you to complete SSO in
  • takes the app to the next state shown in the following screenshot

image

The affirmative button here does not work until you fulfil SSO UIA.

At the end of SSO UIA you see

image

If Element was to listen to postMessage here then it could detect the completion, close the tab, taking the user automatically back to the app.

This would ideally need an additional state in between the first two screenshots which tells the user to complete the flow in that new tab probably with a spinner. This state would automatically advance when the correct postMessage is received and the tab automatically be closed, the following diff gets most of the functionality in without fully implementing the intermediate UX phase which most users would never see (incomplete)

Index: src/components/views/auth/InteractiveAuthEntryComponents.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/components/views/auth/InteractiveAuthEntryComponents.js	(revision 461b56927852197fac75532f93a4ef90cd256716)
+++ src/components/views/auth/InteractiveAuthEntryComponents.js	(date 1611142142842)
@@ -595,7 +595,8 @@
     static UNSTABLE_LOGIN_TYPE = "org.matrix.login.sso";
 
     static PHASE_PREAUTH = 1; // button to start SSO
-    static PHASE_POSTAUTH = 2; // button to confirm SSO completed
+    static PHASE_INAUTH = 2; // spinner with a skip to post-auth if postMessage failed
+    static PHASE_POSTAUTH = 3; // button to confirm SSO completed
 
     _ssoUrl: string;
 
@@ -609,6 +610,9 @@
             this.props.authSessionId,
         );
 
+        this._popupWindow = null;
+        window.addEventListener("message", this._onReceiveMessage);
+
         this.state = {
             phase: SSOAuthEntry.PHASE_PREAUTH,
         };
@@ -618,12 +622,29 @@
         this.props.onPhaseChange(SSOAuthEntry.PHASE_PREAUTH);
     }
 
+    componentWillUnmount() {
+        window.removeEventListener("message", this._onReceiveMessage);
+        if (this._popupWindow) {
+            this._popupWindow.close();
+        }
+    }
+
+    _onReceiveMessage = event => {
+        if (event.data === "authDone" && event.origin === this.props.matrixClient.getHomeserverUrl()) {
+            this.setState({ phase: SSOAuthEntry.PHASE_POSTAUTH });
+            if (this._popupWindow) {
+                this._popupWindow.close();
+            }
+        }
+    };
+
     onStartAuthClick = () => {
         // Note: We don't use PlatformPeg's startSsoAuth functions because we almost
         // certainly will need to open the thing in a new tab to avoid losing application
         // context.
 
-        window.open(this._ssoUrl, '_blank');
+        this._popupWindow = window.open(this._ssoUrl, "_blank");
         this.setState({phase: SSOAuthEntry.PHASE_INAUTH});
         this.props.onPhaseChange(SSOAuthEntry.PHASE_INAUTH);
     };
@@ -711,7 +732,6 @@
             this.props.authSessionId,
         );
         this._popupWindow = window.open(url);
-        this._popupWindow.opener = null;
     };
 
     _onReceiveMessage = event => {
Index: src/components/views/dialogs/InteractiveAuthDialog.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/components/views/dialogs/InteractiveAuthDialog.js	(revision 461b56927852197fac75532f93a4ef90cd256716)
+++ src/components/views/dialogs/InteractiveAuthDialog.js	(date 1611141952479)
@@ -85,6 +85,12 @@
                 continueText: _t("Single Sign On"),
                 continueKind: "primary",
             },
+            [SSOAuthEntry.PHASE_INAUTH]: {
+                title: _t("Continue in new tab"),
+                body: _t("To continue, finish signing in with Single Sign On."),
+                continueText: _t("Done"),
+                continueKind: "secondary",
+            },
             [SSOAuthEntry.PHASE_POSTAUTH]: {
                 title: _t("Confirm to continue"),
                 body: _t("Click the button below to confirm your identity."),

@richvdh
Copy link
Member Author

richvdh commented Jan 20, 2021

-        this._popupWindow.opener = null;

isn't there some security reason this is an important thing to do?

@t3chguy
Copy link
Member

t3chguy commented Jan 20, 2021

That's actually an unrelated change for FallbackAuthEntry which looks like a regression I introduced when I was hap-hazardly slapping it on every window.open without realising that one needs it for postMessage. I believe it should be fine but might need to use a different context like _blank or similar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Authentication A-SSO P1 S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants