Skip to content

fix(socialaccount): Remove hard-coded redirect URL #3287

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

Merged
merged 1 commit into from
Mar 31, 2023

Conversation

c-w
Copy link
Contributor

@c-w c-w commented Mar 20, 2023

This pull request moves all uses of reverse("socialaccount_connections") into the social account adapter so that they can be consistently overwritten in just one place. There was already a helper method for to format the redirect URL, but it wasn't used in all edge cases.

Besides increasing code consistency, the changes in this pull request also fix using django-allauth in combination with dj-rest-auth in the scenario where we have an existing account for a new social login. Previously, we would experience a crash unless a dummy URL is configured for socialaccount_connections. After the fix in this PR is merged, instead of setting this dummy URL we can now override the redirect URL in the adapter which is much cleaner.

@coveralls
Copy link

Coverage Status

Coverage: 91.767% (-0.007%) from 91.774% when pulling 8b98127 on c-w:remove-hard-coded-redirect into 99b67e8 on pennersr:master.

@@ -116,7 +116,6 @@ def get_connect_redirect_url(self, request, socialaccount):
Returns the default URL to redirect to after successfully
connecting a social account.
"""
assert request.user.is_authenticated
Copy link
Owner

Choose a reason for hiding this comment

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

Question: Why is this assert being removed? Same question for the one below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this PR is merged, get_connect_redirect_url will be used in both branches of the if request.user.is_anonymous conditional so I moved the assertion out of the adapter method and into the call site as otherwise calling the adapter method in the the true branch of that conditional would always raise an exception.

Maybe the above explanation isn't super clear so let me annotate some code to help explain my rationale:

if request.user.is_anonymous:
    # this will raise an exception on `assert request.user.is_authenticated`
    # so we need to remove the assertion from the adapter
    connect_redirect_url = get_adapter(request).get_connect_redirect_url(request, sociallogin.account)
    return HttpResponseRedirect(connect_redirect_url)
#
# ... some code omitted for brevity ...
#
# this will not raise on exception on `assert request.user.is_authenticated`
# so for backwards compatibility we can move the assertion from the adapter
# to just before the call site here
assert request.user.is_authenticated
default_next = get_adapter(request).get_connect_redirect_url(request, sociallogin.account)

@pennersr pennersr merged commit 54d7280 into pennersr:master Mar 31, 2023
@c-w c-w deleted the remove-hard-coded-redirect branch March 31, 2023 13:49
@c-w
Copy link
Contributor Author

c-w commented Mar 31, 2023

Thanks for the merge 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants