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

Recalculate authenticated user on complete #651

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

BenRogersNewsome
Copy link

@BenRogersNewsome BenRogersNewsome commented Jan 5, 2022

Proposed changes

Re-calculate whether the user is authenticated after the pipeline runs in the do_complete function and better variable naming to make the do_complete function clearer in light of this change.

In the do_complete function, the pipeline is called as,

user = backend.complete(user=user, *args, **kwargs)

meaning that the user instance that is passed into the pipeline function does not neccessarily equal the instance which is returned from it.

We already check whether the user is authenticated before we run the pipeline, however, we don't check again whether the user which is returned from the pipeline is! We assume that the result obtained at the start of do_complete still stands.

This change alone doesn't make a pratical difference to the default pipeline, however, it does solve bugs that arise when pipeline functions are added which change the user instance. For example, a pipeline function which handles the case in which a user was already logged in pre-oauth, but the user selected a different user on the ssi provider's end (for example in the google oauth flow). To handle this a pipeline function could be written which logs the user out at the start of the pipeline so that two UserSocialAuths don't get associated with the same database user. However, as the do_complete function currently stands this doesn't work, as after the pipeline runs the do_complete function thinks the old user is still logged in (which they aren't)! The fix is to re-calculate whether the user is logged in after the pipeline runs and before we calculate is_authenticated again.

The other change in this PR is just a variable name change. In the do_complete method the user method is re-assigned, baed on the old value of itself, multiple times. It seems like there are actually three variables here: The user which is passed into the function, the user we want to pass into the pipeline, and the user we get out of the pipeline. It happens that we don't care about the first variable after declaring the second one, and don't care about the second variable after declaring the third one, and so re-assinging the user variable does work. However, it does make the code hard to read and understand, so have created three seperate variables out of these as they are different things.

Types of changes

Please check the type of change your PR introduces:

  • Release (new release request)
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (PEP8, lint, formatting, renaming, etc)
  • Refactoring (no functional changes, no api changes)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Build related changes (build process, tests runner, etc)
  • Other (please describe):

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask. We're here to
help! This is simply a reminder of what we are going to look for before merging
your code.

  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works

Other information

Any other information that is important to this PR such as screenshots of how
the component looks before and after the change.

Ben Rogers-Newsome added 4 commits January 5, 2022 17:52
We check if the user is authenticated before the pipeline runs but the structure of the pipeline allows for the user instance to change as the pipeline runs. We don't re-check whether the user is still authenticated before working out whether or not to log them in!
The user variable was being re-assigned based on itself multiple times. In reality there are 3 variables here: The user we pass to the function, the user we pass into the pipeline and the user we get back out of the pipeline. These are 3 separate variables (it just so happens that we don't care about the old one once we create the new one). Separating the variables out into three makes it much clearer what is going on with the pipeline.

Made a similar name change for the added user_authenticated variable.
All tests were assuming that the user was authenticated, which caused
tests to fail now that we are actually checking that the user is authenticated
post-pipeline (a none user is obviously not authenticated so pre-pipeline this
wasn't causing problems).

Mirrored the way that is_active is done.
@BenRogersNewsome BenRogersNewsome force-pushed the recalculate-authenticated-user-on-complete branch from 43340f0 to 928431d Compare January 5, 2022 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant