From 79a96fb831003fde372395d6657fed26eb0cdedf Mon Sep 17 00:00:00 2001 From: Nathan Levesque Date: Wed, 2 Oct 2024 22:02:32 -0400 Subject: [PATCH] Fixed issue with full name not being pulled in --- authentication/backends/ol_open_id_connect.py | 14 ++ authentication/hooks.py | 2 +- authentication/pipeline/user.py | 7 +- authentication/pipeline/user_test.py | 1 + learning_resources/plugins.py | 3 +- learning_resources/plugins_test.py | 2 +- main/settings.py | 8 +- profiles/adapters.py | 218 ------------------ profiles/plugins.py | 10 +- profiles/scim/__init__.py | 0 profiles/scim/adapters.py | 210 +++++++++++++++++ profiles/scim/filters.py | 17 ++ profiles/scim/views_test.py | 74 ++++++ 13 files changed, 337 insertions(+), 229 deletions(-) delete mode 100644 profiles/adapters.py create mode 100644 profiles/scim/__init__.py create mode 100644 profiles/scim/adapters.py create mode 100644 profiles/scim/filters.py create mode 100644 profiles/scim/views_test.py diff --git a/authentication/backends/ol_open_id_connect.py b/authentication/backends/ol_open_id_connect.py index e2f406b54d..cbfe4a8dbb 100644 --- a/authentication/backends/ol_open_id_connect.py +++ b/authentication/backends/ol_open_id_connect.py @@ -10,3 +10,17 @@ class OlOpenIdConnectAuth(OpenIdConnectAuth): """ name = "ol-oidc" + + def get_user_details(self, response): + """Get the user details from the API response""" + details = super().get_user_details(response) + + return { + **details, + "profile": { + "name": response.get("name", ""), + "email_optin": bool(int(response["email_optin"])) + if "email_optin" in response + else None, + }, + } diff --git a/authentication/hooks.py b/authentication/hooks.py index 250e8c9261..c1b596214e 100644 --- a/authentication/hooks.py +++ b/authentication/hooks.py @@ -17,7 +17,7 @@ class AuthenticationHooks: """Pluggy hooks specs for authentication""" @hookspec - def user_created(self, user): + def user_created(self, user, user_data): """Trigger actions after a user is created""" diff --git a/authentication/pipeline/user.py b/authentication/pipeline/user.py index 3ec04bb25d..b200c5716f 100644 --- a/authentication/pipeline/user.py +++ b/authentication/pipeline/user.py @@ -3,6 +3,7 @@ from social_core.exceptions import AuthException from authentication.hooks import get_plugin_manager +from profiles import api as profile_api def forbid_hijack( @@ -23,14 +24,16 @@ def forbid_hijack( return {} -def user_created_actions(**kwargs): +def user_created_actions(*, user, details, **kwargs): """ Trigger plugins when a user is created """ if kwargs.get("is_new"): pm = get_plugin_manager() hook = pm.hook - hook.user_created(user=kwargs["user"]) + hook.user_created(user=user, user_data=details) + else: + profile_api.ensure_profile(user=user, profile_data=details.get("profile", {})) def user_onboarding(*, backend, **kwargs): diff --git a/authentication/pipeline/user_test.py b/authentication/pipeline/user_test.py index b6893ad90b..a88a1d6ea4 100644 --- a/authentication/pipeline/user_test.py +++ b/authentication/pipeline/user_test.py @@ -38,6 +38,7 @@ def test_user_created_actions(mocker, is_new): kwargs = { "user": user, "is_new": is_new, + "details": {}, } user_actions.user_created_actions(**kwargs) diff --git a/learning_resources/plugins.py b/learning_resources/plugins.py index 229ab4a4ba..f4637aa1ba 100644 --- a/learning_resources/plugins.py +++ b/learning_resources/plugins.py @@ -10,12 +10,13 @@ class FavoritesListPlugin: hookimpl = apps.get_app_config("authentication").hookimpl @hookimpl - def user_created(self, user): + def user_created(self, user, user_data): # noqa: ARG002 """ Perform functions on a newly created user Args: user(User): The user to create the list for + user_data(dict): the user data """ UserList.objects.get_or_create( author=user, title=FAVORITES_TITLE, defaults={"description": "My Favorites"} diff --git a/learning_resources/plugins_test.py b/learning_resources/plugins_test.py index 904ba0367c..9efa1747dc 100644 --- a/learning_resources/plugins_test.py +++ b/learning_resources/plugins_test.py @@ -17,6 +17,6 @@ def test_favorites_plugin_user_created(existing_list): UserListFactory.create( author=user, title=FAVORITES_TITLE, description="My Favorites" ) - FavoritesListPlugin().user_created(user) + FavoritesListPlugin().user_created(user, user_data={}) user.refresh_from_db() assert user.user_lists.count() == 1 diff --git a/main/settings.py b/main/settings.py index 3c56978486..fc1d1bffcb 100644 --- a/main/settings.py +++ b/main/settings.py @@ -136,8 +136,9 @@ "documentationUri": "", }, ], - "USER_ADAPTER": "profiles.adapters.SCIMProfile", - "USER_MODEL_GETTER": "profiles.adapters.get_user_model_for_scim", + "USER_ADAPTER": "profiles.scim.adapters.LearnSCIMUser", + "USER_MODEL_GETTER": "profiles.scim.adapters.get_user_model_for_scim", + "USER_FILTER_PARSER": "profiles.scim.filters.LearnUserFilterQuery", } @@ -297,6 +298,9 @@ ), urlparse(APP_BASE_URL).netloc, ] +SOCIAL_AUTH_PROTECTED_USER_FIELDS = [ + "profile", # this avoids an error because profile is a related model +] SOCIAL_AUTH_PIPELINE = ( # Checks if an admin user attempts to login/register while hijacking another user. diff --git a/profiles/adapters.py b/profiles/adapters.py deleted file mode 100644 index 17bccbe2c3..0000000000 --- a/profiles/adapters.py +++ /dev/null @@ -1,218 +0,0 @@ -import copy -import logging - -from django.contrib.auth import get_user_model -from django.db import transaction -from django_scim import constants -from django_scim import exceptions as scim_exceptions -from django_scim.adapters import SCIMUser - -from profiles.models import Profile - -User = get_user_model() - - -logger = logging.getLogger(__name__) - - -def get_user_model_for_scim(): - """ - Get function for the django_scim library configuration (USER_MODEL_GETTER). - - Returns: - model: Profile model. - """ - return Profile - - -class SCIMProfile(SCIMUser): - """ - Custom adapter to extend django_scim library. This is required in order - to extend the profiles.models.Profile model to work with the - django_scim library. - """ - - password_changed = False - activity_changed = False - - resource_type = "User" - - def __init__(self, obj, request=None): - super().__init__(obj, request) - self._from_dict_copy = None - - @property - def is_new_user(self): - """_summary_ - - Returns: - bool: True is the user does not currently exist, - False if the user already exists. - """ - return not bool(self.obj.id) - - @property - def emails(self): - """ - Return the email of the user per the SCIM spec. - """ - return [{"value": self.obj.user.email, "primary": True}] - - @property - def display_name(self): - """ - Return the displayName of the user per the SCIM spec. - """ - if self.obj.first_name and self.obj.last_name: - return f"{self.obj.first_name} {self.obj.last_name}" - return self.obj.user.email - - @property - def meta(self): - """ - Return the meta object of the user per the SCIM spec. - """ - return { - "resourceType": self.resource_type, - "created": self.obj.user.date_joined.isoformat(timespec="milliseconds"), - "lastModified": self.obj.updated_at.isoformat(timespec="milliseconds"), - "location": self.location, - } - - def to_dict(self): - """ - Return a ``dict`` conforming to the SCIM User Schema, - ready for conversion to a JSON object. - """ - return { - "id": self.id, - "externalId": self.obj.scim_external_id, - "schemas": [constants.SchemaURI.USER], - "userName": self.obj.user.username, - "name": { - "givenName": self.obj.user.first_name, - "familyName": self.obj.user.last_name, - "formatted": self.name_formatted, - }, - "displayName": self.display_name, - "emails": self.emails, - "active": self.obj.user.is_active, - "groups": [], - "meta": self.meta, - } - - def from_dict(self, d): - """ - Consume a ``dict`` conforming to the SCIM User Schema, updating the - internal user object with data from the ``dict``. - - Please note, the user object is not saved within this method. To - persist the changes made by this method, please call ``.save()`` on the - adapter. Eg:: - - scim_user.from_dict(d) - scim_user.save() - """ - # Store dict for possible later use when saving user - self._from_dict_copy = copy.deepcopy(d) - - self.obj.user = User() - - self.parse_active(d.get("active")) - - self.obj.first_name = d.get("name", {}).get("givenName") or "" - - self.obj.last_name = d.get("name", {}).get("familyName") or "" - - super().parse_emails(d.get("emails")) - - if self.is_new_user and not self.obj.email: - raise scim_exceptions.BadRequestError("Empty email value") # noqa: TRY003 EM101 - - self.obj.scim_username = d.get("userName") - self.obj.scim_external_id = d.get("externalId") or "" - - def parse_active(self, active): - """ - Set User.is_active to the value from the SCIM request. - - Args: - active (bool): The value of 'active' from the SCIM request. - """ - if active is not None: - if active != self.obj.user.is_active: - self.activity_changed = True - self.obj.user.is_active = active - - def save(self): - """ - Save instances of the Profile and User models. - - Raises: - self.reformat_exception: Error while creating or saving Profile or User model. - """ - try: - with transaction.atomic(): - self.obj.user.email = self.obj.email - self.obj.user.username = self.obj.email - self.obj.user.first_name = self.obj.first_name - self.obj.user.last_name = self.obj.last_name - self.obj.user.save() - self.obj.name = self.display_name - self.obj.save() - logger.info("User saved. User id %i", self.obj.id) - except Exception as e: - raise self.reformat_exception(e) from e - - def delete(self): - """ - Update User's is_active to False. - """ - self.obj.is_active = False - self.obj.save() - logger.info("Deactivated user id %i", self.obj.id) - - def handle_add(self, path, value): - """ - Handle add operations per: - https://tools.ietf.org/html/rfc7644#section-3.5.2.1 - - Args: - path (AttrPath) - value (Union[str, list, dict]) - """ - if path == "externalId": - self.obj.scim_external_id = value - self.obj.save() - - def handle_replace(self, value): - """ - Handle the replace operations. - - All operations happen within an atomic transaction. - - Args: - value (Union[str, list, dict]) - """ - attr_map = { - "familyName": "last_name", - "givenName": "first_name", - "active": "is_active", - "userName": "scim_username", - "externalId": "scim_external_id", - } - - for attr, attr_value in (value or {}).items(): - if attr in attr_map: - setattr(self.obj, attr_map.get(attr), attr_value) - - elif attr == "emails": - self.parse_email(attr_value) - - elif attr == "password": - self.obj.set_password(attr_value) - - else: - raise scim_exceptions.SCIMException("Not Implemented", status=409) # noqa: EM101, TRY003 - - self.obj.save() diff --git a/profiles/plugins.py b/profiles/plugins.py index 122f234d4d..7faccab4e3 100644 --- a/profiles/plugins.py +++ b/profiles/plugins.py @@ -2,18 +2,20 @@ from django.apps import apps -from profiles.models import Profile +from profiles.api import ensure_profile class CreateProfilePlugin: hookimpl = apps.get_app_config("authentication").hookimpl @hookimpl - def user_created(self, user): + def user_created(self, user, user_data): """ Perform functions on a newly created user Args: - user(User): The user to create the list for + user(User): the user that was created + user_data(dict): the user data """ - Profile.objects.get_or_create(user=user) + profile_data = user_data.get("profile", {}) + ensure_profile(user, profile_data) diff --git a/profiles/scim/__init__.py b/profiles/scim/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/profiles/scim/adapters.py b/profiles/scim/adapters.py new file mode 100644 index 0000000000..53370d6d04 --- /dev/null +++ b/profiles/scim/adapters.py @@ -0,0 +1,210 @@ +import logging +from typing import Optional, Union + +from django.contrib.auth import get_user_model +from django.db import transaction +from django_scim import constants, exceptions +from django_scim.adapters import SCIMUser +from scim2_filter_parser.attr_paths import AttrPath + +from profiles.models import Profile + +User = get_user_model() + + +logger = logging.getLogger(__name__) + + +def get_user_model_for_scim(): + """ + Get function for the django_scim library configuration (USER_MODEL_GETTER). + + Returns: + model: User model. + """ + return User + + +class LearnSCIMUser(SCIMUser): + """ + Custom adapter to extend django_scim library. This is required in order + to extend the profiles.models.Profile model to work with the + django_scim library. + """ + + password_changed = False + activity_changed = False + + resource_type = "User" + + id_field = "profile__scim_id" + + ATTR_MAP = { + ("active", None, None): "is_active", + ("name", "givenName", None): "first_name", + ("name", "familyName", None): "last_name", + } + + @property + def is_new_user(self): + """_summary_ + + Returns: + bool: True is the user does not currently exist, + False if the user already exists. + """ + return not bool(self.obj.id) + + @property + def id(self): + """ + Return the SCIM id + """ + return self.obj.profile.scim_id + + @property + def emails(self): + """ + Return the email of the user per the SCIM spec. + """ + return [{"value": self.obj.email, "primary": True}] + + @property + def display_name(self): + """ + Return the displayName of the user per the SCIM spec. + """ + return self.obj.profile.name + + @property + def meta(self): + """ + Return the meta object of the user per the SCIM spec. + """ + return { + "resourceType": self.resource_type, + "created": self.obj.date_joined.isoformat(timespec="milliseconds"), + "lastModified": self.obj.profile.updated_at.isoformat( + timespec="milliseconds" + ), + "location": self.location, + } + + def to_dict(self): + """ + Return a ``dict`` conforming to the SCIM User Schema, + ready for conversion to a JSON object. + """ + return { + "id": self.id, + "externalId": self.obj.profile.scim_external_id, + "schemas": [constants.SchemaURI.USER], + "userName": self.obj.username, + "name": { + "givenName": self.obj.first_name, + "familyName": self.obj.last_name, + "formatted": self.name_formatted, + }, + "displayName": self.display_name, + "emails": self.emails, + "active": self.obj.is_active, + "groups": [], + "meta": self.meta, + } + + def from_dict(self, d): + """ + Consume a ``dict`` conforming to the SCIM User Schema, updating the + internal user object with data from the ``dict``. + + Please note, the user object is not saved within this method. To + persist the changes made by this method, please call ``.save()`` on the + adapter. Eg:: + + scim_user.from_dict(d) + scim_user.save() + """ + self.parse_emails(d.get("emails")) + + self.obj.is_active = d.get("active", True) + self.obj.username = d.get("userName") + self.obj.first_name = d.get("name", {}).get("givenName", "") + self.obj.last_name = d.get("name", {}).get("familyName", "") + + self.obj.profile = getattr(self.obj, "profile", Profile()) + self.obj.profile.scim_username = d.get("userName") + self.obj.profile.scim_external_id = d.get("externalId") + self.obj.profile.name = d.get("name", {}).get("formatted", "") + + def save(self): + """ + Save instances of the Profile and User models. + """ + with transaction.atomic(): + # user must be saved first due to FK Profile -> User + self.obj.save() + self.obj.profile.user = self.obj + self.obj.profile.save() + logger.info("User saved. User id %i", self.obj.id) + + def delete(self): + """ + Update User's is_active to False. + """ + self.obj.is_active = False + self.obj.save() + logger.info("Deactivated user id %i", self.obj.user.id) + + def handle_add( + self, + path: Optional[AttrPath], + value: Union[str, list, dict], + operation: dict, # noqa: ARG002 + ): + """ + Handle add operations per: + https://tools.ietf.org/html/rfc7644#section-3.5.2.1 + + Args: + path (AttrPath) + value (Union[str, list, dict]) + """ + if path is None: + return + + if path.first_path == ("externalId", None, None): + self.obj.profile.scim_external_id = value + self.obj.save() + + def handle_replace( + self, + path: Optional[AttrPath], + value: Union[str, list, dict], + operation: dict, # noqa: ARG002 + ): + """ + Handle the replace operations. + + All operations happen within an atomic transaction. + """ + if not isinstance(value, dict): + # Restructure for use in loop below. + value = {path: value} + + for nested_path, nested_value in (value or {}).items(): + if nested_path.first_path in self.ATTR_MAP: + setattr( + self.obj, self.ATTR_MAP.get(nested_path.first_path), nested_value + ) + + elif nested_path.first_path == ("name", "formatted", None): + self.obj.profile.name = nested_value + + elif nested_path.first_path == ("emails", None, None): + self.parse_emails(value) + + else: + msg = "Not Implemented" + raise exceptions.NotImplementedError(msg) + + self.save() diff --git a/profiles/scim/filters.py b/profiles/scim/filters.py new file mode 100644 index 0000000000..62da43c276 --- /dev/null +++ b/profiles/scim/filters.py @@ -0,0 +1,17 @@ +from typing import Optional + +from django_scim.filters import UserFilterQuery + + +class LearnUserFilterQuery(UserFilterQuery): + """Filters for users""" + + attr_map: dict[tuple[Optional[str], Optional[str], Optional[str]], str] = { + ("userName", None, None): "auth_users.username", + ("active", None, None): "auth_users.is_active", + ("name", "formatted", None): "profiles_profile.name", + } + + joins: tuple[str, ...] = ( + "INNER JOIN profiles_profile ON profiles_profile.user_id = id", + ) diff --git a/profiles/scim/views_test.py b/profiles/scim/views_test.py new file mode 100644 index 0000000000..d84ad8e1c8 --- /dev/null +++ b/profiles/scim/views_test.py @@ -0,0 +1,74 @@ +import json + +from django.contrib.auth import get_user_model +from django.urls import reverse +from django_scim import constants + +User = get_user_model() + + +def test_scim_post_user(staff_client): + """Test that we can create a user via SCIM API""" + user_q = User.objects.filter(profile__scim_external_id="1") + assert not user_q.exists() + + resp = staff_client.post( + reverse("scim:users"), + content_type="application/scim+json", + data=json.dumps( + { + "schemas": [constants.SchemaURI.USER], + "emails": [{"value": "jdoe@example.com", "primary": True}], + "active": True, + "userName": "jdoe", + "externalId": "1", + "name": { + "formatted": "John Smith Doe", + "familyName": "Doe", + "givenName": "John", + }, + } + ), + ) + + assert resp.status_code == 201, f"Error response: {resp.content}" + + user = user_q.first() + + assert user is not None + assert user.email == "jdoe@example.com" + assert user.username == "jdoe" + assert user.first_name == "John" + assert user.last_name == "Doe" + assert user.profile.name == "John Smith Doe" + + # test an update + resp = staff_client.put( + f"{reverse('scim:users')}/{user.profile.scim_id}", + content_type="application/scim+json", + data=json.dumps( + { + "schemas": [constants.SchemaURI.USER], + "emails": [{"value": "jsmith@example.com", "primary": True}], + "active": True, + "userName": "jsmith", + "externalId": "1", + "name": { + "formatted": "Jimmy Smith", + "familyName": "Smith", + "givenName": "Jimmy", + }, + } + ), + ) + + assert resp.status_code == 200, f"Error response: {resp.content}" + + user = user_q.first() + + assert user is not None + assert user.email == "jsmith@example.com" + assert user.username == "jsmith" + assert user.first_name == "Jimmy" + assert user.last_name == "Smith" + assert user.profile.name == "Jimmy Smith"