From 492911dc8c68886cb38092ce70bef31c3044b5e8 Mon Sep 17 00:00:00 2001 From: Janne Mareike Koschinski Date: Fri, 20 May 2022 13:27:31 +0200 Subject: [PATCH 01/11] Fix: a typo causing differentLoggedInUserId to be set incorrectly --- src/components/structures/auth/Registration.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/structures/auth/Registration.tsx b/src/components/structures/auth/Registration.tsx index 7515a4f0d90..5b32a49567c 100644 --- a/src/components/structures/auth/Registration.tsx +++ b/src/components/structures/auth/Registration.tsx @@ -351,9 +351,9 @@ export default class Registration extends React.Component { // starting the registration process. This isn't perfect since it's possible // the user had a separate guest session they didn't actually mean to replace. const [sessionOwner, sessionIsGuest] = await Lifecycle.getStoredSessionOwner(); - if (sessionOwner && !sessionIsGuest && sessionOwner !== response.userId) { + if (sessionOwner && !sessionIsGuest && sessionOwner !== response.user_id) { logger.log( - `Found a session for ${sessionOwner} but ${response.userId} has just registered.`, + `Found a session for ${sessionOwner} but ${response.user_id} has just registered.`, ); newState.differentLoggedInUserId = sessionOwner; } From 1fe4d645a92aed68734c3736b781a3486b6ffcf4 Mon Sep 17 00:00:00 2001 From: Janne Mareike Koschinski Date: Fri, 20 May 2022 13:28:29 +0200 Subject: [PATCH 02/11] Feat: add debug logging --- src/components/structures/auth/Registration.tsx | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/components/structures/auth/Registration.tsx b/src/components/structures/auth/Registration.tsx index 5b32a49567c..7f224110b7c 100644 --- a/src/components/structures/auth/Registration.tsx +++ b/src/components/structures/auth/Registration.tsx @@ -39,6 +39,14 @@ import Spinner from "../../views/elements/Spinner"; import { AuthHeaderDisplay } from './header/AuthHeaderDisplay'; import { AuthHeaderProvider } from './header/AuthHeaderProvider'; +const DEBUG = false; +let debuglog = function(msg: string, ...args: any[]) {}; + +if (DEBUG) { + // using bind means that we get to keep useful line numbers in the console + debuglog = logger.log.bind(console); +} + interface IProps { serverConfig: ValidatedServerConfig; defaultDeviceDisplayName: string; @@ -288,6 +296,7 @@ export default class Registration extends React.Component { }; private onUIAuthFinished = async (success: boolean, response: any) => { + debuglog("Registration: ui authentication finished: ", { success, response }); if (!success) { let errorText = response.message || response.toString(); // can we give a better error message? @@ -358,6 +367,7 @@ export default class Registration extends React.Component { newState.differentLoggedInUserId = sessionOwner; } + debuglog("Registration: ui auth finished:", Boolean(response.access_token)); if (response.access_token) { await this.props.onLoggedIn({ userId: response.user_id, @@ -435,7 +445,7 @@ export default class Registration extends React.Component { inhibit_login: undefined, }; if (auth) registerParams.auth = auth; - if (inhibitLogin !== undefined && inhibitLogin !== null) registerParams.inhibit_login = inhibitLogin; + debuglog("Registration: sending registration request:", auth); return this.state.matrixClient.registerRequest(registerParams); }; From 3a6d60e4264d82077a2df20f98b060dd8d6dc36d Mon Sep 17 00:00:00 2001 From: Janne Mareike Koschinski Date: Fri, 20 May 2022 13:30:08 +0200 Subject: [PATCH 03/11] Feat: move login inhibition from before sending the request to after receiving the response --- .../structures/auth/Registration.tsx | 33 +++++++++++-------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/components/structures/auth/Registration.tsx b/src/components/structures/auth/Registration.tsx index 7f224110b7c..f0dd2b352cc 100644 --- a/src/components/structures/auth/Registration.tsx +++ b/src/components/structures/auth/Registration.tsx @@ -367,8 +367,24 @@ export default class Registration extends React.Component { newState.differentLoggedInUserId = sessionOwner; } - debuglog("Registration: ui auth finished:", Boolean(response.access_token)); - if (response.access_token) { + // if we don't have an email at all, only one client can be involved in this flow, and we can directly log in. + // + // if we've got an email, it needs to be verified. in that case, two clients can be involved in this flow, the + // original client starting the process and the client that submitted the verification token. After the token + // has been submitted, it can not be used again. + // + // we can distinguish them based on whether the client has form values saved (if so, it's the one that started + // the registration), or whether it doesn't have any form values saved (in which case it's the client that + // verified the email address) + // + // as the client that started registration may be gone by the time we've verified the email, and only the client + // that verified the email is guaranteed to exist, we'll always do the login in that client. + const hasEmail = Boolean(this.state.formVals.email); + const hasAccessToken = Boolean(response.access_token); + debuglog("Registration: ui auth finished:", { hasEmail, hasAccessToken }); + if (!hasEmail && hasAccessToken) { + // we'll only try logging in if we either have no email to verify at all or we're the client that verified + // the email, not the client that started the registration flow await this.props.onLoggedIn({ userId: response.user_id, deviceId: response.device_id, @@ -426,22 +442,13 @@ export default class Registration extends React.Component { }; private makeRegisterRequest = auth => { - // We inhibit login if we're trying to register with an email address: this - // avoids a lot of complex race conditions that can occur if we try to log - // the user in one one or both of the tabs they might end up with after - // clicking the email link. - let inhibitLogin = Boolean(this.state.formVals.email); - - // Only send inhibitLogin if we're sending username / pw params - // (Since we need to send no params at all to use the ones saved in the - // session). - if (!this.state.formVals.password) inhibitLogin = null; - const registerParams = { username: this.state.formVals.username, password: this.state.formVals.password, initial_device_display_name: this.props.defaultDeviceDisplayName, auth: undefined, + // we still want to avoid the race conditions involved with multiple clients handling registration, but + // we'll handle these after we've received the access_token in onUIAuthFinished inhibit_login: undefined, }; if (auth) registerParams.auth = auth; From 5409332f263d4b71dbc67790e39ad1e0ea884f0e Mon Sep 17 00:00:00 2001 From: Janne Mareike Koschinski Date: Fri, 20 May 2022 13:30:36 +0200 Subject: [PATCH 04/11] Feat: update UI to match the new auto-login after registration flow --- .../structures/auth/Registration.tsx | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/components/structures/auth/Registration.tsx b/src/components/structures/auth/Registration.tsx index f0dd2b352cc..a27bb3edd57 100644 --- a/src/components/structures/auth/Registration.tsx +++ b/src/components/structures/auth/Registration.tsx @@ -614,22 +614,22 @@ export default class Registration extends React.Component { { _t("Continue with previous account") }

; - } else if (this.state.formVals.password) { - // We're the client that started the registration - regDoneText =

{ _t( - "Log in to your new account.", {}, - { - a: (sub) => { sub }, - }, - ) }

; } else { - // We're not the original client: the user probably got to us by clicking the - // email validation link. We can't offer a 'go straight to your account' link - // as we don't have the original creds. + // regardless of whether we're the client that started the registration or not, we should + // try our credentials anyway regDoneText =

{ _t( - "You can now close this window or log in to your new account.", {}, + "Log in to your new account.", {}, { - a: (sub) => { sub }, + a: (sub) => { + const sessionLoaded = await this.onLoginClickWithCheck(event); + if (sessionLoaded) { + dis.dispatch({ action: "view_home_page" }); + } + }} + >{ sub }, }, ) }

; } From f66ac6cb4b6c54ea1133eca7105d7b86035db596 Mon Sep 17 00:00:00 2001 From: Janne Mareike Koschinski Date: Fri, 20 May 2022 13:48:53 +0200 Subject: [PATCH 05/11] feat: update translations --- src/i18n/strings/en_EN.json | 1 - 1 file changed, 1 deletion(-) diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 9b82b120c32..87dbb0fa914 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -3232,7 +3232,6 @@ "Your new account (%(newAccountId)s) is registered, but you're already logged into a different account (%(loggedInUserId)s).": "Your new account (%(newAccountId)s) is registered, but you're already logged into a different account (%(loggedInUserId)s).", "Continue with previous account": "Continue with previous account", "Log in to your new account.": "Log in to your new account.", - "You can now close this window or log in to your new account.": "You can now close this window or log in to your new account.", "Registration Successful": "Registration Successful", "Create account": "Create account", "Host account on": "Host account on", From 0a3ba99b69b1bff89f0aa210eae5f41521d4e9fa Mon Sep 17 00:00:00 2001 From: Janne Mareike Koschinski Date: Fri, 20 May 2022 15:27:07 +0200 Subject: [PATCH 06/11] fix: update debuglog to new standard --- src/components/structures/auth/Registration.tsx | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/components/structures/auth/Registration.tsx b/src/components/structures/auth/Registration.tsx index a27bb3edd57..8d1da2322fd 100644 --- a/src/components/structures/auth/Registration.tsx +++ b/src/components/structures/auth/Registration.tsx @@ -38,14 +38,13 @@ import InteractiveAuth from "../InteractiveAuth"; import Spinner from "../../views/elements/Spinner"; import { AuthHeaderDisplay } from './header/AuthHeaderDisplay'; import { AuthHeaderProvider } from './header/AuthHeaderProvider'; +import SettingsStore from '../../../settings/SettingsStore'; -const DEBUG = false; -let debuglog = function(msg: string, ...args: any[]) {}; - -if (DEBUG) { - // using bind means that we get to keep useful line numbers in the console - debuglog = logger.log.bind(console); -} +const debuglog = (...args: any[]) => { + if (SettingsStore.getValue("debug_registration")) { + logger.log.call(console, "Registration debuglog:", ...args); + } +}; interface IProps { serverConfig: ValidatedServerConfig; From 1d41b9518ce142d2d1561946893c495acf3880a3 Mon Sep 17 00:00:00 2001 From: Janne Mareike Koschinski Date: Fri, 20 May 2022 17:00:19 +0200 Subject: [PATCH 07/11] types: improve authdata types --- src/components/structures/InteractiveAuth.tsx | 17 ++++++++++++----- src/components/structures/auth/Registration.tsx | 12 ++++++------ 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/components/structures/InteractiveAuth.tsx b/src/components/structures/InteractiveAuth.tsx index b42e65d57fb..4342355c74c 100644 --- a/src/components/structures/InteractiveAuth.tsx +++ b/src/components/structures/InteractiveAuth.tsx @@ -31,6 +31,17 @@ import Spinner from "../views/elements/Spinner"; export const ERROR_USER_CANCELLED = new Error("User cancelled auth session"); +type InteractiveAuthCallbackSuccess = ( + success: true, + response: IAuthData, + extra?: { emailSid?: string, clientSecret?: string } +) => void; +type InteractiveAuthCallbackFailure = ( + success: false, + response: IAuthData | Error, +) => void; +export type InteractiveAuthCallback = InteractiveAuthCallbackSuccess & InteractiveAuthCallbackFailure; + interface IProps { // matrix client to use for UI auth requests matrixClient: MatrixClient; @@ -66,11 +77,7 @@ interface IProps { // the auth session. // * clientSecret {string} The client secret used in auth // sessions with the ID server. - onAuthFinished( - status: boolean, - result: IAuthData | Error, - extra?: { emailSid?: string, clientSecret?: string }, - ): void; + onAuthFinished: InteractiveAuthCallback; // As js-sdk interactive-auth requestEmailToken?(email: string, secret: string, attempt: number, session: string): Promise<{ sid: string }>; // Called when the stage changes, or the stage's phase changes. First diff --git a/src/components/structures/auth/Registration.tsx b/src/components/structures/auth/Registration.tsx index 8d1da2322fd..8c037db2bf6 100644 --- a/src/components/structures/auth/Registration.tsx +++ b/src/components/structures/auth/Registration.tsx @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { createClient } from 'matrix-js-sdk/src/matrix'; +import { AuthType, createClient } from 'matrix-js-sdk/src/matrix'; import React, { Fragment, ReactNode } from 'react'; import { MatrixClient } from "matrix-js-sdk/src/client"; import classNames from "classnames"; @@ -34,7 +34,7 @@ import RegistrationForm from '../../views/auth/RegistrationForm'; import AccessibleButton from '../../views/elements/AccessibleButton'; import AuthBody from "../../views/auth/AuthBody"; import AuthHeader from "../../views/auth/AuthHeader"; -import InteractiveAuth from "../InteractiveAuth"; +import InteractiveAuth, { InteractiveAuthCallback } from "../InteractiveAuth"; import Spinner from "../../views/elements/Spinner"; import { AuthHeaderDisplay } from './header/AuthHeaderDisplay'; import { AuthHeaderProvider } from './header/AuthHeaderProvider'; @@ -294,10 +294,10 @@ export default class Registration extends React.Component { ); }; - private onUIAuthFinished = async (success: boolean, response: any) => { + private onUIAuthFinished: InteractiveAuthCallback = async (success, response) => { debuglog("Registration: ui authentication finished: ", { success, response }); if (!success) { - let errorText = response.message || response.toString(); + let errorText: ReactNode = response.message || response.toString(); // can we give a better error message? if (response.errcode === 'M_RESOURCE_LIMIT_EXCEEDED') { const errorTop = messageForResourceLimitError( @@ -320,10 +320,10 @@ export default class Registration extends React.Component {

{ errorTop }

{ errorDetail }

; - } else if (response.required_stages && response.required_stages.indexOf('m.login.msisdn') > -1) { + } else if (response.required_stages && response.required_stages.indexOf(AuthType.Msisdn) > -1) { let msisdnAvailable = false; for (const flow of response.available_flows) { - msisdnAvailable = msisdnAvailable || flow.stages.includes('m.login.msisdn'); + msisdnAvailable = msisdnAvailable || flow.stages.includes(AuthType.Msisdn); } if (!msisdnAvailable) { errorText = _t('This server does not support authentication with a phone number.'); From 0e436b25dcccf592ef3b0c665e591c5d8bf9885c Mon Sep 17 00:00:00 2001 From: Janne Mareike Koschinski Date: Fri, 20 May 2022 17:43:50 +0200 Subject: [PATCH 08/11] types: adapt to changed authcallback type --- src/components/views/dialogs/DeactivateAccountDialog.tsx | 2 +- src/components/views/dialogs/InteractiveAuthDialog.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/views/dialogs/DeactivateAccountDialog.tsx b/src/components/views/dialogs/DeactivateAccountDialog.tsx index dbdc3b3639a..a82f56bd81c 100644 --- a/src/components/views/dialogs/DeactivateAccountDialog.tsx +++ b/src/components/views/dialogs/DeactivateAccountDialog.tsx @@ -104,7 +104,7 @@ export default class DeactivateAccountDialog extends React.Component { + private onUIAuthFinished = (success, result) => { if (success) return; // great! makeRequest() will be called too. if (result === ERROR_USER_CANCELLED) { diff --git a/src/components/views/dialogs/InteractiveAuthDialog.tsx b/src/components/views/dialogs/InteractiveAuthDialog.tsx index 46bad5fd0e1..08cd05db4bc 100644 --- a/src/components/views/dialogs/InteractiveAuthDialog.tsx +++ b/src/components/views/dialogs/InteractiveAuthDialog.tsx @@ -117,7 +117,7 @@ export default class InteractiveAuthDialog extends React.Component { + private onAuthFinished = (success, result): void => { if (success) { this.props.onFinished(true, result); } else { From 0ea713fa07af4a69b73c1038772eac43174a6818 Mon Sep 17 00:00:00 2001 From: Janne Mareike Koschinski Date: Fri, 20 May 2022 18:39:25 +0200 Subject: [PATCH 09/11] fix: add forgotten setting --- src/settings/Settings.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/settings/Settings.tsx b/src/settings/Settings.tsx index 0df3d87481e..e3d5b0afce0 100644 --- a/src/settings/Settings.tsx +++ b/src/settings/Settings.tsx @@ -929,6 +929,10 @@ export const SETTINGS: {[setting: string]: ISetting} = { supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS, default: false, }, + "debug_registration": { + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS, + default: false, + }, "audioInputMuted": { supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS, default: false, From 0a14d4739d85837e3918bc224e4618b65af51bcd Mon Sep 17 00:00:00 2001 From: Janne Mareike Koschinski Date: Fri, 20 May 2022 19:03:26 +0200 Subject: [PATCH 10/11] fix: switch auth callbacks to correct types --- src/components/views/dialogs/DeactivateAccountDialog.tsx | 4 ++-- src/components/views/dialogs/InteractiveAuthDialog.tsx | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/components/views/dialogs/DeactivateAccountDialog.tsx b/src/components/views/dialogs/DeactivateAccountDialog.tsx index a82f56bd81c..55251025fb7 100644 --- a/src/components/views/dialogs/DeactivateAccountDialog.tsx +++ b/src/components/views/dialogs/DeactivateAccountDialog.tsx @@ -22,7 +22,7 @@ import { logger } from "matrix-js-sdk/src/logger"; import Analytics from '../../../Analytics'; import { MatrixClientPeg } from '../../../MatrixClientPeg'; import { _t } from '../../../languageHandler'; -import InteractiveAuth, { ERROR_USER_CANCELLED } from "../../structures/InteractiveAuth"; +import InteractiveAuth, { ERROR_USER_CANCELLED, InteractiveAuthCallback } from "../../structures/InteractiveAuth"; import { DEFAULT_PHASE, PasswordAuthEntry, SSOAuthEntry } from "../auth/InteractiveAuthEntryComponents"; import StyledCheckbox from "../elements/StyledCheckbox"; import BaseDialog from "./BaseDialog"; @@ -104,7 +104,7 @@ export default class DeactivateAccountDialog extends React.Component { + private onUIAuthFinished: InteractiveAuthCallback = (success, result) => { if (success) return; // great! makeRequest() will be called too. if (result === ERROR_USER_CANCELLED) { diff --git a/src/components/views/dialogs/InteractiveAuthDialog.tsx b/src/components/views/dialogs/InteractiveAuthDialog.tsx index 08cd05db4bc..6f10790811e 100644 --- a/src/components/views/dialogs/InteractiveAuthDialog.tsx +++ b/src/components/views/dialogs/InteractiveAuthDialog.tsx @@ -22,7 +22,7 @@ import { IAuthData } from "matrix-js-sdk/src/interactive-auth"; import { _t } from '../../../languageHandler'; import AccessibleButton from '../elements/AccessibleButton'; -import InteractiveAuth, { ERROR_USER_CANCELLED } from "../../structures/InteractiveAuth"; +import InteractiveAuth, { ERROR_USER_CANCELLED, InteractiveAuthCallback } from "../../structures/InteractiveAuth"; import { SSOAuthEntry } from "../auth/InteractiveAuthEntryComponents"; import BaseDialog from "./BaseDialog"; import { IDialogProps } from "./IDialogProps"; @@ -117,7 +117,7 @@ export default class InteractiveAuthDialog extends React.Component { + private onAuthFinished: InteractiveAuthCallback = (success, result): void => { if (success) { this.props.onFinished(true, result); } else { From 452b5376c75a1ff08042fb5c0dcb8379e6b3508d Mon Sep 17 00:00:00 2001 From: Janne Mareike Koschinski Date: Fri, 20 May 2022 19:03:55 +0200 Subject: [PATCH 11/11] fix: use .includes where possible --- src/components/structures/auth/Registration.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/structures/auth/Registration.tsx b/src/components/structures/auth/Registration.tsx index 8c037db2bf6..d349205ab92 100644 --- a/src/components/structures/auth/Registration.tsx +++ b/src/components/structures/auth/Registration.tsx @@ -320,7 +320,7 @@ export default class Registration extends React.Component {

{ errorTop }

{ errorDetail }

; - } else if (response.required_stages && response.required_stages.indexOf(AuthType.Msisdn) > -1) { + } else if (response.required_stages && response.required_stages.includes(AuthType.Msisdn)) { let msisdnAvailable = false; for (const flow of response.available_flows) { msisdnAvailable = msisdnAvailable || flow.stages.includes(AuthType.Msisdn);