Port registration over to use InteractiveAuth #729

dbkr · 2017-02-27T18:40:04Z

These changes are moved over from the dbkr/msisdn_signin branch

Also fixes a couple of bugs:

Allow registration on other servers if you're registered as a guest
Fix spurious logout after completing registration in a different browser (Opening register link in same browser causes logout element-hq/element-web#3119)

Requires matrix-org/matrix-js-sdk#380 (hence failing tests)
Requires element-hq/element-web#3333

These changes are moved over from the dbkr/msisdn_signin branch

on registration (both for email validation and completion of the whole auth session).

* Also fix bug where you couldn't picxk a different server if you were already registered as a guest (because it still sent the access token which the new server rejected) * Propagate errors from UI auth back to registration so it goes back to the registration screen

as commented

dbkr · 2017-02-27T18:40:35Z

src/Login.js

@@ -0,0 +1,177 @@
+/*


Moved from Signup.js

richvdh · 2017-02-28T18:27:13Z

src/components/structures/InteractiveAuth.js

        });

        this._authLogic.attemptAuth().then((result) => {
            this.props.onFinished(true, result);
        }).catch((error) => {
+            this.props.onFinished(false, error);


in an InteractiveAuthDialog, this will close the dialog and swallow the error. Admittedly the current handling of non-interactive-auth errors seems to be totally broken, but this also seems wrong.

(I'm sure I tested non-interactive-auth errors. Perhaps it got broken when matthew added the auth cache stuff?)

Hmm, yeah. Seems like the issue is that while the non-dialog version wants the InteractiveAuth to go away, since there's no value in the thing sitting there saying 'error'. This isn't sensible for the dialog version though, and you do want the dialog to say it failed, then you can close it & try again.

So seems like the right fix is to make the dialog version catch onFinished(false) and replace the InteractiveAuth component with an error message?

Yes, I think so. It might also be worth renaming the onFinished property here to distinguish it from the one used in dialogs.

(This ended up mostly being merged by hand as git made a complete mess of the merge)

richvdh · 2017-03-02T12:22:46Z

src/components/structures/InteractiveAuth.js

@@ -61,11 +79,18 @@ export default React.createClass({
            authData: this.props.authData,
            doRequest: this._requestCallback,
            startAuthStage: this._startAuthStage,


now redundant

richvdh · 2017-03-02T12:32:26Z

src/components/views/login/InteractiveAuthEntryComponents.js

+    },
+
+    propTypes: {
+        matrixClient: React.PropTypes.object,


richvdh · 2017-03-02T12:33:00Z

src/components/views/login/InteractiveAuthEntryComponents.js

@@ -48,6 +66,7 @@ export const PasswordAuthEntry = React.createClass({
    },

    propTypes: {
+        matrixClient: React.PropTypes.object,


richvdh · 2017-03-02T12:33:25Z

src/components/views/login/InteractiveAuthEntryComponents.js

@@ -189,7 +280,7 @@ export const FallbackAuthEntry = React.createClass({
    },

    _onShowFallbackClick: function() {
-        var url = MatrixClientPeg.get().getFallbackAuthUrl(
+        var url = this.props.matrixClient.getFallbackAuthUrl(


add matrixClient to propTypes

richvdh · 2017-03-02T12:50:20Z

src/components/structures/MatrixChat.js

+                // stop the client: if we are syncing whilst the registration
+                // is completed in another browser, we'll be 401ed for using
+                // a guest access token for a non-guest account.
+                Lifecycle.stopMatrixClient();


do we need to resume it if the user cancels registration? or does that happen anyway?

It's already done (have commented)

richvdh · 2017-03-02T13:37:05Z

src/components/structures/MatrixChat.js

+        if (this.props.startingFragmentQueryParams.referrer) {
+            params.referrer = this.props.startingFragmentQueryParams.referrer;
+        }
+        return this.props.makeRegistrationUrl(params);


can you add makeRegistrationUrl to propTypes pls

richvdh · 2017-03-02T13:38:01Z

src/components/structures/MatrixChat.js

@@ -1120,7 +1132,6 @@ module.exports = React.createClass({
                    sessionId={this.state.register_session_id}
                    idSid={this.state.register_id_sid}
                    email={this.props.startingFragmentQueryParams.email}
-                    referrer={this.props.startingFragmentQueryParams.referrer}


need to remove referrer from Registration.propTypes

Actually, Registration was still using this: I've re-added it

ick. I wonder if the _makeRegistrationUrl wrapper to add the referrer belongs in Registration then. I'm bored of this PR though, let's just get it shipped

richvdh · 2017-03-02T13:39:06Z

src/components/structures/login/Registration.js

@@ -42,7 +42,7 @@ module.exports = React.createClass({
        onLoggedIn: React.PropTypes.func.isRequired,
        clientSecret: React.PropTypes.string,
        sessionId: React.PropTypes.string,
-        registrationUrl: React.PropTypes.string,
+        makeRegistrationUrl: React.PropTypes.func,


isRequired, I think?

richvdh · 2017-03-02T13:41:16Z

src/components/structures/login/Registration.js

@@ -42,7 +42,7 @@ module.exports = React.createClass({
        onLoggedIn: React.PropTypes.func.isRequired,


there's a TODO at line 35 which can go

richvdh · 2017-03-02T13:48:01Z

src/components/structures/login/Registration.js

-        });
+        }
+
+        trackPromise.then((teamToken) => {


what's trackPromise here? you seem to have vaped the code that set it?

Thanks - yeah, this was git making a mess of merging in luke's PR, and this must have been something I lost in trying to fix the merge.

as Registration was still using it

dbkr · 2017-03-02T15:09:23Z

ptal

richvdh · 2017-03-03T10:22:28Z

src/components/structures/login/Registration.js

-                            }
-                        });
-                    }, (err) => {
-                        console.error('Error getting team config', err);
                    });

                    return teamToken;


Now that I've got github to give me a sensible diff, I can see you've moved this inside the getTeam.then callback - previously teamToken would be returned even if the getTeam call failed. Was that deliberate? I suspect it will be fine, but thought I'd check.

Well spotted, no that wasn't deliberate, unsure it matters a great deal but have put it back.

to when teamToken was returned

richvdh · 2017-03-03T10:28:43Z

src/components/structures/InteractiveAuth.js

        });

        this._authLogic.attemptAuth().then((result) => {
            this.props.onFinished(true, result);
        }).catch((error) => {
+            this.props.onFinished(false, error);


Looks like this still needs attention.

richvdh

just the onFinished thing

when auth fails

richvdh

lgtm

dbkr added 5 commits February 24, 2017 11:41

Port registration over to use InteractiveAuth

5146750

These changes are moved over from the dbkr/msisdn_signin branch

Spurious newline

76b381b

Poll for authentication completion

454aa07

on registration (both for email validation and completion of the whole auth session).

Fix race when signup is completed elsewhere

c22db1d

as commented

dbkr assigned richvdh Feb 27, 2017

dbkr commented Feb 27, 2017

View reviewed changes

src/Login.js

@@ -0,0 +1,177 @@

/*

Copy link

Member Author

dbkr Feb 27, 2017

Choose a reason for hiding this comment

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

Moved from Signup.js

dbkr mentioned this pull request Feb 27, 2017

Support InteractiveAuth based registration element-hq/element-web#3333

Merged

richvdh reviewed Feb 28, 2017

View reviewed changes

dbkr added 4 commits March 1, 2017 10:45

Merge remote-tracking branch 'origin/develop' into dbkr/register_ui_auth

dd33624

(This ended up mostly being merged by hand as git made a complete mess of the merge)

PR feedback: Move the email token request here

c9ef879

Get errorText from error in stageUpdated

ef5d2bc

Fix syntax

9c5fb00

richvdh suggested changes Mar 2, 2017

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Mar 2, 2017

dbkr added 10 commits March 2, 2017 14:35

Remove redundant prop

770c1ce

Add proptypes / isRequired

518a749

doc

a5cf3a9

Add makeRegistrationUrl to proptypes

dd762cc

Oops, re-add referrer param

3028b33

as Registration was still using it

Actuall pass our proxy makeRegistrationUrl

0914226

Oops, lost defaultDeviceDisplayName

88586a0

Remove todo which is now done!

51b2001

makeRegistrationUrl is required

91ef26a

Oops, lost trackPromise

9c6782a

dbkr assigned richvdh and unassigned dbkr Mar 2, 2017

richvdh reviewed Mar 3, 2017

View reviewed changes

Revery unintentional change

8522231

to when teamToken was returned

richvdh reviewed Mar 3, 2017

View reviewed changes

richvdh suggested changes Mar 3, 2017

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Mar 3, 2017

Make UIAuth Dialog show an error

977b8a7

when auth fails

dbkr assigned richvdh and unassigned dbkr Mar 3, 2017

richvdh approved these changes Mar 3, 2017

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Mar 3, 2017

dbkr merged commit 1bdf213 into develop Mar 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port registration over to use InteractiveAuth #729

Port registration over to use InteractiveAuth #729

dbkr commented Feb 27, 2017 •

edited

Loading

dbkr Feb 27, 2017

richvdh Feb 28, 2017

dbkr Mar 1, 2017

richvdh Mar 2, 2017

dbkr Mar 3, 2017

richvdh Mar 2, 2017

dbkr Mar 2, 2017

richvdh Mar 2, 2017

dbkr Mar 2, 2017

richvdh Mar 2, 2017

richvdh Mar 2, 2017

dbkr Mar 2, 2017

richvdh Mar 2, 2017

dbkr Mar 2, 2017

richvdh Mar 2, 2017

dbkr Mar 2, 2017

richvdh Mar 2, 2017

dbkr Mar 2, 2017

richvdh Mar 3, 2017

richvdh Mar 2, 2017

richvdh Mar 2, 2017

dbkr Mar 2, 2017

richvdh Mar 2, 2017

dbkr Mar 2, 2017

dbkr commented Mar 2, 2017

richvdh Mar 3, 2017

dbkr Mar 3, 2017

richvdh Mar 3, 2017

richvdh left a comment

richvdh left a comment

		@@ -42,7 +42,7 @@ module.exports = React.createClass({
		onLoggedIn: React.PropTypes.func.isRequired,

Port registration over to use InteractiveAuth #729

Port registration over to use InteractiveAuth #729

Conversation

dbkr commented Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkr commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

dbkr commented Feb 27, 2017 •

edited

Loading