Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement timeout warning for SSO token timeout #1067

Conversation

timisenco2015
Copy link
Contributor

@timisenco2015 timisenco2015 commented Oct 10, 2023

Description

There has been a reported issue of users suddenly being logged out due to token expiration. This feature will notify the users that their token is about to expire. They can log out or continue using the app and renew their token.
Screenshot 2023-10-10 072426

Types of changes

New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING doc
  • I have checked that unit tests pass locally with my changes
  • I have run the npm script lint on the frontend and backend
  • [] I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have approval from the product owner for the contribution in this pull request

Further comments

@github-actions
Copy link

github-actions bot commented Oct 10, 2023

Coverage Report (Application)

Totals Coverage
Statements: 41.89% ( 2636 / 6292 )
Methods: 36.84% ( 326 / 885 )
Lines: 47.43% ( 1759 / 3709 )
Branches: 32.45% ( 551 / 1698 )

@github-actions
Copy link

github-actions bot commented Oct 10, 2023

Coverage Report (Frontend)

Totals Coverage
Statements: 67.45% ( 1142 / 1693 )
Methods: 64.24% ( 203 / 316 )
Lines: 71.61% ( 802 / 1120 )
Branches: 53.31% ( 137 / 257 )

@github-actions
Copy link

github-actions bot commented Oct 10, 2023

