From a8a773e737c38ed5ad787d056472ff01f9fea822 Mon Sep 17 00:00:00 2001 From: Ben Rogers-Newsome <> Date: Wed, 5 Jan 2022 17:52:08 +0000 Subject: [PATCH 1/4] Check whether user is still authenticated just after pipeline ends 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! --- social_core/actions.py | 1 + 1 file changed, 1 insertion(+) diff --git a/social_core/actions.py b/social_core/actions.py index eee563d8f..4aa62398c 100644 --- a/social_core/actions.py +++ b/social_core/actions.py @@ -55,6 +55,7 @@ def do_complete(backend, login, user=None, redirect_name='next', if user and not isinstance(user, user_model): return user + is_authenticated = user_is_authenticated(user) if is_authenticated: if not user: url = setting_url(backend, redirect_value, 'LOGIN_REDIRECT_URL') From a65bd7e08da6b010d1cbaf84ce902edf17defc9a Mon Sep 17 00:00:00 2001 From: Ben Rogers-Newsome <> Date: Wed, 5 Jan 2022 17:58:51 +0000 Subject: [PATCH 2/4] Better variable naming for user in complete 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. --- social_core/actions.py | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/social_core/actions.py b/social_core/actions.py index 4aa62398c..0e53c16ef 100644 --- a/social_core/actions.py +++ b/social_core/actions.py @@ -33,16 +33,16 @@ def do_complete(backend, login, user=None, redirect_name='next', *args, **kwargs): data = backend.strategy.request_data() - is_authenticated = user_is_authenticated(user) - user = user if is_authenticated else None + is_authenticated_pre_pipeline = user_is_authenticated(user) + pre_pipeline_user = user if is_authenticated_pre_pipeline else None - partial = partial_pipeline_data(backend, user, *args, **kwargs) + partial = partial_pipeline_data(backend, pre_pipeline_user, *args, **kwargs) if partial: - user = backend.continue_pipeline(partial) + post_pipeline_user = backend.continue_pipeline(partial) # clean partial data after usage backend.strategy.clean_partial_pipeline(partial.token) else: - user = backend.complete(user=user, *args, **kwargs) + post_pipeline_user = backend.complete(user=pre_pipeline_user, *args, **kwargs) # pop redirect value before the session is trashed on login(), but after # the pipeline so that the pipeline can change the redirect if needed @@ -52,23 +52,23 @@ def do_complete(backend, login, user=None, redirect_name='next', # check if the output value is something else than a user and just # return it to the client user_model = backend.strategy.storage.user.user_model() - if user and not isinstance(user, user_model): - return user + if post_pipeline_user and not isinstance(post_pipeline_user, user_model): + return post_pipeline_user - is_authenticated = user_is_authenticated(user) - if is_authenticated: - if not user: + is_authenticated_post_pipeline = user_is_authenticated(post_pipeline_user) + if is_authenticated_post_pipeline: + if not post_pipeline_user: url = setting_url(backend, redirect_value, 'LOGIN_REDIRECT_URL') else: url = setting_url(backend, redirect_value, 'NEW_ASSOCIATION_REDIRECT_URL', 'LOGIN_REDIRECT_URL') - elif user: - if user_is_active(user): + elif post_pipeline_user: + if user_is_active(post_pipeline_user): # catch is_new/social_user in case login() resets the instance - is_new = getattr(user, 'is_new', False) - social_user = user.social_user - login(backend, user, social_user) + is_new = getattr(post_pipeline_user, 'is_new', False) + social_user = post_pipeline_user.social_user + login(backend, post_pipeline_user, social_user) # store last login backend name in session backend.strategy.session_set('social_auth_last_login_backend', social_user.provider) @@ -83,8 +83,8 @@ def do_complete(backend, login, user=None, redirect_name='next', 'LOGIN_REDIRECT_URL') else: if backend.setting('INACTIVE_USER_LOGIN', False): - social_user = user.social_user - login(backend, user, social_user) + social_user = post_pipeline_user.social_user + login(backend, post_pipeline_user, social_user) url = setting_url(backend, 'INACTIVE_USER_URL', 'LOGIN_ERROR_URL', 'LOGIN_URL') else: From 1c072dbf0349860f75336d8f182cb395e84c5510 Mon Sep 17 00:00:00 2001 From: Ben Rogers-Newsome <> Date: Wed, 5 Jan 2022 18:26:14 +0000 Subject: [PATCH 3/4] Updated CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4dd4f8669..7cdfe918b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Changed - Fixed Slack user identity API call with Bearer headers - Fixed microsoft-graph login error +- Re-calculate whether the user is still authenticated after the pipeline in the `do_complete` function. ## [4.1.0](https://github.com/python-social-auth/social-core/releases/tag/4.1.0) - 2021-03-01 From 928431d7ac6bfded76951912e5051e72f3974db3 Mon Sep 17 00:00:00 2001 From: Ben Rogers-Newsome <> Date: Wed, 5 Jan 2022 18:53:48 +0000 Subject: [PATCH 4/4] Updated tests 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. --- social_core/tests/actions/test_login.py | 8 ++++++++ social_core/tests/models.py | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/social_core/tests/actions/test_login.py b/social_core/tests/actions/test_login.py index 869250191..3caa4477c 100644 --- a/social_core/tests/actions/test_login.py +++ b/social_core/tests/actions/test_login.py @@ -4,6 +4,14 @@ class LoginActionTest(BaseActionTest): + def setUp(self): + super().setUp() + User.set_authenticated(False) + + def tearDown(self): + super().tearDown() + User.set_authenticated(True) + def test_login(self): self.do_login() diff --git a/social_core/tests/models.py b/social_core/tests/models.py index f49a74656..7a30995a2 100644 --- a/social_core/tests/models.py +++ b/social_core/tests/models.py @@ -23,6 +23,7 @@ class User(BaseModel): NEXT_ID = 1 cache = {} _is_active = True + _is_authenticated = True def __init__(self, username, email=None, **extra_user_fields): self.id = User.next_id() @@ -39,10 +40,17 @@ def __init__(self, username, email=None, **extra_user_fields): def is_active(self): return self._is_active + def is_authenticated(self): + return self._is_authenticated + @classmethod def set_active(cls, is_active=True): cls._is_active = is_active + @classmethod + def set_authenticated(cls, is_authenticated=True): + cls._is_authenticated = is_authenticated + def set_password(self, password): self.password = password