OIDC: extract success/failure handlers from token login function #11154
Conversation
kerryarchibald
added
the
T-Task
|
Strict error is not related to these changes. |
src/Lifecycle.ts
Outdated
| title: _t("We couldn't log you in"), | ||
| description, | ||
| button: _t("Try again"), | ||
| onFinished: tryAgain, |
There was a problem hiding this comment.
why do we pass shouldTryAgain into the callback function? wouldn't it make more sense for this to be
| onFinished: tryAgain, | |
| onFinished: (shouldTryAgain) => { if (shouldTryAgain && !!tryAgain ) { tryAgain(); } }, |
or something to that effect?
There was a problem hiding this comment.
The dialog handles the case of a falsy onFinished
There was a problem hiding this comment.
oh, now I think I see what you mean (if onFinished is falsey, it won't be called), but that's not really my point here.
My point is that I don't think it should be up to the tryAgain callback to check whether the user pressed "Try again" or "Cancel".
src/Lifecycle.ts
Outdated
| "We asked the browser to remember which homeserver you use to let you sign in, " + | ||
| "but unfortunately your browser has forgotten it. Go to the sign in page and try again.", | ||
| ), | ||
| ); | ||
| Modal.createDialog(ErrorDialog, { |
There was a problem hiding this comment.
this dialog creation looks redundant now?
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
src/Lifecycle.ts
Outdated
| const tryAgainCallback: TryAgainFunction = (shouldTryAgain) => { | ||
| if (shouldTryAgain) { | ||
| const cli = createClient({ | ||
| baseUrl: homeserver, | ||
| idBaseUrl: identityServer, | ||
| }); | ||
| const idpId = localStorage.getItem(SSO_IDP_ID_KEY) || undefined; | ||
| PlatformPeg.get()?.startSingleSignOn(cli, "sso", fragmentAfterLogin, idpId, SSOAction.LOGIN); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Per the other thread: I don't understand why the tryAgainCallback is called even if we don't actually want to try again.
| const tryAgainCallback: TryAgainFunction = (shouldTryAgain) => { | |
| if (shouldTryAgain) { | |
| const cli = createClient({ | |
| baseUrl: homeserver, | |
| idBaseUrl: identityServer, | |
| }); | |
| const idpId = localStorage.getItem(SSO_IDP_ID_KEY) || undefined; | |
| PlatformPeg.get()?.startSingleSignOn(cli, "sso", fragmentAfterLogin, idpId, SSOAction.LOGIN); | |
| } | |
| }; | |
| const tryAgainCallback: TryAgainFunction = () => { | |
| const cli = createClient({ | |
| baseUrl: homeserver, | |
| idBaseUrl: identityServer, | |
| }); | |
| const idpId = localStorage.getItem(SSO_IDP_ID_KEY) || undefined; | |
| PlatformPeg.get()?.startSingleSignOn(cli, "sso", fragmentAfterLogin, idpId, SSOAction.LOGIN); | |
| }; |
For element-hq/element-web#25657
Extracts success/failure handlers from
Lifecycle.attemptTokenLoginfunction so they can be reused post OIDC authorization.Checklist
This change is marked as an internal change (Task), so will not be included in the changelog.