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

fix: userinfo /v1/session/userinfo should return authenticated=false if token has expired #6282

Merged
merged 2 commits into from
May 20, 2021

Conversation

alexmt
Copy link
Collaborator

@alexmt alexmt commented May 20, 2021

Closes #5687

PR rollbacks #5897 since it broke re-load feature. Instead it implements another approach that fixes #5687. The /v1/session/userinfo API should return {"authenticated": true} only if the provided token is valid and not expired. So only second commit eea45f1 needs review.

…#5897)"

This reverts commit 1148982.

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
@alexmt
Copy link
Collaborator Author

alexmt commented May 20, 2021

cc @jMarkP . Sorry, during #5897 review I did not realize that UI needs token info from /v1/session/userinfo even if token is expired. This PR fixes issue in a different way: ensures that authenticated field in the /v1/session/userinfo response is false if token is expired (or not valid for some other reason)

@alexmt alexmt requested a review from rbreeze May 20, 2021 18:16
@codecov
Copy link

codecov bot commented May 20, 2021

Codecov Report

Merging #6282 (eea45f1) into master (b2f547e) will decrease coverage by 0.02%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6282      +/-   ##
==========================================
- Coverage   41.10%   41.08%   -0.03%     
==========================================
  Files         151      151              
  Lines       19931    19931              
==========================================
- Hits         8192     8188       -4     
- Misses      10611    10613       +2     
- Partials     1128     1130       +2     
Impacted Files Coverage Δ
server/server.go 54.87% <62.50%> (-0.73%) ⬇️
util/session/sessionmanager.go 70.11% <100.00%> (ø)
util/settings/settings.go 46.75% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2f547e...eea45f1. Read the comment docs.

…if token has expired

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>
@jMarkP
Copy link
Contributor

jMarkP commented May 20, 2021

Ah I see. Apologies for that. I'd come from the assumption that lapsed login === no login. Thanks for the fix!

Copy link
Member

@rbreeze rbreeze left a comment

Choose a reason for hiding this comment

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

LGTM!

@alexmt alexmt merged commit 0e9823e into argoproj:master May 20, 2021
@alexmt alexmt deleted the 5897-readonly-relogin branch May 20, 2021 19:58
@jannfis jannfis added the needs-verification PR requires pre-release verification label Jun 25, 2021
@alexmt alexmt removed the needs-verification PR requires pre-release verification label Aug 13, 2021
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.

'log out' link does not change to 'log in' after token expires
4 participants