From 8289971d36d2c7881abb3e8505c4abc1b97f731b Mon Sep 17 00:00:00 2001
From: Arie Peterson <arie@greenhost.nl>
Date: Thu, 6 Jun 2024 12:36:30 +0200
Subject: [PATCH] Only include full name in SCIM when changed
---
backend/areas/apps/models.py | 14 ++++
backend/areas/users/user_service.py | 28 ++++++--
backend/cliapp/cliapp/cli.py | 4 ++
backend/helpers/kratos_user.py | 18 ++++++
backend/helpers/provision.py | 64 ++++++++++++++++---
.../9ee5a7d65fa7_scim_app_attributes.py | 31 +++++++++
6 files changed, 145 insertions(+), 14 deletions(-)
create mode 100644 backend/migrations/versions/9ee5a7d65fa7_scim_app_attributes.py
diff --git a/backend/areas/apps/models.py b/backend/areas/apps/models.py
index e9330ab9..88930330 100644
--- a/backend/areas/apps/models.py
+++ b/backend/areas/apps/models.py
@@ -316,3 +316,17 @@ class OAuthClientApp(db.Model): # pylint: disable=too-few-public-methods
def __repr__(self):
return (f"oauthclient_id: {self.oauthclient_id}, app_id: {self.app_id},"
f" app: {self.app}")
+
+class ScimAttribute(db.Model): # pylint: disable=too-few-public-methods
+ """
+ The ScimAttribute object records that a certain user attribute needs to be
+ set in a certain app via SCIM.
+ """
+
+ user_id = db.Column(String(length=64), primary_key=True)
+ app_id = db.Column(Integer, ForeignKey("app.id"), primary_key=True)
+ attribute = db.Column(String(length=64), primary_key=True)
+
+ def __repr__(self):
+ return (f"attribute: {self.attribute}, user_id: {self.user_id},"
+ f" app_id: {self.app_id}")
diff --git a/backend/areas/users/user_service.py b/backend/areas/users/user_service.py
index cb59124f..a123cab1 100644
--- a/backend/areas/users/user_service.py
+++ b/backend/areas/users/user_service.py
@@ -20,6 +20,7 @@ from areas.apps.apps_service import AppsService
from areas.roles import Role
from helpers import KratosApi
from helpers.error_handler import KratosError
+from helpers.provision import Provision
from helpers.threads import request_provision
@@ -123,11 +124,28 @@ class UserService:
@classmethod
def put_user(cls, id, data):
- kratos_data = {
- "schema_id": "default",
- "traits": {"email": data["email"], "name": data.get("name", "")},
- }
- KratosApi.put("/admin/identities/{}".format(id), kratos_data)
+ # Get the old version of the identity. We need that for comparison to
+ # see if some attributes are changed by our update.
+ old_user = KratosApi.get("/admin/identities/{}".format(id)).json()
+ old_name = old_user["traits"].get("name", "")
+ new_name = data.get("name", "")
+ # Create list of patches with our changes.
+ patches = []
+ patches.append(JsonPatch(op="replace", path="/traits/email", value=data['email']))
+ patches.append(JsonPatch(op="replace", path="/traits/name", value=new_name))
+ # Determine whether we're really changing the name, and if so record
+ # that fact in the database in the form of a ScimAttribute. We'll use
+ # that information later during provisioning via SCIM.
+ if old_name != new_name:
+ current_app.logger.info(f"Name changed for: {data['email']}")
+ current_app.logger.info(f" old name: {old_name}")
+ current_app.logger.info(f" new name: {new_name}")
+ Provision.store_attribute(attribute='name', user_id=id)
+ # We used a PUT before, but that deletes any attributes that we don't
+ # specify, which is not so convenient. So we PATCH just the attributes
+ # we're changing instead.
+ patch_doc = JsonPatchDocument(value=patches)
+ kratos_identity_api.patch_identity(id, json_patch_document=patch_doc)
if data["app_roles"]:
app_roles = data["app_roles"]
diff --git a/backend/cliapp/cliapp/cli.py b/backend/cliapp/cliapp/cli.py
index ca93b5e1..5174e053 100644
--- a/backend/cliapp/cliapp/cli.py
+++ b/backend/cliapp/cliapp/cli.py
@@ -319,6 +319,10 @@ def update_user(email, field, value):
else:
current_app.logger.error(f"Field not found: {field}")
+ # TODO: this currently deletes the last_recovery and last_login because
+ # `save` uses a simple PUT and is not aware of those fields. We should
+ # switch to PATCH instead, or refactor so `save` uses the same code as
+ # `put_user`.
user.save()
diff --git a/backend/helpers/kratos_user.py b/backend/helpers/kratos_user.py
index 877aaca6..34f0527a 100644
--- a/backend/helpers/kratos_user.py
+++ b/backend/helpers/kratos_user.py
@@ -2,6 +2,7 @@
Implement the Kratos model to interact with kratos users
"""
+from flask import current_app
import json
import re
import urllib.parse
@@ -12,6 +13,10 @@ from urllib.request import Request
from ory_kratos_client.model.create_identity_body import CreateIdentityBody
from ory_kratos_client.model.create_recovery_link_for_identity_body \
import CreateRecoveryLinkForIdentityBody
+from ory_kratos_client.model.json_patch \
+ import JsonPatch
+from ory_kratos_client.model.json_patch_document \
+ import JsonPatchDocument
from ory_kratos_client.model.update_identity_body import UpdateIdentityBody
from ory_kratos_client.rest import ApiException as KratosApiException
@@ -33,6 +38,7 @@ class KratosUser():
state = None
created_at = None
updated_at = None
+ metadata_admin = None
def __init__(self, api, uuid = None):
self.api = api
@@ -55,6 +61,9 @@ class KratosUser():
self.state = obj.state
self.created_at = obj.created_at
self.updated_at = obj.updated_at
+ self.metadata_admin = obj.metadata_admin
+ if self.metadata_admin is None:
+ self.metadata_admin = {}
except KratosApiException as error:
raise BackendError(f"Unable to get entry, kratos replied with: {error}", error) from error
@@ -107,6 +116,15 @@ class KratosUser():
except KratosApiException as error:
raise BackendError(f"Unable to save entry, kratos replied with:{error}") from error
+ def set_metadata(self, **kwargs):
+ current_app.logger.info(f"Setting metadata for {self.__uuid}:")
+ patches = []
+ for k, v in kwargs.items():
+ current_app.logger.info(f" {k}={v}")
+ patches.append(JsonPatch(op="replace", path=f"/metadata_admin/{k}", value=v))
+ patch_doc = JsonPatchDocument(value=patches)
+ self.api.patch_identity(self.__uuid, json_patch_document=patch_doc)
+
def delete(self):
"""Deletes the object from kratos
:raise: BackendError if Krator API call fails
diff --git a/backend/helpers/provision.py b/backend/helpers/provision.py
index beaba6a9..41b32edb 100644
--- a/backend/helpers/provision.py
+++ b/backend/helpers/provision.py
@@ -5,8 +5,10 @@ import ory_kratos_client
from ory_kratos_client.api import identity_api
import ory_kratos_client.exceptions
import requests
+from sqlalchemy import exc, select
+from sqlalchemy.sql.expression import literal
-from areas.apps.models import App, AppRole, ProvisionStatus
+from areas.apps.models import App, AppRole, ProvisionStatus, ScimAttribute
from areas.roles.models import Role
import config
from database import db
@@ -175,7 +177,7 @@ class Provision:
db.session.commit()
else:
logging.info(f"Deleting user {app_role.user_id} from {app.slug}")
- url = f"{app.scim_url}Users/{existing_user.scim_id}"
+ url = f"{app.scim_url}/Users/{existing_user.scim_id}"
response = requests.delete(url, headers=scim_headers)
logging.debug(f"SCIM http status: {response.status_code}")
if response.status_code == 204:
@@ -210,15 +212,37 @@ class Provision:
# Zulip does not read the `emails` property, instead getting the
# email from the `userName` property.
'userName': kratos_user.email,
- 'displayName': kratos_user.name,
'emails': [{
'value': kratos_user.email,
'primary': True
}],
- 'name': {
- 'formatted': kratos_user.name,
- },
}
+ # Decide whether to include the displayName in the SCIM data. Usually
+ # we want that, but if this is an existing user, and we think the
+ # displayName as stored in Stackspin has not changed since the last
+ # provisioning run, then we do not set it to prevent overwriting a
+ # displayName the user may have set inside the app, bypassing
+ # Stackspin.
+ name_changed = ScimAttribute.query.filter_by(
+ user_id=app_role.user_id,
+ app_id=app_role.app_id,
+ attribute='name',
+ ).first()
+ logging.debug(f"Name changed: {name_changed}")
+ include_name = existing_user is None or name_changed is not None
+ if include_name:
+ logging.debug(f"Setting name in provisioning request.")
+ data['displayName'] = kratos_user.name
+ data['name'] = {
+ 'formatted': kratos_user.name,
+ }
+ # Now clear the `name_changed` attribute so we don't set the name
+ # again -- until it's changed again in Stackspin.
+ Provision.done_attribute(
+ attribute='name',
+ user_id=app_role.user_id,
+ app_id=app_role.app_id,
+ )
# Make some app-specific additions and modifications to the SCIM data.
if app.slug == 'nextcloud':
@@ -230,7 +254,7 @@ class Provision:
data['userName'] = f"stackspin-{app_role.user_id}"
if app.slug == 'zulip':
# Zulip does not accept an empty formatted name.
- if kratos_user.name is None or kratos_user.name == '':
+ if include_name and (kratos_user.name is None or kratos_user.name == ''):
data['name']['formatted'] = "name not set"
# Zulip doesn't support SCIM user groups, but we can set the user
# role as a field on the user object.
@@ -287,7 +311,7 @@ class Provision:
# Track how many users we've received thus far so we know when to stop.
running_total = 0
while True:
- url = f"{app.scim_url}Users?count={count}&startIndex={startIndex}"
+ url = f"{app.scim_url}/Users?count={count}&startIndex={startIndex}"
scim_headers = {
'Authorization': 'Bearer ' + app.scim_token
}
@@ -417,4 +441,26 @@ class Provision:
logging.info("SCIM result was not json")
logging.info(response.content)
raise ProvisionError("{app.slug} returned non-json data to SCIM group PUT.")
- logging.info(f"got: {response_json}")
+ logging.debug(f"got: {response_json}")
+
+ @staticmethod
+ def store_attribute(attribute, user_id):
+ select_apps = select(
+ literal(user_id),
+ literal(attribute),
+ App.id
+ ).where(App.scim_url != None)
+ insert = ScimAttribute.__table__.insert().prefix_with('IGNORE').from_select(
+ ['user_id', 'attribute', 'app_id'],
+ select_apps
+ )
+ db.session.execute(insert)
+
+ @staticmethod
+ def done_attribute(attribute, user_id, app_id):
+ logging.debug(f"Deleting ScimAttribute with attribute={attribute} user_id={user_id} app_id={app_id}")
+ db.session.query(ScimAttribute).filter(
+ ScimAttribute.attribute == attribute,
+ ScimAttribute.user_id == user_id,
+ ScimAttribute.app_id == app_id,
+ ).delete()
diff --git a/backend/migrations/versions/9ee5a7d65fa7_scim_app_attributes.py b/backend/migrations/versions/9ee5a7d65fa7_scim_app_attributes.py
new file mode 100644
index 00000000..572edc9b
--- /dev/null
+++ b/backend/migrations/versions/9ee5a7d65fa7_scim_app_attributes.py
@@ -0,0 +1,31 @@
+"""Extend SCIM support to include some attributes during provisioning only when
+they are changed, or the user is first created in the app.
+
+Revision ID: 9ee5a7d65fa7
+Revises: 267d280db490
+Create Date: 2024-06-04 15:39:00
+
+"""
+from alembic import op
+import sqlalchemy as sa
+
+# revision identifiers, used by Alembic.
+revision = '9ee5a7d65fa7'
+down_revision = '267d280db490'
+branch_labels = None
+depends_on = None
+
+def upgrade():
+ # An entry in this table records that a certain user attribute needs to be
+ # set in a certain app via SCIM.
+ op.create_table(
+ "scim_attribute",
+ sa.Column("user_id", sa.String(length=64), nullable=False),
+ sa.Column("app_id", sa.Integer(), nullable=False),
+ sa.Column("attribute", sa.String(length=64), nullable=False),
+ sa.PrimaryKeyConstraint("user_id", "app_id", "attribute"),
+ sa.ForeignKeyConstraint(["app_id"],["app.id"]),
+ )
+
+def downgrade():
+ op.drop_table("scim_attribute")
--
GitLab