Conversation
7535a5b to
d856378
Compare
squash: Adds refresh token use when refresh token is needed on connection resuming. squash: Fix bugs and move to PKCE flow.
d856378 to
7a93da7
Compare
| const config = state['features/base/config']; | ||
|
|
||
| // if we have auto redirect enabled, and we have previously logged in successfully | ||
| // let's redirect to the auth url to get the token and login again |
There was a problem hiding this comment.
We don't need this anymore? Why?
There was a problem hiding this comment.
Well this was setting the jwt on prejoin without clicking join. But this means it is invoked without user interaction and that does not work with popups, the browser blocks them. So I moved this to happen before we join, after clicking the join button.
|
|
||
| const nonce = generateNonce(); | ||
|
|
||
| sessionStorage.setItem('oauth_nonce', nonce); |
There was a problem hiding this comment.
do we need this? where do we use it? I see that we remove it but I don't see where we get it?
There was a problem hiding this comment.
This is sessionStorage.setItem( so we set it here. We generate it.
Hum, I think it was used for verifying the response in sso.html. Probably we do not do it at the moment, but we can add it. I will double check when I do the changes for the other PR.
| window.addEventListener('message', handler); | ||
|
|
||
| // Check if popup was closed | ||
| const checkClosed = setInterval(() => { |
There was a problem hiding this comment.
this seems kind of dirty. Maybe we can do a message that will let us know that the popup will close so that we can cleanup or something else?
There was a problem hiding this comment.
We do close it on error or success few lines above, this is the backup plan.
There was a problem hiding this comment.
sure but still. Don't we have another option of handling this?
Also currently if we close the popup from the handler this interval will still run and execute and will call reject....
There was a problem hiding this comment.
Ok I dropped the auto closing logic ... if something goes wrong we need to see it and have the popup open otherwise there is no trace what happened.
6179ba6 to
1cc639a
Compare
|
|
||
| const nonce = generateNonce(); | ||
|
|
||
| sessionStorage.setItem('oauth_nonce', nonce); |
There was a problem hiding this comment.
We should check if the session storage is enabled. If it is disabled trough the browser settings this will throw an exception.
| }; | ||
|
|
||
| // Listen for messages from the popup | ||
| window.addEventListener('message', handler); |
There was a problem hiding this comment.
Maybe we should add the listener to popup. this way if the user closes the popup the listener will be auto removed! otherwise in this use case we will be leaking listeners.
| const state = { | ||
| tenant: string | undefined, | ||
| refreshToken?: string | undefined): object => { | ||
| const state: any = { |
There was a problem hiding this comment.
why the any? If we need to specify a type lets define a real one and use it. It might be useful to have a type for this state anyway...
| tenant: string | undefined): Promise<string | undefined> => { | ||
| tenant: string | undefined, | ||
| // eslint-disable-next-line max-params | ||
| refreshToken?: string | undefined): Promise<string | undefined> => { |
There was a problem hiding this comment.
I'm wondering since we have ? do we need the undefined type?
| const refreshToken = state['features/base/jwt'].refreshToken; | ||
|
|
||
| if (typeof APP !== 'undefined' && jwt | ||
| && validateJwt(jwt).find((e: any) => e.key === JWT_VALIDATION_ERRORS.TOKEN_EXPIRED)) { |
There was a problem hiding this comment.
do we need to do this since from the error we know the jwt is expired?
| .then((url: string | undefined) => { | ||
| if (url) { | ||
| // only if it is inline token auth and token is about to expire | ||
| // if not expired yet use it to refresh the token |
There was a problem hiding this comment.
I'm confused from this comment. Isn't the token expired for sure in this case?
There was a problem hiding this comment.
Yeah the other check was checking 30 seconds earlier ... but in the ljm I did it exact check. Will fix the comment.
| if (url) { | ||
| // only if it is inline token auth and token is about to expire | ||
| // if not expired yet use it to refresh the token | ||
| dispatch(showNotification({ |
There was a problem hiding this comment.
I'm wondering about the use case where the jwt is passed externally from the URL.
It seems to me that even in this case the user will be prompted to login again which would be wrong I guess because the embedder of the meeting would expect to work only with their jwt token. Is this correct or I'm missing something? If this is the case maybe we should show different error message in this case and have a config that would disable the whole login thingy. WDYT?
There was a problem hiding this comment.
Yeah, you are right. I will check it as jaas case is different than this ...
| store.dispatch(setDelayedLoadOfAvatarUrl()); | ||
| store.dispatch(setKnownAvatarUrl(delayedLoadOfAvatarUrl)); | ||
| } | ||
| break; |
There was a problem hiding this comment.
interesting. was this an old bug?
There was a problem hiding this comment.
Yes, but it was not cause any problem because of the first if in setJwt functinon.
| store.dispatch(hideNotification(PROMPT_LOGIN_NOTIFICATION_ID)); | ||
|
|
||
| if (isTokenAuthInline(state['features/base/config'])) { | ||
| // Use refresh token if available, otherwise fall back to silent login |
There was a problem hiding this comment.
Can you elaborate on the comment? I don't understand it.
It seems to me that we always open a popup. What is the silent login?
There was a problem hiding this comment.
Will take a look and fix it. I experimented with several options, there is this silent login that is supported from keycloak but modern browsers stop it most of the times, due to security concerns.
|
|
||
| dispatch(authStatusChanged(true, email || '')); | ||
| // it may happen to be already set (silent login) | ||
| dispatch(authStatusChanged(true, email || getState()['features/base/conference'].authLogin || '')); |
There was a problem hiding this comment.
I'm kind of lost what is the silent login?
There was a problem hiding this comment.
See the comment above, I will go through the comments and fix them all.
| }); | ||
| } | ||
|
|
||
| store.dispatch(authStatusChanged(true, jwtPayload.email)); |
There was a problem hiding this comment.
why do we started setting the email?
There was a problem hiding this comment.
So we can show it in settings -> profile which is the indication that you are logged in.
If email is missing from the state of authentication feature (if I remember correctly) it means you are not logged in.
There was a problem hiding this comment.
hmm I thought it is the first argument which we set to true here, isn't it?
There was a problem hiding this comment.
Nope ... it has some other semantics ... and some comment that we will remove it ...
There was a problem hiding this comment.
Actually you are correct! but why did we started dispatching this one here?
I see that we also do the same on connection established. do we need both?
There was a problem hiding this comment.
do we have the email from jwt on connection established ... if we are going from anonymous (no token) to setting a token we need it here I think.
| joinConference(); | ||
| // if we have auto redirect enabled, and we have previously logged in successfully | ||
| // let's redirect to the auth url to get the token and login again | ||
| if (tokenPreAuthConfig) { |
There was a problem hiding this comment.
actually why did we moved this to be executed on join and not when the user loads the page?
There was a problem hiding this comment.
yes, cause when you join the page there is no user interaction, while clicking join button is user interaction. Otherwise popups do not work and are blocked by the browser.
| <script> | ||
| // Notify parent window that logout is complete | ||
| if (window.parent) { | ||
| window.parent.postMessage({ |
| <div id="error" class="error"></div> | ||
| </div> | ||
|
|
||
| <script> |
There was a problem hiding this comment.
This seems like pretty large JS. Does it make sense to put it in a separate file? Maybe even a new bundle to be sure about browser compatibility and maybe we will be able to reuse some code? WDYT?
| window.opener.postMessage(message, window.location.origin); | ||
|
|
||
| // Auto-close after a short delay | ||
| setTimeout(() => window.close(), 1000); |
There was a problem hiding this comment.
isn't the opener already closing the popup?
| } | ||
|
|
||
| // Get authorization code | ||
| const code = urlParams.get('code'); |
There was a problem hiding this comment.
where is this coming from?

No description provided.