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

Ensure SSO works on admin level #237

Merged
merged 6 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions MANIFEST.in
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ recursive-include src/pretalx/mail/templates *
recursive-include src/pretalx/orga/templates *
recursive-include src/pretalx/schedule/templates *
recursive-include src/pretalx/sso_provider/templates *
recursive-include src/pretalx/eventyay_common/templates *
recursive-exclude src/tests *
recursive-include src *.py

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
{% extends "orga/base.html" %}
{% load i18n %}
{% load rules %}
{% block extra_title %}{% translate "SSO settings" %} :: {% endblock extra_title %}
{% block content %}
<h2>{% translate "SSO client settings" %}</h2>
<form method="post">
{% csrf_token %}
{{ form }}
<div class="submit-group panel">
<span>
{% if sso_provider %}
<a class="btn-outline-danger btn-lg" role="button" href='{% url "orga:admin.sso.delete" %}'>
{% translate "Delete key" %}
</a>
{% endif %}
</span>
<span>
<button type="submit" class="btn-success btn-lg">
<i class="fa fa-check"></i>
{{ phrases.base.save }}
</button>
</span>
</div>
</form>
{% endblock content %}
22 changes: 19 additions & 3 deletions src/pretalx/eventyay_common/views/auth.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import logging

Check failure on line 1 in src/pretalx/eventyay_common/views/auth.py

View workflow job for this annotation

GitHub Actions / black

would reformat
import os

from allauth.socialaccount.models import SocialApp
from django.conf import settings
from django.contrib import messages
from django.contrib.auth import login
from django.shortcuts import redirect
from django.urls import reverse
Expand All @@ -19,9 +21,16 @@


def oauth2_login_view(request, *args, **kwargs):
sso_provider = SocialApp.objects.filter(
provider=settings.EVENTYAY_SSO_PROVIDER
).first()
if not sso_provider:
messages.error(request, "SSO not configured yet, please contact the "
"administrator or come back later.")
return redirect(reverse("orga:login"))
# Create an OAuth2 session using the client ID and redirect URI
oauth2_session = OAuth2Session(
client_id=settings.OAUTH2_PROVIDER["CLIENT_ID"],
client_id=sso_provider.client_id,
redirect_uri=settings.OAUTH2_PROVIDER["REDIRECT_URI"],
scope=settings.OAUTH2_PROVIDER["SCOPE"],
)
Expand All @@ -39,8 +48,15 @@


def oauth2_callback(request):
Copy link

Choose a reason for hiding this comment

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

issue: Add error handling for missing SSO provider

The function should handle the case where no SSO provider exists, possibly redirecting to a configuration page with an appropriate message.

sso_provider = SocialApp.objects.filter(
provider=settings.EVENTYAY_SSO_PROVIDER
).first()
if not sso_provider:
messages.error(request, "SSO not configured yet, please contact the "
"administrator or come back later.")
return redirect(reverse("orga:login"))
oauth2_session = OAuth2Session(
settings.OAUTH2_PROVIDER["CLIENT_ID"],
sso_provider.client_id,
Copy link
Member

Choose a reason for hiding this comment

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

.first() returns nullable object. sso_provider can be None and sso_provider.client_id will raise error.

redirect_uri=settings.OAUTH2_PROVIDER["REDIRECT_URI"],
state=request.session.get("oauth2_state"),
)
Expand All @@ -49,7 +65,7 @@
# Fetch the token using the authorization code
oauth2_session.fetch_token(
settings.OAUTH2_PROVIDER["ACCESS_TOKEN_URL"],
client_secret=settings.OAUTH2_PROVIDER["CLIENT_SECRET"],
client_secret=sso_provider.secret,
authorization_response=request.build_absolute_uri(),
scope=settings.OAUTH2_PROVIDER["SCOPE"],
)
Expand Down
91 changes: 91 additions & 0 deletions src/pretalx/eventyay_common/views/sso.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
from allauth.socialaccount.models import SocialApp
from django.conf import settings
from django.contrib import messages
from django.contrib.sites.models import Site
from django.db import transaction
from django.http import HttpResponseRedirect
from django.shortcuts import redirect
from django.urls import reverse
from django.utils.translation import gettext_lazy as _
from django.views.generic import DetailView

from pretalx.common.text.phrases import phrases
from pretalx.common.views import CreateOrUpdateView
from pretalx.common.views.mixins import ActionConfirmMixin, PermissionRequired
from pretalx.orga.forms.sso_client_form import SSOClientForm


class SSOConfigureView(PermissionRequired, CreateOrUpdateView):
template_name = "eventyay_common/sso/detail.html"
permission_required = "person.is_administrator"
form_class = SSOClientForm
model = SocialApp

def get_object(self):
Copy link

Choose a reason for hiding this comment

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

issue (complexity): Consider caching the get_object result and simplifying form handling to reduce code complexity.

The current implementation has unnecessary complexity that can be simplified while maintaining Django patterns:

  1. Cache the get_object result as a property to avoid multiple database queries:
@property
def sso_provider(self):
    if not hasattr(self, '_sso_provider'):
        self._sso_provider = SocialApp.objects.filter(
            provider=settings.EVENTYAY_SSO_PROVIDER
        ).first()
    return self._sso_provider
  1. Simplify form handling by leveraging Django's built-in functionality:
def form_valid(self, form):
    instance = form.save(commit=False)
    instance.provider = settings.EVENTYAY_SSO_PROVIDER
    instance.name = "Eventyay Ticket Provider"
    instance.save()
    instance.sites.add(Site.objects.get(pk=settings.SITE_ID))
    messages.success(self.request, phrases.base.saved)
    return super().form_valid(form)

def get_success_url(self):
    return self.request.path

This removes the need for a separate form_invalid method since the parent class already handles it appropriately. The property caching pattern reduces database queries and improves code organization while maintaining Django conventions.

"""
Get the SocialApp instance for the 'eventyay' provider if it exists.
If not, return None to create a new instance.
Note: "eventyay" is the provider name for the Eventyay Ticket Provider.
"""
return SocialApp.objects.filter(provider=settings.EVENTYAY_SSO_PROVIDER).first()

def get_success_url(self):
messages.success(self.request, phrases.base.saved)
return self.request.path

def form_valid(self, form):
"""
Handle the form submission and save the instance.
"""
instance = form.save(commit=False)
instance.provider = settings.EVENTYAY_SSO_PROVIDER
instance.name = "Eventyay Ticket Provider"
with transaction.atomic():
instance.save()
site = Site.objects.get(pk=settings.SITE_ID)
instance.sites.add(site)
return redirect(self.get_success_url())

def form_invalid(self, form):
"""
Handle invalid form submissions.
"""
messages.error(
self.request,
"There was an error updating the Eventyay Ticket "
"Provider configuration.",
)
return self.render_to_response(self.get_context_data(form=form))

def get_context_data(self, **kwargs):
"""
Add additional context to the template if necessary.
"""
context = super().get_context_data(**kwargs)
context["sso_provider"] = self.get_object()
return context


class SSODeleteView(PermissionRequired, ActionConfirmMixin, DetailView):
permission_required = "person.is_administrator"
model = SocialApp
action_text = (
_("You will not able to login with eventyay-tickets account.")
+ " "
+ phrases.base.delete_warning
)

def get_object(self, queryset=None):
return SocialApp.objects.filter(provider=settings.EVENTYAY_SSO_PROVIDER).first()

def action_object_name(self):
return _("SSO Provider") + f": {self.get_object().name}"

@property
def action_back_url(self):
return reverse("orga:admin.sso.settings")

def post(self, *args, **kwargs):
sso_provider = self.get_object()
sso_provider.delete()
return HttpResponseRedirect(reverse("orga:admin.sso.settings"))
14 changes: 2 additions & 12 deletions src/pretalx/orga/forms/sso_client_form.py
Original file line number Diff line number Diff line change
@@ -1,22 +1,12 @@
from allauth.socialaccount.models import SocialApp
from django import forms
from django.conf import settings
from django.contrib.sites.models import Site


class SSOClientForm(forms.ModelForm):
def __init__(self, provider_id, *args, **kwargs):
social_app = SocialApp.objects.filter(provider=provider_id).first()
kwargs["instance"] = social_app
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.fields["secret"].required = True # Secret is required
self.fields["secret"].required = True

class Meta:
model = SocialApp
fields = ["client_id", "secret"]

def save(self, organiser=None):
self.instance.name = organiser
self.instance.provider = organiser
super().save()
self.instance.sites.add(Site.objects.get(pk=settings.SITE_ID))
3 changes: 3 additions & 0 deletions src/pretalx/orga/templates/orga/base.html
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,9 @@
<a href='{% url "orga:admin.user.list" %}' class="nav-link nav-link-second-level{% if "/orga/admin/users" in request.path_info %} active{% endif %}">
<span>{% translate "Users" %}</span>
</a>
<a href='{% url "orga:admin.sso.settings" %}' class="nav-link nav-link-second-level{% if "/orga/admin/sso/settings" in request.path_info %} active{% endif %}">
<span>{% translate "SSO settings" %}</span>
</a>
</div>
</div>
{% endif %}
Expand Down
11 changes: 0 additions & 11 deletions src/pretalx/orga/templates/orga/organiser/detail.html
Original file line number Diff line number Diff line change
@@ -1,17 +1,12 @@
{% extends "orga/base.html" %}

{% load i18n %}
{% load rules %}

{% block extra_title %}{% translate "Organiser" %} :: {{ request.organiser.name }} :: {% endblock extra_title %}

{% block content %}
<h2>{% translate "Settings" %}</h2>
<form method="post">
{% csrf_token %}

{{ form }}

<div class="submit-group panel">
<span>
{% has_perm "person.is_administrator" request.user request.organiser as can_delete_event %}
Expand All @@ -28,11 +23,5 @@ <h2>{% translate "Settings" %}</h2>
</button>
</span>
</div>
</fieldset>
{% if request.organiser %}
<fieldset>
{% include "orga/organiser/organiser_sso.html" %}
</fieldset>
{% endif %}
</form>
{% endblock content %}
26 changes: 0 additions & 26 deletions src/pretalx/orga/templates/orga/organiser/organiser_sso.html

This file was deleted.

14 changes: 14 additions & 0 deletions src/pretalx/orga/urls.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from django.urls import include, path
from django.views.generic.base import RedirectView

from pretalx.eventyay_common.views import sso
from pretalx.orga.views import (
admin,
auth,
Expand Down Expand Up @@ -39,6 +40,19 @@
name="admin.user.delete",
),
path("admin/users/", admin.AdminUserList.as_view(), name="admin.user.list"),
path(
"admin/sso/",
include(
[
path(
"settings",
sso.SSOConfigureView.as_view(),
name="admin.sso.settings",
),
path("delete", sso.SSODeleteView.as_view(), name="admin.sso.delete"),
]
),
),
path("me", event.UserSettings.as_view(), name="user.view"),
path("me/subuser", person.SubuserView.as_view(), name="user.subuser"),
path(
Expand Down
66 changes: 1 addition & 65 deletions src/pretalx/orga/views/organiser.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import logging

from allauth.socialaccount.models import SocialApp
from django.contrib import messages
from django.db import transaction
from django.db.models import Count, Q
Expand All @@ -16,7 +15,7 @@

from pretalx.common.exceptions import SendMailException
from pretalx.common.text.phrases import phrases
from pretalx.common.views import CreateOrUpdateView, is_form_bound
from pretalx.common.views import CreateOrUpdateView
from pretalx.common.views.mixins import (
ActionConfirmMixin,
Filterable,
Expand All @@ -32,7 +31,6 @@
TeamInvite,
check_access_permissions,
)
from pretalx.orga.forms.sso_client_form import SSOClientForm
from pretalx.person.forms import UserSpeakerFilterForm
from pretalx.person.models import User
from pretalx.submission.models.submission import SubmissionStates
Expand Down Expand Up @@ -319,68 +317,6 @@ def get_success_url(self):
messages.success(self.request, phrases.base.saved)
return self.request.path

@context
@cached_property
def sso_client_form(self):
organiser = self.kwargs.get("organiser", None)
if self.request.POST.get("form") == "remove_sso_client":
bind = is_form_bound(self.request, "remove_sso_client")
else:
bind = is_form_bound(self.request, "sso_client")
return SSOClientForm(
provider_id=organiser,
data=self.request.POST if bind else None,
)

def save_sso_client(self, request, *args, **kwargs):
try:
self.sso_client_form.save(organiser=self.kwargs.get("organiser", None))
except Exception as e:
logger.error(
f"Error saving SSO client for organiser {self.kwargs.get('organiser', None)}: {e}"
)
messages.error(request, _("An error occurred: ") + str(e))
return redirect(self.request.path)
return redirect(self.get_success_url())

def post(self, request, *args, **kwargs):
try:
if self.is_remove_sso_client_request(request):
return self.handle_remove_sso_client(request)
elif self.is_sso_client_request(request):
return self.handle_sso_client(request, *args, **kwargs)
else:
self.set_object()
return super().post(request, *args, **kwargs)
except Exception as e:
messages.error(request, _("An error occurred: ") + str(e))
return redirect(self.request.path)

def is_remove_sso_client_request(self, request):
return (
is_form_bound(self.request, "remove_sso_client")
and request.POST.get("form") == "remove_sso_client"
)

def handle_remove_sso_client(self, request):
provider_id = self.kwargs.get("organiser")
try:
social_app = SocialApp.objects.get(provider=provider_id)
social_app.delete()
except SocialApp.DoesNotExist:
messages.error(request, _("The key does not exist."))
return redirect(self.request.path)

def is_sso_client_request(self, request):
return (
is_form_bound(self.request, "sso_client")
and request.POST.get("form") == "sso_client"
and self.sso_client_form.is_valid()
)

def handle_sso_client(self, request, *args, **kwargs):
return self.save_sso_client(request, *args, **kwargs)


class OrganiserDelete(PermissionRequired, ActionConfirmMixin, DetailView):
permission_required = "person.is_administrator"
Expand Down
Loading
Loading