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 2FA provider registry population on login #10578

Merged
merged 1 commit into from
Aug 8, 2018

Conversation

ChristophWurst
Copy link
Member

If the 2FA provider registry has not been populated yet, we have to make
sure all available providers are loaded and queried on login. Otherwise
previously active 2FA providers aren't detected as enabled.

Supersedes #10574.

@rullzer good job on finding and debugging this. I cleaned up your patch and added a few more tests 🚀

If the 2FA provider registry has not been populated yet, we have to make
sure all available providers are loaded and queried on login. Otherwise
previously active 2FA providers aren't detected as enabled.

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

the naming of fixMissingProviderStates is a bit irritating, but that's nitpicking.

@rullzer rullzer merged commit a3fdec7 into master Aug 8, 2018
@rullzer rullzer deleted the fix/2fa-provider-registry-population branch August 8, 2018 09:12
@ChristophWurst
Copy link
Member Author

the naming of fixMissingProviderStates is a bit irritating, but that's nitpicking.

Suggestions are very welcome!

From https://www.martinfowler.com/bliki/TwoHardThings.html:

There are only two hard things in Computer Science: cache invalidation and naming things.

-- Phil Karlton

@blizzz
Copy link
Member

blizzz commented Aug 8, 2018

Of course, that's true. I could think of some, but nothing that's getting significantly better or more elegant. It's OK and not worth to waste to much time on it.

@rullzer rullzer mentioned this pull request Aug 8, 2018
58 tasks
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.

3 participants