social-core icon indicating copy to clipboard operation
social-core copied to clipboard

Recalculate authenticated user on complete

Open BenRogersNewsome opened this issue 3 years ago • 0 comments

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)
  • [x] Bugfix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [x] 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.

  • [x] 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.

BenRogersNewsome avatar Jan 05 '22 18:01 BenRogersNewsome