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

Create a boolean flag to enable/disable social logins #305

Closed
maxking opened this issue Feb 12, 2019 · 8 comments
Closed

Create a boolean flag to enable/disable social logins #305

maxking opened this issue Feb 12, 2019 · 8 comments

Comments

@maxking
Copy link
Owner

maxking commented Feb 12, 2019

No description provided.

@gangefors
Copy link
Contributor

gangefors commented Jan 8, 2021

I found something in the source of mailman-web that will help with the configuration of social logins (or the removal of them).

https://gitlab.com/mailman/mailman-web/-/blob/master/mailman_web/settings/__init__.py#L6
https://gitlab.com/mailman/mailman-web/-/blob/master/mailman_web/settings/base.py#L29
https://gitlab.com/mailman/mailman-web/-/blob/master/mailman_web/settings/mailman.py#L93

So if you are not using the latest master you can replicate this behaviour by NOT including the social auth providers in INSTALLED_APPS as you do now.

Then it would be as easy as extending INSTALLED_APPS to enable the social logins needed. Enabling a social login would most likely need additional configuration anyway so appending the providers would be a small task in the big picture. And for the people that don't want social logins there would be no configuration.

Making this change would however break exsisting behaviour so for the people that upgrade there would have to be some way to fall back on adding them by default. Maybe this is where a boolean FLAG comes in? Disabling the default social providers with a env var (e.g. DJANGO_DISABLE_DEFAULT_SOCIAL_AUTH) and then if one wants to add some anyway they can do that with this new pattern (extending INSTALLED_APPS in settings_local.py). It's not as pretty as the pattern provided in the source repo but at least it is backwards compatible.

How does this sound?

I can probably help with a PR if no one else wants to implement it.

@maxking
Copy link
Owner Author

maxking commented Jan 9, 2021

Yeah, I think it is a good idea to separate INSTALLED_APPS and social auth stuff and perhaps use the env var to turn off the social logins, although, it can also be done by setting whatever the new social auth settings we create to an empty list overriding the default set.

I am thinking something along the lines of:

INSTALLED_APPS = [‘...’]

MAILMAN_WEB_SOCIAL_AUTH = [
  ‘facebook’,
  ‘twitter’,
]

try:
    from settings_local import *
except ImportError:
    pass

INSTALLED_APPS = INSTALLED_APPS + MAILMAN_WEB_SOCIAL_AUTH

You can set MAILMAN_WEB_SOCIAL_AUTH to [] to disable the auth. I do see however an issue with people who have overridden INSTALLED_APPS in settings_local.py (like, the existing users) would have issues with upgrade. Not sure how to fix that. Perhaps, we need some way to figure out of INSTALLED_APPS was overridden in settings_local.py,

@gangefors
Copy link
Contributor

gangefors commented Jan 9, 2021

Thanks for the quick response.

Would the following pattern work?
Do you see any potential issues with it?

For old default installs it would have the same list of apps as we have now, i.e. all social providers.
For old installs where INSTALLED_APPS is overridden we would do nothing to that list.
For new installs where one wants to remove all or use a specific set of social providers one just needs to override MAILMAN_WEB_SOCIAL_AUTH in settings_local.py.

INSTALLED_APPS = []
DEFAULT_APPS = [
    # list of default apps, no social login providers
]
MAILMAN_WEB_SOCIAL_AUTH = [
    # list of default social login providers
]

try:
    from settings_local import *
except ImportError:
    pass

if not INSTALLED_APPS:
    INSTALLED_APPS = DEFAULT_APPS + MAILMAN_WEB_SOCIAL_AUTH

@maxking
Copy link
Owner Author

maxking commented Jan 11, 2021

Yeah, that's what I was thinking too. Do you want to open a Pull Request with the changes?

Please also document the new MAILMAN_WEB_SOCIAL_AUTH env variable so people upgrading can read the release notes and change accordingly. It would be good to point out that older version of the docs recomended overriding INSTALLED_APPS to configure social apps, but it not the best way to achieve the right result if in future there are changes to the "required" applications, it won't be picked up.

@danil-smirnov
Copy link
Contributor

Hey guys, there is a Yahoo button that appears by default in the social login tab as well. As I see in the code it comes from the SOCIALACCOUNT_PROVIDERS dict rather than from the INSTALLED_APPS list.

It would be cool to make it disabled with the flag as well. If not possible I'd prefer to have it commented in the code as an example rather than appearing in the UI.

@gangefors
Copy link
Contributor

@danil-smirnov Aren't SOCIALACCOUNT_PROVIDERS related to the configuration of the plugins and not to the actual installed plugins?

This is the override that I use and I see no social addons on my login page.

@danil-smirnov
Copy link
Contributor

Ah yes @gangefors you right it will not show if openid plugin is not installed due to the flag off. Thanks

@maxking
Copy link
Owner Author

maxking commented Mar 20, 2021

Closed by #446

@maxking maxking closed this as completed Mar 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants