Split authenticate into separate overridable function #374
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* split authenticate into separate overridable function * cleaner exception * basic unit test * remove new test * space to revert change
marcelkornblum
changed the title
* split authenticate into separate overridable function * cleaner exception * basic unit test * remove new test * space to revert change * return the user
peppelinux
approved these changes
Apr 28, 2023
|
Please can I ask you to increase the version number here as well? I don't see this as an additional feature, so I'd go for 1.5.7, but feel free to give your suggestions also for this |
|
Absolutely - and yes agree it's not a feature; I've bumped it to 1.5.7 Thanks! |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR splits the user authentication in the AssertionConsumerServiceView into a separate function that can be overridden separately. No functionality has been changed.
We've been using this library for a long time and have multiple IdPs that we connect to. Unfortunately that means we sometimes get duplicate user records created, where the same individual has accounts with more than one IdP. We're changing our flow to incorporate merging records together, but as part of that we need users to authenticate with their IdP for the second record / email address we will merge with. We want this to complete without them automatically then logging in to that account.
The cleanest way we can see to do this would be to override just the user authentication part of the post method of the AssertionConsumerServiceView meaning that we are confident that the user controls the account but we keep the user from their first auth available. This change means that we don't have to work around it with overrides to the backend and the handle_acs_failure methods, which would be possible but inelegant.
This small change would allow our flow to work tidily and be easy to understand. We appreciate any feedback you may have.
/cc @jafacakes2011 @lgarvey