boostorg / website-v2

New Boost website
https://boost.io
Boost Software License 1.0
8 stars 13 forks source link

Social Auth - multiple domains #975

Closed sdarwin closed 6 months ago

sdarwin commented 8 months ago

GitHub OAuth Apps at https://github.com/settings/developers only support one Authorization callback URL. When configuring social applications at https://www.preview.boost.org/admin/socialaccount/socialapp/ one GitHub Provider would be added there. That causes various difficulties. If hosting the website on multiple domains, not all of the domains can "Login with Github". Another issue is that after syncing the production database to staging, it will fail to "Login with Github" until the social application has been modified.

Proposed idea:

What if multiple social apps are added. One for each domain. In django-allauth, a selection is made based on the existing "name" field of the social app in the database. Let's say the format will be "GH www.example.com" or "GitHub OAuth Provider www.neverssl.com". Split the string on whitespace. The last element after splitting is the domain "www.example.com". If a visitor's http request domain name matches the social auth domain name, it's a match. That's the auth to use. If there is no match... auth will likely fail anyway in the case of multiple domains. Then, choose any "GitHub" social app that is present, especially if there is only one.

This only applies to GitHub, because Google supports multiple domains already. The Google button, and social application, doesn't need to be modified.

I opened an issue 3640 at django-allauth about the topic. If we could solve the issue, send them a pull request.

sdarwin commented 8 months ago

Or, if modifying django-allauth anyway, instead of leveraging the "name" field, add a new optional field "web url". If that's set, it will be used in the calculation.

sdarwin commented 7 months ago

The allauth developer replied with a recommended solution of using the "sites" framework. I have tested this and it seems to work correctly.

Added multiple sites: https://www.boost.io/admin/sites/site/
Added multiple social auth providers: https://www.boost.io/admin/socialaccount/socialapp/

In the original implementation of Django "sites" it was necessary to specify SITE_ID=1 in settings. With newer releases, they have added get_current_site(request) which determines the SITE_ID from the request object. When testing this, a statically coded SITE_ID=1 still takes precedence. That means, in order to use get_current_site(request) to dynamically select between multiple sites in real-time based on request, the SITE_ID should be commented out from the settings file.

However that causes pytest to fail in CI.

@frankwiles could pytest be modified to not cause an error when SITE_ID is missing?

sdarwin commented 6 months ago

More investigation:

Ideally, SITE_ID could be removed from settings and get_current_site(request) will discover the site based on the request URL.

However pytest fails.

Here is the output: https://github.com/boostorg/website-v2/actions/runs/8528465943

For example, the first test which fails is "test_news_create_get[news-news-create-NewsForm]".

That means "news" depends on the SITE.

The goal is to use the "sites" framework with multiple sites, to allow allauth Social Auth to choose a different set of authentication keys, depends on the site. And that part works!

"News" should always show the same content regardless of site_id. News should be agnostic to the site information. In that case, why does it depend on the SITE?

Action items in this issue are:

kennethreitz commented 6 months ago

Took some time to poke at this this morning.

It appears as though SITE_ID is simply required to run the tests. The views in question (news etc) appear to function as intended without relying on the sites framework (in a good way) I believe.

kennethreitz commented 6 months ago

One solution (the easiest) is to add this to settings:

if DEBUG:
    SITE_ID = 1

This sets the SITE_ID to 1 for running the tests.

kennethreitz commented 6 months ago

I believe this is now solved!