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

Abdul/id token fix #5395

Merged
merged 4 commits into from
Aug 6, 2021
Merged

Abdul/id token fix #5395

merged 4 commits into from
Aug 6, 2021

Conversation

iamazzeez
Copy link
Contributor

@iamazzeez iamazzeez commented Jul 29, 2021

🔩 Description: What code changed, and why?

⛓️ Related Resources

Description

  • Expected behaviour of automate is to call /refresh endpoint every minute once user logs in
  • But due to id_token error in console which occurs intermittently after login, it stops /refresh call to session-service
  • But after login manual refresh of automate ui page /refresh call starts working it calls every minute

What is fixed

  • id_token error in console after login is removed
  • /refresh call is being called every minute after login without manual page refresh
  • Logged in multiple times /refresh call is getting called every minute.

👟 How to Build and Test the Change

  • pull changes from abdul/id_token_fix
  • rebuild components/automate-ui
  • check for /refresh call being called every minute in networks tab, after login via, Local user or SAML or LDAP

✅ Checklist

All PRs must tick these:

With occasional exceptions, all PRs from Progress employees must tick these:

  • Is the code clear? (complicated code or lots of comments--subdivide and use well-named methods, meaningful variable names, etc.)
  • Consistency checked? (user notifications, user prompts, visual patterns, code patterns, variable names)
  • Repeated code blocks eliminated? (adapt and reuse existing components, blocks, functions, etc.)
  • Spelling, grammar, typos checked? (at a minimum use make spell in any component directory)
  • Code well-formatted? (indents, line breaks, etc. improve rather than hinder readability)

All PRs from Progress employees should tick these if appropriate:

  • Tests added/updated? (all new code needs new tests)
  • Docs added/updated? (all customer-facing changes)

Please add a note next to any checkbox above if you are NOT ticking it.

📷 Screenshots, if applicable

Signed-off-by: Abdul-Az <aazeez@progress.com>
Signed-off-by: Abdul-Az <aazeez@progress.com>
Signed-off-by: Abdul-Az <aazeez@progress.com>
@netlify
Copy link

netlify bot commented Jul 29, 2021

✔️ Deploy Preview for chef-automate ready!

🔨 Explore the source changes: 0ecb940

🔍 Inspect the deploy log: https://app.netlify.com/sites/chef-automate/deploys/610a6636065b7b00079b9ce8

😎 Browse the preview: https://deploy-preview-5395--chef-automate.netlify.app

@iamazzeez iamazzeez requested a review from vinay033 July 30, 2021 06:48
Signed-off-by: Abdul-Az <aazeez@progress.com>
@sonarcloud
Copy link

sonarcloud bot commented Aug 4, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Collaborator

@kalroy kalroy left a comment

Choose a reason for hiding this comment

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

Tested the changes in Local environment using:

  1. Local Login
  2. SAML login

Also used Chrome throttling to test in slow network:

  1. Fast 3G
  2. Slow 3G

Worked fine.

@iamazzeez
Copy link
Contributor Author

Tested the changes in EC2 environment using:

LDAP login
Also used Chrome throttling to test in slow network:

Fast 3G
Slow 3G
Worked fine.

@kalroy kalroy merged commit 13841df into master Aug 6, 2021
@kalroy kalroy deleted the abdul/id_token_fix branch August 6, 2021 06:22
@iamazzeez
Copy link
Contributor Author

Found an issue in acceptance once after upgrade for the first time,
Refresh call threw error 401 (Unauthorised) and Automate UI says no permission.
This is for Local user login
Used Admin user Credentials to login
This isn’t getting reproduced

Below are the screenshots,

MicrosoftTeams-image (1).pngMicrosoftTeams-image (2).pngMicrosoftTeams-image.png

@trickyearlobe
Copy link
Contributor

trickyearlobe commented Aug 18, 2021

Hi @kalroy, @Abdul-AZ
This blog post says that this bug #5395 is now fixed, but I just updated to 20210813114337 and now I'm getting logged out after exactly 2 mins on SAML sessions with

/accounts/static/_/js/k=gaia.gaiafe_glif.en_GB.Z5NyInQc2Cw.O/am=B8jRwghKPCABAEAeAAAAAAAAAOBoEVAGmKMbPg/d=1/excm=glif_initial_css/rs=ABkqax2Xztl8kgxpRWDDf23OZNeAUqzIiA/m=glifb,identifier_view,unknownerror_view:0 XHR finished loading: GET "https://**REDACTED**.local/apis/iam/v2/projects".

For a local login, I don't get logged out. The API call looks successful

polyfills-es2019.4e0adb070b509b4d648d.js:1 XHR finished loading: GET "https://**REDACTED**.local/apis/iam/v2/projects".

@iamazzeez
Copy link
Contributor Author

iamazzeez commented Aug 19, 2021

Hi @trickyearlobe , As of now SAML users token expiry time is 24hrs user should be logged in for 24hrs, Even after 24hrs session refresh call, which happens every minute will refresh the token, thus user will be logged in indefinitely.

Speaking about your issue, is this happening repeatedly?
Also could you help me with error in console at the time of logout if any?
As i am not able to reproduce this at my end.

@trickyearlobe
Copy link
Contributor

Yes, it's repeatable.
I'll ping you for a screen share.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants