Skip to content

add preferred login provider selection for social auth #1537

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.

Sign up for GitHub

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

Open
hk2166 wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

add preferred login provider selection for social auth #1537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 44 additions & 4 deletions app/eventyay/eventyay_common/templates/eventyay_common/auth/login.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,47 @@
</div>
</div>
<div class="form-group buttons">
<button type="button" id="toggle-login" class="btn btn-primary btn-block" data-toggle="collapse" data-target="#login-form">
{% translate "Login with Email" %}
</button>
{% if login_providers %}
{% comment %} First, show the preferred login provider {% endcomment %}
{% for provider, settings in login_providers.items %}
{% if settings.state %}
{% if settings.state and settings.preferred %}
<a href='{% url "plugins:socialauth:social.oauth.login" provider %}{% append_next request.GET.next %}'
data-method="post" class="btn btn-primary btn-block btn-lg" style="font-weight: bold;">
{% with provider|capfirst as provider_capitalized %}
{% blocktranslate %}Login with {{ provider_capitalized }}{% endblocktranslate %}
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The 'blocktranslate' tag requires explicit variable naming when used with the 'with' tag. The correct syntax should be 'blocktranslate with name=provider_capitalized' instead of just 'blocktranslate'. Without the 'with' keyword, Django won't properly interpolate the variable into the translation string.

Copilot uses AI. Check for mistakes.
{% endwith %}
</a>
{% endif %}
Comment on lines 54 to +62
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The preferred provider button is displayed if settings.preferred is True, but there's no check for whether the provider actually has valid credentials configured. If an admin sets a provider as preferred but hasn't configured valid client_id and secret, the button will be displayed but clicking it will likely fail. Consider checking both settings.preferred and that credentials are configured (settings.client_id and settings.secret are non-empty) before displaying the preferred button.

Copilot uses AI. Check for mistakes.
{% endfor %}

{% comment %} Check if there are any other enabled providers {% endcomment %}
{% with has_other_providers=False %}
{% for provider, settings in login_providers.items %}
{% if settings.state and not settings.preferred %}
{% with has_other_providers=True %}{% endwith %}
{% endif %}
{% endfor %}
{% endwith %}

Comment on lines +65 to +73
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The variable name 'settings' shadows the 'settings' parameter from the outer loop, making it impossible to access the provider settings inside the inner loop. This will cause the condition to always evaluate to False since you're checking 'settings.get('preferred', False)' which refers to the inner 'settings' variable (initialized to False) rather than the provider settings dictionary.

The 'with' tag in Django templates is meant to create new context variables, not to update existing ones. This pattern doesn't work as intended. Consider using a custom template tag or filter, or restructuring the logic to avoid this nested loop pattern.

Suggested change
{% comment %} Check if there are any other enabled providers {% endcomment %}
{% with has_other_providers=False %}
{% for provider, settings in login_providers.items %}
{% if settings.state and not settings.preferred %}
{% with has_other_providers=True %}{% endwith %}
{% endif %}
{% endfor %}
{% endwith %}

Copilot uses AI. Check for mistakes.
{% comment %} Show "or use the following:" text if there are other providers or email login {% endcomment %}
{% if login_providers %}
{% for provider, settings in login_providers.items %}
{% if settings.state and settings.preferred %}
<p class="text-center" style="margin: 15px 0 10px 0; color: #666;">
{% translate "or use the following:" %}
</p>
{% endif %}
{% endfor %}
{% endif %}
Comment on lines +75 to +83
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The separator text "or use the following:" will be displayed multiple times if multiple providers somehow have 'preferred' set to True. While this shouldn't happen based on the backend logic, the template doesn't guard against it. Consider restructuring to show the separator only once by checking if any preferred provider exists before the loop, or by using a flag variable to track if the separator has been shown.

Suggested change
{% if login_providers %}
{% for provider, settings in login_providers.items %}
{% if settings.state and settings.preferred %}
<p class="text-center" style="margin: 15px 0 10px 0; color: #666;">
{% translate "or use the following:" %}
</p>
{% endif %}
{% endfor %}
{% endif %}
<p class="text-center" style="margin: 15px 0 10px 0; color: #666;">
{% translate "or use the following:" %}
</p>

Copilot uses AI. Check for mistakes.

{% comment %} Show email login button {% endcomment %}
<button type="button" id="toggle-login" class="btn btn-primary btn-block" data-toggle="collapse" data-target="#login-form">
{% translate "Login with Email" %}
</button>

{% comment %} Then, show other enabled providers {% endcomment %}
{% for provider, settings in login_providers.items %}
{% if settings.state and not settings.preferred %}
<a href='{% url "plugins:socialauth:social.oauth.login" provider %}{% append_next request.GET.next %}'
data-method="post" class="btn btn-primary btn-block">
{% with provider|capfirst as provider_capitalized %}
Expand All @@ -63,6 +98,11 @@
</a>
{% endif %}
{% endfor %}
{% else %}
{% comment %} No social login providers, just show email login {% endcomment %}
<button type="button" id="toggle-login" class="btn btn-primary btn-block" data-toggle="collapse" data-target="#login-form">
{% translate "Login with Email" %}
</button>
{% endif %}
</div>
</form>
Expand Down
18 changes: 17 additions & 1 deletion app/eventyay/eventyay_common/views/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,23 @@ def login(request):
request, form.user_cache, form.cleaned_data.get('keep_logged_in', False)
)
else:
form = LoginForm(backend=backend, request=request)
gs = GlobalSettingsObject()
login_providers = gs.settings.get('login_providers', as_type=dict)

# Check if there's a preferred provider - if so, pre-select "Keep me logged in"
has_preferred_provider = any(
settings.get('preferred', False)
for settings in login_providers.values()
if settings.get('state', False)
Comment on lines +148 to +150
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The variable name 'settings' in the loop parameter shadows the Django 'settings' module that's being accessed on line 148. This makes the code confusing and error-prone. Consider renaming the loop variable to something like 'provider_settings' or 'config' to avoid the name collision and improve clarity.

Suggested change
settings.get('preferred', False)
for settings in login_providers.values()
if settings.get('state', False)
provider_settings.get('preferred', False)
for provider_settings in login_providers.values()
if provider_settings.get('state', False)

Copilot uses AI. Check for mistakes.
) if login_providers else False

# Initialize form with keep_logged_in pre-selected if there's a preferred provider
initial_data = {}
if has_preferred_provider and 'keep_logged_in' in LoginForm(backend=backend, request=request).fields:
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

Creating a LoginForm instance just to check if 'keep_logged_in' exists in the fields is inefficient. The LoginForm class definition shows that 'keep_logged_in' is always present as a field (line 24 of auth.py forms). This check is unnecessary and creates an extra form instance that's immediately discarded. Simply remove the condition or verify the field existence in a more efficient way.

Suggested change
if has_preferred_provider and 'keep_logged_in' in LoginForm(backend=backend, request=request).fields:
if has_preferred_provider:

Copilot uses AI. Check for mistakes.
initial_data['keep_logged_in'] = True

form = LoginForm(backend=backend, request=request, initial=initial_data)

ctx['form'] = form
ctx['can_register'] = settings.EVENTYAY_REGISTRATION
ctx['can_reset'] = settings.EVENTYAY_PASSWORD_RESET
Expand Down
1 change: 1 addition & 0 deletions app/eventyay/plugins/socialauth/schemas/login_providers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ class ProviderConfig(BaseModel):
state: bool = Field(description='State of this providers', default=False)
client_id: str = Field(description='Client ID of this provider', default='')
secret: str = Field(description='Secret of this provider', default='')
preferred: bool = Field(description='Is this the preferred login method', default=False)


class LoginProviders(BaseModel):
Expand Down
183 changes: 100 additions & 83 deletions app/eventyay/plugins/socialauth/templates/socialauth/social_auth_settings.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,89 +4,106 @@
{% load rich_text %}

{% block title %}{% trans "Social login settings" %}{% endblock %}

{% block content %}
<nav id="event-nav" class="header-nav">
<div class="navigation">
<div class="navigation-title">
<h1>{% trans "Social login settings" %}</h1>
</div>
{% include "pretixcontrol/event/component_link.html" %}
</div>
</nav>
<form action="" method="post" class="form-horizontal form-plugins">
{% csrf_token %}
{% if "success" in request.GET %}
<div class="alert alert-success">
{% trans "Your changes have been saved." %}
</div>
{% endif %}
<div class="tabbed-form">
<fieldset>
<legend>{% trans "Social login providers" %}</legend>
<div class="table-responsive">
<table class="table">
{% for provider, settings in login_providers.items %}
<tr class="{% if settings.state %}success{% else %}default{% endif %}">
<td>
{% with provider|capfirst as provider_capitalized %}
<strong>{% trans provider_capitalized %}</strong>
<p>{% blocktrans %}Login with {{ provider_capitalized }}{% endblocktrans %}</p>
{% endwith %}
</td>
<td class="text-right flip" width="20%">
{% if settings.state %}
<button class="btn btn-default btn-block" name="{{ provider }}_login" value="disabled">
{% trans "Disable" %}
</button>
{% else %}
<button class="btn btn-default btn-block" name="{{ provider }}_login" value="enabled">
{% trans "Enable" %}
</button>
{% endif %}
</td>
</tr>
{% if settings.state %}
<tr>
<td colspan="2">
<div class="form-group">
<label class="col-sm-3 control-label">
{% if provider == "mediawiki" %}
{% trans "Client Application Key" %}
{% else %}
{% trans "Client ID" %}
{% endif %}
</label>
<div class="col-sm-9">
<input type="text" class="form-control" name="{{ provider }}_client_id" value="{{ settings.client_id }}">
</div>
</div>
<div class="form-group">
<label class="col-sm-3 control-label">
{% if provider == "mediawiki" %}
{% trans "Client Application Secret" %}
{% else %}
{% trans "Secret" %}
{% endif %}
</label>
<div class="col-sm-9">
<input type="password" class="form-control" name="{{ provider }}_secret" value="{{ settings.secret }}">
</div>
</div>
</td>
</tr>
{% endif %}
{% endfor %}
</table>
<nav id="event-nav" class="header-nav">
<div class="navigation">
<div class="navigation-title">
<h1>{% trans "Social login settings" %}</h1>
</div>
{% include "pretixcontrol/event/component_link.html" %}
</div>
</nav>
<form action="" method="post" class="form-horizontal form-plugins">
{% csrf_token %}
{% if "success" in request.GET %}
<div class="alert alert-success">
{% trans "Your changes have been saved." %}
</div>
{% endif %}
<div class="tabbed-form">
<fieldset>
<legend>{% trans "Social login providers" %}</legend>
<div class="table-responsive">
<table class="table">
{% for provider, settings in login_providers.items %}
<tr class="{% if settings.state %}success{% else %}default{% endif %}">
<td>
{% with provider|capfirst as provider_capitalized %}
<strong>{{ provider_capitalized }}</strong>
<p>{% blocktrans with name=provider_capitalized %}Login with {{ name }}{% endblocktrans %}</p>
{% endwith %}
</td>
<td class="text-right flip" width="20%">
{% if settings.state %}
<button class="btn btn-default btn-block" name="{{ provider }}_login" value="disabled">
{% trans "Disable" %}
</button>
{% else %}
<button class="btn btn-default btn-block" name="{{ provider }}_login" value="enabled">
{% trans "Enable" %}
</button>
{% endif %}
</td>
</tr>
{% if settings.state %}
<tr>
<td colspan="2">
<div class="form-group">
<label class="col-sm-3 control-label">
{% if provider == "mediawiki" %}
{% trans "Client Application Key" %}
{% else %}
{% trans "Client ID" %}
{% endif %}
</label>
<div class="col-sm-9">
<input type="text" class="form-control" name="{{ provider }}_client_id" value="{{ settings.client_id }}"/>
</div>
</div>
<div class="form-group">
<label class="col-sm-3 control-label">
{% if provider == "mediawiki" %}
{% trans "Client Application Secret" %}
{% else %}
{% trans "Secret" %}
{% endif %}
</label>
<div class="col-sm-9">
<input type="password" class="form-control" name="{{ provider }}_secret" value="{{ settings.secret }}"/>
</div>
</div>
<div class="form-group">
<label class="col-sm-3 control-label">
{% trans "Preferred Login Method" %}
</label>
<div class="col-sm-9">
<div class="radio">
<label>
<input type="radio" name="preferred_login_provider" value="{{ provider }}" {% if settings.preferred %}checked{% endif %}/>
{% trans "Make this login the preferred login method" %}
</label>
</div>
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The radio button group for preferred login provider lacks a "None" or "No preferred provider" option. Once a user selects a preferred provider, they cannot deselect it to return to having no preferred provider. Consider adding a radio button option with an empty value or a special "none" value to allow administrators to clear the preferred provider selection.

Suggested change
</div>
</div>
{% if forloop.first %}
<div class="radio">
<label>
<input type="radio" name="preferred_login_provider" value=""/>
{% trans "No preferred login method" %}
</label>
</div>
{% endif %}

Copilot uses AI. Check for mistakes.
<p class="help-block">
{% trans "The preferred login method will be shown first and emphasized in the login page." %}
</p>
</div>
</fieldset>
<fieldset>
{% include "socialauth/social_setup.html" %}
</fieldset>
</div>
<div class="form-group submit-group">
<button type="submit" class="btn btn-primary btn-save" name="save_credentials" value="credentials">
{% trans "Save" %}
</button>
</div>
</form>
</div>
</td>
</tr>
{% endif %}
{% endfor %}
</table>
</div>
</fieldset>
<fieldset>
{% include "socialauth/social_setup.html" %}
</fieldset>
</div>
<div class="form-group submit-group">
<button type="submit" class="btn btn-primary btn-save" name="save_credentials" value="credentials">
{% trans "Save" %}
</button>
</div>
</form>
{% endblock %}
4 changes: 4 additions & 0 deletions app/eventyay/plugins/socialauth/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,12 +161,16 @@ def get_context_data(self, **kwargs):
def post(self, request, *args, **kwargs):
login_providers = self.gs.settings.get('login_providers', as_type=dict)
setting_state = request.POST.get('save_credentials', '').lower()
preferred_provider = request.POST.get('preferred_login_provider', '')

for provider in LoginProviders.model_fields.keys():
if setting_state == self.SettingState.CREDENTIALS:
self.update_credentials(request, provider, login_providers)
else:
self.update_provider_state(request, provider, login_providers)

# Update preferred status
Comment on lines +171 to +172
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The logic for updating the preferred status is inside the provider loop, which means it runs for every provider even though the preferred provider selection is independent of individual provider operations. Consider moving this logic outside the loop to make the code clearer and avoid unnecessary repeated checks. You could collect the preferred provider once and then set all providers' preferred status in a single pass after the main loop.

Suggested change
# Update preferred status
# Update preferred status for all providers in a single pass
for provider in LoginProviders.model_fields.keys():

Copilot uses AI. Check for mistakes.
login_providers[provider]['preferred'] = (provider == preferred_provider)

Comment on lines +171 to 174
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The preferred status update logic runs for all providers regardless of whether they are enabled or have credentials configured. This means that if a user selects a preferred provider and then later disables that provider, the provider will remain marked as preferred=True in the settings even though it's not usable. Consider only setting preferred=True if the provider is also enabled (state=True), or clear the preferred status when a provider is disabled.

Suggested change
# Update preferred status
login_providers[provider]['preferred'] = (provider == preferred_provider)
# Update preferred status only for enabled and configured providers
provider_settings = login_providers.get(provider, {})
is_enabled = bool(provider_settings.get('state'))
has_client_id = bool(provider_settings.get('client_id'))
has_secret = bool(provider_settings.get('secret'))
is_usable = is_enabled and has_client_id and has_secret
login_providers[provider]['preferred'] = (
provider == preferred_provider and is_usable
)

Copilot uses AI. Check for mistakes.
self.gs.settings.set('login_providers', login_providers)
return redirect(self.get_success_url())
Expand Down
12 changes: 12 additions & 0 deletions deployment/docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,17 @@ name: eventyay-next

services:
web:
build:
context: ../app
dockerfile: Dockerfile
image: eventyay/eventyay-next:enext
container_name: eventyay-next-web
command: gunicorn eventyay.config.wsgi:application --bind 0.0.0.0:8000
volumes:
- ./data/static:/home/app/web/eventyay/static.dist
- ./data/data:/data
- ./eventyay.cfg:/etc/eventyay.cfg:ro
- ../app/eventyay:/usr/src/app/eventyay
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

Mounting the local source code directory as a volume ('../app/eventyay:/usr/src/app/eventyay') in production deployment configuration is risky. This pattern is typically used for development hot-reloading but can lead to inconsistent deployments, version mismatches, and security issues in production. The code should be baked into the Docker image at build time rather than mounted at runtime for production deployments.

Copilot uses AI. Check for mistakes.
ports:
- 8000:8000
expose:
Expand All @@ -20,12 +24,16 @@ services:
- redis

websocket:
build:
context: ../app
dockerfile: Dockerfile
image: eventyay/eventyay-next:enext
container_name: eventyay-next-websocket
entrypoint: daphne -b 0.0.0.0 -p 8001 eventyay.config.asgi:application
volumes:
- ./data/data:/data
- ./eventyay.cfg:/etc/eventyay.cfg:ro
- ../app/eventyay:/usr/src/app/eventyay
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

Mounting the local source code directory as a volume ('../app/eventyay:/usr/src/app/eventyay') in production deployment configuration is risky. This pattern is typically used for development hot-reloading but can lead to inconsistent deployments, version mismatches, and security issues in production. The code should be baked into the Docker image at build time rather than mounted at runtime for production deployments.

Copilot uses AI. Check for mistakes.
ports:
- 8001:8001
expose:
Expand All @@ -39,11 +47,15 @@ services:
- redis

worker:
build:
context: ../app
dockerfile: Dockerfile
image: eventyay/eventyay-next:enext
container_name: eventyay-next-worker
entrypoint: celery -A eventyay worker -l info
volumes:
- ./eventyay.cfg:/etc/eventyay.cfg:ro
- ../app/eventyay:/usr/src/app/eventyay
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

Mounting the local source code directory as a volume ('../app/eventyay:/usr/src/app/eventyay') in production deployment configuration is risky. This pattern is typically used for development hot-reloading but can lead to inconsistent deployments, version mismatches, and security issues in production. The code should be baked into the Docker image at build time rather than mounted at runtime for production deployments.

Copilot uses AI. Check for mistakes.
env_file:
- ./.env
environment:
Expand Down
Loading