timisenco2015 and others added 4 commits October 20, 2023 07:40
- add a countdown timer to the token expiration warning dialog
- wrote unit tests for the frontend changes
- added multi lang to the new components created
* fix: FORMS-891 vuetify traditional chinese support (#1072)

Co-authored-by: Walter Moar <[email protected]>

* fix: filter out draft submissions on the submission page search

* fix: remove boolean check for filterformSubmissionStatusCode props

* fix: urgent timeout increase to fix outage (#1084)

* Fix/urgent timeout increase (#1085)

* fix: urgent timeout increase to fix outage

* fix: database view performance (#1091)

* fix: simplification of user form access view (#1087)

* fix: database view performance (#1090)

* fix: simplification of user form access view

* fix: remove the sorting in user_form_access_vw

---------

Co-authored-by: Walter Moar <[email protected]>

* fix: FORMS-893 semver vuln and endpoint bug (#1094)

* FORMS-881 - Not allow to save drafts if form validation fails (#1048)

* fix: not allow to save drafts if form validation fails

* fix: make validation run on render for drafts. remove disable save draft button on validation fails

* update: add countdown timer to token expiration warning dialog

- add a countdown timer to the token expiration warning dialog
- wrote unit tests for the frontend changes
- added multi lang to the new components created

* feat: FORMS-882 add submissionId to exports

Co-authored-by: Walter Moar <[email protected]>

* fix: FORMS-899 revert timeout and simplify views (#1093)

* fix: FORMS-899 remove sorting in views

* fix: FORMS-899 revert frontend timeout change

---------

Co-authored-by: Walter Moar <[email protected]>

---------

Co-authored-by: bcgov-citz-ccft <[email protected]>
Co-authored-by: Walter Moar <[email protected]>
Co-authored-by: bcvesalink <[email protected]>
* update: add countdown timer to token expiration warning dialog

- add a countdown timer to the token expiration warning dialog
- wrote unit tests for the frontend changes
- added multi lang to the new components created

* fix: clean up codes
},
data() {
return {
timerDate: moment().add(15, 'minutes'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This 15 minutes value should be based on the token - it could be different in different environments or deployments, and we might wand to change it in Keycloak, which would break it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The token expires every 5 minutes, and the refresh token every 30 minutes. Addition of both the Idle time and timer counter down is 25 minutes. This would allow users to refresh the token if they stay signed in

Copy link
Collaborator

Choose a reason for hiding this comment

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

But if we go into Keycloak and change the token to expire every 60 minutes, then this will be displayed too early. I think all the logic should be based on the timeout value in the token (not exactly sure what that is - maybe the exp?). I'll need to look into how these tokens are being used.

@timisenco2015 timisenco2015 marked this pull request as draft October 25, 2023 23:34
@bcvesalink
Copy link
Contributor

@WalterMoar @timisenco2015 just to clarify and make my suggestion on this topic. Instead of asking user to login again, what if we store the refresh token in DB within the user table and then if any api call (Save form, Get form, etc.) returns 403 status, make keycloak api call to update access_token with stored in DB refresh_token so if it's still valid we would update the access_token in header?

@WalterMoar
Copy link
Collaborator

@WalterMoar @timisenco2015 just to clarify and make my suggestion on this topic. Instead of asking user to login again, what if we store the refresh token in DB within the user table and then if any api call (Save form, Get form, etc.) returns 403 status, make keycloak api call to update access_token with stored in DB refresh_token so if it's still valid we would update the access_token in header?

Interesting! I don't know if the API should be responsible for renewing tokens, though, and I think it would lose an important functionality that we are supposed to have now: 10 hour login sessions. And the refresh token is only valid for 30 minutes, so it wouldn't even get people through a coffee / lunch break, but the current "expected" behaviour should allow being idle all day. Just to clarify:

The behaviour now is that the access token has a lifespan of 5 minutes. Every 10 seconds it's checked to see if it expires in less than 60 seconds, and if so then it (and the refresh token) are refreshed - so the access token gets refreshed every 4 minutes:

    const updateTokenInterval = setInterval(
      () =>
        keycloak.updateToken(60).catch(() => {
          keycloak.clearToken();
        }),
      10000
    );

So the access token should be refreshed and valid from when the user logs in until 10 hours later (the maximum time an authenticated login is valid, as defined in Keycloak). In theory we should never have 403s in these 10 hours - the exception is that for some reason the access token is not being refreshed on this schedule.

In Chrome and Firefox on Windows this seems to work fine, although there might be some other use case I haven't tried yet. I think that for performance Chrome changes the interval for hidden tabs, but only if the interval is very small, so it shouldn't affect us. I'm also wondering if people are using some other browser that maybe has different behaviour for setInterval() when the browser tab is hidden, the browser is minimized, the screen is locked, etc.

@bcvesalink
Copy link
Contributor

@WalterMoar @timisenco2015 just to clarify and make my suggestion on this topic. Instead of asking user to login again, what if we store the refresh token in DB within the user table and then if any api call (Save form, Get form, etc.) returns 403 status, make keycloak api call to update access_token with stored in DB refresh_token so if it's still valid we would update the access_token in header?

Interesting! I don't know if the API should be responsible for renewing tokens, though, and I think it would lose an important functionality that we are supposed to have now: 10 hour login sessions. And the refresh token is only valid for 30 minutes, so it wouldn't even get people through a coffee / lunch break, but the current "expected" behaviour should allow being idle all day. Just to clarify:

The behaviour now is that the access token has a lifespan of 5 minutes. Every 10 seconds it's checked to see if it expires in less than 60 seconds, and if so then it (and the refresh token) are refreshed - so the access token gets refreshed every 4 minutes:

    const updateTokenInterval = setInterval(
      () =>
        keycloak.updateToken(60).catch(() => {
          keycloak.clearToken();
        }),
      10000
    );

So the access token should be refreshed and valid from when the user logs in until 10 hours later (the maximum time an authenticated login is valid, as defined in Keycloak). In theory we should never have 403s in these 10 hours - the exception is that for some reason the access token is not being refreshed on this schedule.

In Chrome and Firefox on Windows this seems to work fine, although there might be some other use case I haven't tried yet. I think that for performance Chrome changes the interval for hidden tabs, but only if the interval is very small, so it shouldn't affect us. I'm also wondering if people are using some other browser that maybe has different behaviour for setInterval() when the browser tab is hidden, the browser is minimized, the screen is locked, etc.

it's not on API side to refresh the token, I meant on middleware side here https://github.com/bcgov/common-hosted-form-service/blob/master/app/src/forms/auth/middleware/userAccess.js#L39 but ok, if we we go for above you specify, should we even ask user to login again (popup window I mean). Can we just update the token without asking user?

@WalterMoar
Copy link
Collaborator

@WalterMoar @timisenco2015 just to clarify and make my suggestion on this topic. Instead of asking user to login again, what if we store the refresh token in DB within the user table and then if any api call (Save form, Get form, etc.) returns 403 status, make keycloak api call to update access_token with stored in DB refresh_token so if it's still valid we would update the access_token in header?

Interesting! I don't know if the API should be responsible for renewing tokens, though, and I think it would lose an important functionality that we are supposed to have now: 10 hour login sessions. And the refresh token is only valid for 30 minutes, so it wouldn't even get people through a coffee / lunch break, but the current "expected" behaviour should allow being idle all day. Just to clarify:
The behaviour now is that the access token has a lifespan of 5 minutes. Every 10 seconds it's checked to see if it expires in less than 60 seconds, and if so then it (and the refresh token) are refreshed - so the access token gets refreshed every 4 minutes:

    const updateTokenInterval = setInterval(
      () =>
        keycloak.updateToken(60).catch(() => {
          keycloak.clearToken();
        }),
      10000
    );

So the access token should be refreshed and valid from when the user logs in until 10 hours later (the maximum time an authenticated login is valid, as defined in Keycloak). In theory we should never have 403s in these 10 hours - the exception is that for some reason the access token is not being refreshed on this schedule.
In Chrome and Firefox on Windows this seems to work fine, although there might be some other use case I haven't tried yet. I think that for performance Chrome changes the interval for hidden tabs, but only if the interval is very small, so it shouldn't affect us. I'm also wondering if people are using some other browser that maybe has different behaviour for setInterval() when the browser tab is hidden, the browser is minimized, the screen is locked, etc.

it's not on API side to refresh the token, I meant on middleware side here https://github.com/bcgov/common-hosted-form-service/blob/master/app/src/forms/auth/middleware/userAccess.js#L39 but ok, if we we go for above you specify, should we even ask user to login again (popup window I mean). Can we just update the token without asking user?

Yes - this is exactly the thing - in the frontend we should already be automatically updating the token before it expires, and should continue to do so for 10 hours. In Confluence I asked the JEDI people if perhaps they are hitting the 10 hour limit. In that case, they will definitely have an expired token because it can no longer be renewed, and a new login will be needed.

I think that point in userAccess.js would be perfect for adding some logging about why the token is invalid. That might help us to diagnose the problem.

@WalterMoar
Copy link
Collaborator

Closing this since the problem the users are having does not seem to be due to timeouts.

@WalterMoar WalterMoar closed this Dec 7, 2023
@WalterMoar WalterMoar deleted the Forms-890_on-screen_indication_for_the_SSO_timer_value branch December 7, 2023 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants