From 18092ece597cd614de7b00b15b242c669f57e114 Mon Sep 17 00:00:00 2001 From: Mikko Nieminen Date: Wed, 10 Jul 2024 15:23:59 +0200 Subject: [PATCH] update invite handling (wip), update docs (#1367) --- docs/source/app_projectroles_settings.rst | 14 ++++- docs/source/major_changes.rst | 8 +++ .../templates/include/_login_oidc.html | 12 ++-- .../static/projectroles/css/projectroles.css | 3 + .../templates/projectroles/_login_oidc.html | 13 +++-- .../templates/projectroles/login.html | 16 ++---- .../templates/projectroles/user_form.html | 14 ++++- projectroles/tests/test_ui.py | 3 + projectroles/views.py | 56 ++++++++++++------- userprofile/templates/userprofile/detail.html | 2 +- userprofile/views.py | 1 - 11 files changed, 97 insertions(+), 45 deletions(-) diff --git a/docs/source/app_projectroles_settings.rst b/docs/source/app_projectroles_settings.rst index 503272d2..8382bb6e 100644 --- a/docs/source/app_projectroles_settings.rst +++ b/docs/source/app_projectroles_settings.rst @@ -523,7 +523,7 @@ Critical settings which should be provided through environment variables: Endpoint URL for the OIDC provider. The configuration file ``.well-known/openid-configuration`` is expected to be found under this URL. ``SOCIAL_AUTH_OIDC_KEY`` - Key for the OIDC provider. + Your client ID in the OIDC provider. ``SOCIAL_AUTH_OIDC_SECRET`` Secret for the OIDC provider. ``SOCIAL_AUTH_OIDC_USERNAME_KEY`` @@ -538,6 +538,18 @@ provider, add it as ``{PROJECTROLES_TEMPLATE_INCLUDE_PATH}/_login_oidc.html`` (by default: ``your_site/templates/include/_login_oidc.html``). The include will be displayed as an element in the login view. +Below is an example of a custom template. You can e.g. change the content of the +link to the logo of your OIDC provider. Note that the login URL must equal +``{% url 'social:begin' 'oidc' %}?next={{ oidc_redirect_url|default:'/' }}`` to +ensure it works in all views. + +.. code-block:: django + + + OpenID Connect Login + + SAML SSO Configuration (Removed in v1.0) ======================================== diff --git a/docs/source/major_changes.rst b/docs/source/major_changes.rst index ac7d5a9b..a235324d 100644 --- a/docs/source/major_changes.rst +++ b/docs/source/major_changes.rst @@ -277,6 +277,14 @@ Django Settings Changed The ``PROJECTROLES_HIDE_APP_LINKS`` Django setting, which was deprecated in v0.13, has been removed. Use ``PROJECTROLES_HIDE_PROJECT_APPS`` instead. +Login Template Updated +---------------------- + +The default login template ``login.html`` has been updated by adding OpenID +Connect (OIDC) controls and removing SAML controls. If you have overridden the +login template with your own and wish to use OIDC authentication, make sure to +update your template accordingly. + Base Test Classes Renamed ------------------------- diff --git a/example_site/templates/include/_login_oidc.html b/example_site/templates/include/_login_oidc.html index c5f97381..e2bb3b2c 100644 --- a/example_site/templates/include/_login_oidc.html +++ b/example_site/templates/include/_login_oidc.html @@ -1,9 +1,7 @@ {# Place custom OpenIDC login link and/or info into this template #} -
- - OpenID Connect Login - -
+ + OpenID Connect Login + diff --git a/projectroles/static/projectroles/css/projectroles.css b/projectroles/static/projectroles/css/projectroles.css index 1ab500e4..5d14e241 100644 --- a/projectroles/static/projectroles/css/projectroles.css +++ b/projectroles/static/projectroles/css/projectroles.css @@ -645,6 +645,9 @@ span.select2-selection__rendered { /* Misc --------------------------------------------------------------------- */ +hr { + border-color: #dfdfdf; +} img.sodar-navbar-logo { height: 36px; diff --git a/projectroles/templates/projectroles/_login_oidc.html b/projectroles/templates/projectroles/_login_oidc.html index ead3cb82..2c31f8da 100644 --- a/projectroles/templates/projectroles/_login_oidc.html +++ b/projectroles/templates/projectroles/_login_oidc.html @@ -1,9 +1,14 @@ -{# Default OIDC login element shown if no custom template is provided #} +{# Display custom or default OIDC login element #} +{% load projectroles_common_tags %} -
+{% template_exists template_include_path|add:'/_login_oidc.html' as tpl_oidc %} + +{% if tpl_oidc %} + {% include template_include_path|add:'/_login_oidc.html' %} +{% else %} + href="{% url 'social:begin' 'oidc' %}?next={{ oidc_redirect_url|default:'/' }}"> OpenID Connect Login -
+{% endif %} diff --git a/projectroles/templates/projectroles/login.html b/projectroles/templates/projectroles/login.html index f234bd64..62cc4876 100644 --- a/projectroles/templates/projectroles/login.html +++ b/projectroles/templates/projectroles/login.html @@ -7,6 +7,7 @@ {% block title %}Login{% endblock title %} {% block content %} +{% get_django_setting 'PROJECTROLES_TEMPLATE_INCLUDE_PATH' as template_include_path %}
{# Django messages / site app messages #} @@ -26,7 +27,6 @@

Login

- {% autoescape off %} {% get_login_info %} {% endautoescape %} @@ -48,16 +48,12 @@

Login

- {# Path for template includes #} - {% get_django_setting 'PROJECTROLES_TEMPLATE_INCLUDE_PATH' as template_include_path %} - - {# OpenID Connect auth #} + {# OpenID Connect (OIDC) auth #} {% get_django_setting 'ENABLE_OIDC' as enable_oidc %} - {% template_exists template_include_path|add:'/_login_oidc.html' as tpl_oidc %} - {% if enable_oidc and tpl_oidc %} - {% include template_include_path|add:'/_login_oidc.html' %} - {% elif enable_oidc %} - {% include 'projectroles/_login_oidc.html' %} {# Defaut template #} + {% if enable_oidc %} +
+ {% include 'projectroles/_login_oidc.html' %} +
{% endif %} {# Optional template for additional login page HTML #} diff --git a/projectroles/templates/projectroles/user_form.html b/projectroles/templates/projectroles/user_form.html index 9c657fee..96477848 100644 --- a/projectroles/templates/projectroles/user_form.html +++ b/projectroles/templates/projectroles/user_form.html @@ -5,20 +5,32 @@ {% load crispy_forms_filters %} {% block title %} + {% get_django_setting 'ENABLE_OIDC' as enable_oidc %} + {% if not object and enable_oidc %}Login or{% endif %} {% if object %}Update{% else %}Create{% endif %} Local User {% endblock title %} {% block projectroles %} +{% get_django_setting 'ENABLE_OIDC' as enable_oidc %} +{% get_django_setting 'PROJECTROLES_TEMPLATE_INCLUDE_PATH' as template_include_path %}

+ {% if not object and enable_oidc %}Login or{% endif %} {% if object %}Update{% else %}Create{% endif %} Local User

- {# TODO: Add link to OIDC login if new user and OIDC enabled #} + {% if not object and enable_oidc %} +
+

Please log in if you have an existing account.

+ {% url 'projectroles:invite_process_ldap' secret=invite.secret as oidc_redirect_url %} + {% include 'projectroles/_login_oidc.html' %} +
+
+ {% endif %}
{% csrf_token %} {{ form | crispy }} diff --git a/projectroles/tests/test_ui.py b/projectroles/tests/test_ui.py index ad45ae11..1153e1ae 100644 --- a/projectroles/tests/test_ui.py +++ b/projectroles/tests/test_ui.py @@ -2361,6 +2361,9 @@ def test_invite_preview(self): ) +# TODO: Test OIDC element and URL on local invite process view + + class TestRemoteSiteListView(RemoteSiteMixin, UITestBase): """Tests for RemoteSiteListView UI""" diff --git a/projectroles/views.py b/projectroles/views.py index 5a9778b2..cf3a3941 100644 --- a/projectroles/views.py +++ b/projectroles/views.py @@ -105,6 +105,8 @@ ) USER_PROFILE_UPDATE_MSG = 'User profile updated, please log in again.' USER_PROFILE_LDAP_MSG = 'Error: Profile editing not allowed for LDAP users.' +INVITE_TYPE_LDAP = 'LDAP' +INVITE_TYPE_LOCAL_OIDC = 'LOCAL-OIDC' INVITE_LDAP_LOCAL_VIEW_MSG = ( 'Error: Invite was issued for LDAP user, but local invite view ' 'was requested.' @@ -2549,10 +2551,14 @@ def form_valid(self, form): class ProjectInviteProcessMixin(ProjectModifyPluginViewMixin): """Mixin for accepting and processing project invites""" - # TODO: Handle OIDC invite together with local @classmethod def get_invite_type(cls, invite): - """Return invite type ("ldap", "local" or "error")""" + """ + Return invite type: LDAP or local/OIDC. + + :param invite: ProjectInvite object + :return: String + """ # Check if domain is associated with LDAP domain = invite.email.split('@')[1].lower() domain_no_tld = domain.split('.')[0].lower() @@ -2566,10 +2572,10 @@ def get_invite_type(cls, invite): if settings.ENABLE_LDAP and ( domain_no_tld in ldap_domains or domain in alt_domains ): - return 'ldap' - elif settings.PROJECTROLES_ALLOW_LOCAL_USERS: - return 'local' - return 'error' + return INVITE_TYPE_LDAP + elif settings.ENABLE_OIDC or settings.PROJECTROLES_ALLOW_LOCAL_USERS: + return INVITE_TYPE_LOCAL_OIDC + # Return None for no auth for invite user @classmethod def revoke_invite( @@ -2733,17 +2739,17 @@ def get(self, *args, **kwargs): kwargs={'project': invite.project.sodar_uuid}, ) ) - - # TODO: Handle OIDC invite together with local invite_type = self.get_invite_type(invite) - if invite_type == 'ldap': + if invite_type == INVITE_TYPE_LDAP: return redirect( reverse( 'projectroles:invite_process_ldap', kwargs={'secret': kwargs['secret']}, ) ) - elif invite_type == 'local': + # TODO: OIDC is enabled but local invites are disabled, redirect to + # process_logged_in + elif invite_type == INVITE_TYPE_LOCAL_OIDC: return redirect( reverse( 'projectroles:invite_process_local', @@ -2751,6 +2757,7 @@ def get(self, *args, **kwargs): ) ) # Error + # TODO: Add OIDC note here messages.error( self.request, 'Local users are not allowed on this site.' ) @@ -2769,15 +2776,19 @@ def get(self, *args, **kwargs): if not invite: return redirect(reverse('home')) timeline = get_backend_api('timeline_backend') + # TODO: Check for email to match in case of other user + # TODO: Ensure all relevant checks are performed after refactoring + ''' # Check if invite has correct type - if self.get_invite_type(invite) == 'local': + if self.get_invite_type(invite) == INVITE_TYPE_LOCAL_OIDC: messages.error( self.request, - 'Invite was issued for local user, but LDAP invite view was ' - 'requested.', + 'Invite was issued for local/OIDC user, but LDAP invite view ' + 'was requested.', ) return redirect(reverse('home')) - # Check if user already accepted the invite + ''' + # Check if user has already accepted the invite if self.user_role_exists(invite, self.request.user): return redirect( reverse( @@ -2801,26 +2812,29 @@ def get(self, *args, **kwargs): ) -# TODO: Handle OIDC invite together with local, forward to logged-in view once -# logged in class ProjectInviteProcessLocalView(ProjectInviteProcessMixin, FormView): """View to handle accepting a project local invite""" form_class = LocalUserForm template_name = 'projectroles/user_form.html' + def get_context_data(self, **kwargs): + context = super().get_context_data(**kwargs) + context['invite'] = self.get_invite(self.kwargs['secret']) + return context + def get(self, *args, **kwargs): + # TODO: Remove double invite retrieval invite = self.get_invite(self.kwargs['secret']) if not invite: return redirect(reverse('home')) timeline = get_backend_api('timeline_backend') # Check if local users are enabled - # TODO: Also allow OIDC if not settings.PROJECTROLES_ALLOW_LOCAL_USERS: messages.error(self.request, INVITE_LOCAL_NOT_ALLOWED_MSG) return redirect(reverse('home')) # Check invite for correct type - if self.get_invite_type(invite) == 'ldap': + if self.get_invite_type(invite) == INVITE_TYPE_LDAP: messages.error(self.request, INVITE_LDAP_LOCAL_VIEW_MSG) return redirect(reverse('home')) @@ -2857,7 +2871,6 @@ def get(self, *args, **kwargs): messages.error(self.request, INVITE_USER_NOT_EQUAL_MSG) return redirect(reverse('home')) # User exists but is not local - # TODO: This should instead check if LDAP user if not user.is_local(): messages.error(self.request, 'User exists, but is not local.') return redirect(reverse('home')) @@ -2896,6 +2909,9 @@ def form_valid(self, form): return redirect(reverse('home')) timeline = get_backend_api('timeline_backend') + # TODO: Remove redundant checks + # TODO: Redirect to logged in process view + # TODO: Ensure all checks are performed there # Check if local users are allowed if not settings.PROJECTROLES_ALLOW_LOCAL_USERS: messages.error( @@ -2906,7 +2922,7 @@ def form_valid(self, form): return redirect(reverse('home')) # Check invite for correct type - if self.get_invite_type(invite) == 'ldap': + if self.get_invite_type(invite) == INVITE_TYPE_LDAP: messages.error( self.request, 'Invite was issued for LDAP user, but local invite view was ' diff --git a/userprofile/templates/userprofile/detail.html b/userprofile/templates/userprofile/detail.html index 4a701e4e..147fb603 100644 --- a/userprofile/templates/userprofile/detail.html +++ b/userprofile/templates/userprofile/detail.html @@ -43,7 +43,7 @@

{{ request.user.get_full_name }}

Details - {% if local_user %} + {% if request.user.is_local %}