From a7b862de9cc8e5c8b3332036fefe6b4615427aca Mon Sep 17 00:00:00 2001 From: Fabio Ambauen Date: Fri, 17 Jan 2025 17:24:12 +0100 Subject: [PATCH 1/2] fix: do never link users with organisation identities This is the first step to resolve this issue. This fix prevents organisation Identities to be linked with keycloak users. This could potentially lead to a situation, where a user can make a keycloak account with their email address, but then be unable to login to mySAGW, if the Email is already set on an organisation Identity, due to the unique constraint on the email field. --- api/mysagw/oidc_auth/models.py | 8 +++- .../oidc_auth/tests/test_authentication.py | 41 +++++++++++++++++++ 2 files changed, 48 insertions(+), 1 deletion(-) diff --git a/api/mysagw/oidc_auth/models.py b/api/mysagw/oidc_auth/models.py index 82134de5..c83af081 100644 --- a/api/mysagw/oidc_auth/models.py +++ b/api/mysagw/oidc_auth/models.py @@ -3,6 +3,7 @@ from django.conf import settings from django.db import IntegrityError, transaction from django.db.models import Q +from rest_framework.exceptions import ValidationError from mysagw.identity.models import Identity @@ -87,8 +88,13 @@ def _update_or_create_identity(self): settings.OIDC_MONITORING_CLIENT_USERNAME, ]: return None + if Identity.objects.filter( + is_organisation=True, email__iexact=self.email + ).exists(): + msg = "Can't create Identity, because there is already an organisation with this email address." + raise ValidationError(msg) try: - identity = Identity.objects.get( + identity = Identity.objects.filter(is_organisation=False).get( Q(idp_id=self.id) | Q(email__iexact=self.email), ) # we only want to save if necessary in order to prevent adding historical diff --git a/api/mysagw/oidc_auth/tests/test_authentication.py b/api/mysagw/oidc_auth/tests/test_authentication.py index 60669e28..b208644b 100644 --- a/api/mysagw/oidc_auth/tests/test_authentication.py +++ b/api/mysagw/oidc_auth/tests/test_authentication.py @@ -4,10 +4,12 @@ import pytest from django.core.cache import cache +from django.urls import reverse from mozilla_django_oidc.contrib.drf import OIDCAuthentication from requests.exceptions import HTTPError from rest_framework import exceptions, status from rest_framework.exceptions import AuthenticationFailed +from rest_framework.test import APIClient from simple_history.models import HistoricalRecords from mysagw.identity.models import Identity @@ -245,3 +247,42 @@ def test_authentication_idp_missing_claim( request = rf.get("/openid", HTTP_AUTHORIZATION="Bearer Token") with pytest.raises(AuthenticationFailed): OIDCAuthentication().authenticate(request) + + +@pytest.mark.parametrize( + "identity__is_organisation,identity__organisation_name,identity__email", + [ + (True, "org name", "email@example.com"), + ], +) +def test_authentication_email_already_used( + db, rf, requests_mock, settings, get_claims, identity +): + idp_id = str(uuid4()) + claims = get_claims( + id_claim=idp_id, + email_claim="email@example.com", + first_name_claim="Winston", + last_name_claim="Smith", + salutation_claim="neutral", + title_claim=None, + ) + assert Identity.objects.count() == 1 + + requests_mock.get(settings.OIDC_OP_USER_ENDPOINT, text=json.dumps(claims)) + + url = reverse("me") + + client = APIClient() + response = client.get(url, HTTP_AUTHORIZATION="Bearer Token") + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json() == { + "errors": [ + { + "detail": "Can't create Identity, because there is already an organisation with this email address.", + "status": "400", + "source": {"pointer": "/data"}, + "code": "invalid", + } + ] + } From e41cbc7cc220486e86ba119f7f209cee10548891 Mon Sep 17 00:00:00 2001 From: yelinz Date: Thu, 23 Jan 2025 15:00:52 +0100 Subject: [PATCH 2/2] feat(ember): add support page redirect to support page when receiving specific identity error --- ember/app/router.js | 1 + ember/app/services/session.js | 42 ++++++++++++++++++++---------- ember/app/ui/support/route.js | 3 +++ ember/app/ui/support/template.hbs | 10 +++++++ ember/translations/support/de.yaml | 3 +++ ember/translations/support/en.yaml | 3 +++ ember/translations/support/fr.yaml | 3 +++ 7 files changed, 51 insertions(+), 14 deletions(-) create mode 100644 ember/app/ui/support/route.js create mode 100644 ember/app/ui/support/template.hbs create mode 100755 ember/translations/support/de.yaml create mode 100644 ember/translations/support/en.yaml create mode 100755 ember/translations/support/fr.yaml diff --git a/ember/app/router.js b/ember/app/router.js index 04acbf1b..3412e5e3 100644 --- a/ember/app/router.js +++ b/ember/app/router.js @@ -12,6 +12,7 @@ const resetNamespace = true; //eslint-disable-next-line array-callback-return Router.map(function () { this.route("login"); + this.route("support"); this.route("notfound", { path: "/*path" }); this.route("protected", { path: "/" }, function () { diff --git a/ember/app/services/session.js b/ember/app/services/session.js index 20b5887d..cd4f7249 100644 --- a/ember/app/services/session.js +++ b/ember/app/services/session.js @@ -7,6 +7,7 @@ import ENV from "mysagw/config/environment"; export default class CustomSession extends Session { @service store; + @service router; currentIdentity = trackedTask(this, this.fetchCurrentIdentity, () => [ this.isAuthenticated, @@ -18,21 +19,34 @@ export default class CustomSession extends Session { if (!this.isAuthenticated) return null; - const data = yield Promise.all([ - this.store.query( - "membership", - { - include: "organisation", - }, - { adapterOptions: { customEndpoint: "my-memberships" } }, - ), - this.store.queryRecord("identity", {}), - ]); + try { + const data = yield Promise.all([ + this.store.query( + "membership", + { + include: "organisation", + }, + { adapterOptions: { customEndpoint: "my-memberships" } }, + ), + this.store.queryRecord("identity", {}), + ]); + + return { + memberships: data[0], + identity: data[1], + }; + } catch (error) { + this.invalidate(); + + if ( + error.errors[0].detail === + "Can't create Identity, because there is already an organisation with this email address." + ) { + return this.router.transitionTo("support"); + } - return { - memberships: data[0], - identity: data[1], - }; + return this.router.transitionTo("login"); + } } get identity() { diff --git a/ember/app/ui/support/route.js b/ember/app/ui/support/route.js new file mode 100644 index 00000000..40247ebc --- /dev/null +++ b/ember/app/ui/support/route.js @@ -0,0 +1,3 @@ +import Route from "@ember/routing/route"; + +export default class SupportRoute extends Route {} diff --git a/ember/app/ui/support/template.hbs b/ember/app/ui/support/template.hbs new file mode 100644 index 00000000..ed10b789 --- /dev/null +++ b/ember/app/ui/support/template.hbs @@ -0,0 +1,10 @@ +
+

{{t "support.title"}}

+

+ {{t "support.subtitle"}} + + {{! template-lint-disable no-bare-strings }} + sagw@sagw.ch + +

+
\ No newline at end of file diff --git a/ember/translations/support/de.yaml b/ember/translations/support/de.yaml new file mode 100755 index 00000000..51ce8946 --- /dev/null +++ b/ember/translations/support/de.yaml @@ -0,0 +1,3 @@ +support: + title: "Ein Fehler ist aufgetreten." + subtitle: "Bitte wenden Sie sich an" diff --git a/ember/translations/support/en.yaml b/ember/translations/support/en.yaml new file mode 100644 index 00000000..621ef065 --- /dev/null +++ b/ember/translations/support/en.yaml @@ -0,0 +1,3 @@ +support: + title: "An error has occurred." + subtitle: "Please contact" diff --git a/ember/translations/support/fr.yaml b/ember/translations/support/fr.yaml new file mode 100755 index 00000000..d750bca3 --- /dev/null +++ b/ember/translations/support/fr.yaml @@ -0,0 +1,3 @@ +support: + title: "Une erreur s'est produite." + subtitle: "Veuillez contacter"