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

Update user info even on local authentication #144

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

denisonbarbosa
Copy link
Member

We used to rely on the cached information if the access token was still valid. Now, we always try to fetch the user information (username, groups, etc) if the session is online, even if we did not refresh the token.

UDENG-4436

We used to rely on the cached information if the access token was still
valid. Now, we always try to fetch the user information (username,
groups, etc) if the session is online, even if we did not refresh the
token.
@denisonbarbosa denisonbarbosa marked this pull request as ready for review September 10, 2024 10:19
@denisonbarbosa denisonbarbosa requested a review from a team as a code owner September 10, 2024 10:19
Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

Ack on the change and golden file change. From what I understand, we are going to fail if we were online (so got a token, but didn’t refresh it), and groups retrieval fails, is this correct? That behaviour makes sense to me.

@denisonbarbosa denisonbarbosa merged commit ee3fa2e into main Sep 10, 2024
4 checks passed
@denisonbarbosa denisonbarbosa deleted the update-userinfo-password-login branch September 10, 2024 11:04
Comment on lines +485 to 488
if !session.isOffline {
authInfo.UserInfo, err = b.fetchUserInfo(ctx, session, &authInfo)
if err != nil {
slog.Error(err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

This has the effect that the we deny authentication when something goes wrong during refresh, even if the local token is still valid. I think it would be better to log the error and proceed with the login in that case.

Copy link
Member Author

@denisonbarbosa denisonbarbosa Sep 10, 2024

Choose a reason for hiding this comment

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

That's intended. If we want to refresh the user information every authentication, we need to fail if something goes wrong. IMO, we should not have "nondeterministic behavior" during the auth process. If we are online and intend on fetching user information from the provider, authentication should be denied if it's not possible to do so.

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