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

Evict user from auth-cache on reset password #3407

Merged
merged 11 commits into from
May 24, 2022

Conversation

idanovo
Copy link
Contributor

@idanovo idanovo commented May 22, 2022

No description provided.

@idanovo idanovo self-assigned this May 22, 2022
@idanovo idanovo marked this pull request as ready for review May 22, 2022 13:10
Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

This seems like an easy way to resolve many of our issues when changing user passwords. I think it will be OK: after logging in the user gets a JWT and authenticates with it. So the password is never really used again for a significant period of time!

(However other auth information for the user, such as group memberships and groups and their policies, are re-used on each call and must remain cached!)

Co-authored-by: arielshaqed <ariels@treeverse.io>
This is why the simpler solution here might be just to not use auth cache for email-password authentication.
This way, we still won't need to authenticate when a user uses API calls that use access-key and secret for authentication and won't need to do the effort of evicting the user auth from all the auth cache instances.
In case we decide to go with this solution, we have to enable email/password authentication only through the UI.
Otherwise, user might try to use email/password authentication with our Python/Java client and won't have their credentials cached.
Copy link
Contributor

Choose a reason for hiding this comment

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

And why is that so bad?
We already state in the docs that email+password should be used only for the UI.
If the user decides to use them for the API, an extra DB call will happen for every request - I think it will not cause a bottleneck. If it does in the future - we can solve it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Think of a Spark app reading from multiple workers. One user in one org makes a mistake, and all users get rate limited.

A shared auth server will have this problem in trumps, even more so if it fails to protect its DB access.

Co-authored-by: johnnyaug <yoni.augarten@treeverse.io>
@idanovo idanovo added the exclude-changelog PR description should not be included in next release changelog label May 22, 2022
@idanovo idanovo changed the title Evict user from auth-cache on reset password-design Evict user from auth-cache on reset password May 23, 2022
Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

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

Remove the unused cache userEmailCache

@idanovo idanovo requested a review from nopcoder May 23, 2022 14:17
Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

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

lgtm!

@idanovo idanovo merged commit a5bf7d8 into master May 24, 2022
@idanovo idanovo deleted the 3404-evict-user-from-auth-cache-on-reset-password branch May 24, 2022 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude-changelog PR description should not be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants