diff --git a/authentication/backends/ol_open_id_connect.py b/authentication/backends/ol_open_id_connect.py index cbfe4a8dbb..e2f406b54d 100644 --- a/authentication/backends/ol_open_id_connect.py +++ b/authentication/backends/ol_open_id_connect.py @@ -10,17 +10,3 @@ 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 c1b596214e..250e8c9261 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, user_data): + def user_created(self, user): """Trigger actions after a user is created""" diff --git a/authentication/pipeline/user.py b/authentication/pipeline/user.py index b200c5716f..3ec04bb25d 100644 --- a/authentication/pipeline/user.py +++ b/authentication/pipeline/user.py @@ -3,7 +3,6 @@ from social_core.exceptions import AuthException from authentication.hooks import get_plugin_manager -from profiles import api as profile_api def forbid_hijack( @@ -24,16 +23,14 @@ def forbid_hijack( return {} -def user_created_actions(*, user, details, **kwargs): +def user_created_actions(**kwargs): """ Trigger plugins when a user is created """ if kwargs.get("is_new"): pm = get_plugin_manager() hook = pm.hook - hook.user_created(user=user, user_data=details) - else: - profile_api.ensure_profile(user=user, profile_data=details.get("profile", {})) + hook.user_created(user=kwargs["user"]) def user_onboarding(*, backend, **kwargs): diff --git a/authentication/pipeline/user_test.py b/authentication/pipeline/user_test.py index a88a1d6ea4..b6893ad90b 100644 --- a/authentication/pipeline/user_test.py +++ b/authentication/pipeline/user_test.py @@ -38,7 +38,6 @@ 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 f4637aa1ba..229ab4a4ba 100644 --- a/learning_resources/plugins.py +++ b/learning_resources/plugins.py @@ -10,13 +10,12 @@ class FavoritesListPlugin: hookimpl = apps.get_app_config("authentication").hookimpl @hookimpl - def user_created(self, user, user_data): # noqa: ARG002 + def user_created(self, user): """ 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 9efa1747dc..904ba0367c 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, user_data={}) + FavoritesListPlugin().user_created(user) user.refresh_from_db() assert user.user_lists.count() == 1 diff --git a/main/settings.py b/main/settings.py index fc1d1bffcb..3c56978486 100644 --- a/main/settings.py +++ b/main/settings.py @@ -136,9 +136,8 @@ "documentationUri": "", }, ], - "USER_ADAPTER": "profiles.scim.adapters.LearnSCIMUser", - "USER_MODEL_GETTER": "profiles.scim.adapters.get_user_model_for_scim", - "USER_FILTER_PARSER": "profiles.scim.filters.LearnUserFilterQuery", + "USER_ADAPTER": "profiles.adapters.SCIMProfile", + "USER_MODEL_GETTER": "profiles.adapters.get_user_model_for_scim", } @@ -298,9 +297,6 @@ ), 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 new file mode 100644 index 0000000000..17bccbe2c3 --- /dev/null +++ b/profiles/adapters.py @@ -0,0 +1,218 @@ +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 7faccab4e3..122f234d4d 100644 --- a/profiles/plugins.py +++ b/profiles/plugins.py @@ -2,20 +2,18 @@ from django.apps import apps -from profiles.api import ensure_profile +from profiles.models import Profile class CreateProfilePlugin: hookimpl = apps.get_app_config("authentication").hookimpl @hookimpl - def user_created(self, user, user_data): + def user_created(self, user): """ Perform functions on a newly created user Args: - user(User): the user that was created - user_data(dict): the user data + user(User): The user to create the list for """ - profile_data = user_data.get("profile", {}) - ensure_profile(user, profile_data) + Profile.objects.get_or_create(user=user) diff --git a/profiles/scim/__init__.py b/profiles/scim/__init__.py deleted file mode 100644 index e69de29bb2..0000000000 diff --git a/profiles/scim/adapters.py b/profiles/scim/adapters.py deleted file mode 100644 index 53370d6d04..0000000000 --- a/profiles/scim/adapters.py +++ /dev/null @@ -1,210 +0,0 @@ -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 deleted file mode 100644 index 62da43c276..0000000000 --- a/profiles/scim/filters.py +++ /dev/null @@ -1,17 +0,0 @@ -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 deleted file mode 100644 index d84ad8e1c8..0000000000 --- a/profiles/scim/views_test.py +++ /dev/null @@ -1,74 +0,0 @@ -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"