NOJIRA Fix Passport registration issue not polling user info #935
Conversation
| @@ -369,15 +369,23 @@ export default class AuthManager { | |||
| }, PassportErrorType.LOGOUT_ERROR); | |||
| } | |||
|
|
|||
| public async loginSilent(): Promise<User | null> { | |||
| return withPassportError<User | null>(async () => this.getUser(), PassportErrorType.SILENT_LOGIN_ERROR); | |||
| public async loginSilent({ forceRefresh } = { forceRefresh: false }): Promise<User | null> { | |||
There was a problem hiding this comment.
I used to an object instead of a raw boolean param for improved reliability.
I.e.
authManager.loginSilent({ forceRefresh: true });
reads better than
authManager.loginSilent(true);
| private async getWebUser({ forceRefresh = false }: { forceRefresh: boolean }) : Promise<User | null> { | ||
| // Force an HTTP call to the OIDC server's authorization endpoint through an iframe | ||
| if (forceRefresh) { | ||
| const newOidcUser = await this.userManager.signinSilent(); |
There was a problem hiding this comment.
if force login through he iframe, the silent_redirect_uri have to be set.
Do we have/need a document for this configuration?
There was a problem hiding this comment.
But if we're forcing a fetch, why do we want to get it from local storage using userManager.getUser()?
I may be misunderstanding your point
There was a problem hiding this comment.
Sorry, changed my original comment.
Forcing a fetch signinSilent need either refresh token or set silent_redirect_uri.
I thought we didn't want to login via the iframe, so we must need the refresh token. Otherwise, it will throw an error when calling signinSilent.
But I realized I was wrong after I saw the comments in line 380.
There was a problem hiding this comment.
if force login through he iframe, the silent_redirect_uri have been set.
do we have/need a document for this configuration?
@carmen0208 do we?
Check out our previous implementation: https://github.com/immutable/ts-immutable-sdk/pull/913/files#diff-0e2f0102f41362f5a9e1c19e1b9431b869889486e498fc903832b769214dfaebL377
We were already calling signinSilent
There was a problem hiding this comment.
@Jon-Alonso
Yes, we had, but it is ok because it will ensure it contains the refresh_token when calling the signinSilent, and the way to ensure it is by calling the this.userManager.getUser() before, which returns the user with the refresh token.
The risk here is if there is no refresh token(because we never check), and also if the silent_redirect_uri is not been configured. signinSilent will throw an error.
There was a problem hiding this comment.
By the looks of it the actual lib is already doing this for us: https://github.com/authts/oidc-client-ts/blob/f3a805aa00a021a8844498df8a3c8dd64c7c2f7b/src/UserManager.ts#L254
Summary
Fixes an issue preventing Passport account signups from working.
Why the changes
The issue being fixed was introduced in https://github.com/immutable/ts-immutable-sdk/pull/913/files#diff-0e2f0102f41362f5a9e1c19e1b9431b869889486e498fc903832b769214dfaebR376
which was previously always forcing an HTTP request through signing silent to get the user.