-
Notifications
You must be signed in to change notification settings - Fork 239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SCIM provisioning API basic implementation #17144
base: develop
Are you sure you want to change the base?
Conversation
ea6a6d6
to
dd52360
Compare
(I've taken this out of the review queue as its in draft, let us know if you want feedback) |
Hi @erikjohnston There is one design question though. I see that there is a dependency to pydantic in synapse, and I recently published scim2-models that is a library that helps to parse and serialize SCIM2 payloads using pydantic. I think the SCIM implementation would greatly benefit from using scim2-models, as a big part of the specification compliance would be delegated to the library. Would it be acceptable to add a dependency towards scim2-models in synapse, or should I continue checking and building SCIM2 payloads manually? |
f893967
to
81d751b
Compare
dcd72ed
to
6a1e1b2
Compare
Hi @erikjohnston |
Implementation of a subset of SCIM endpoint and capabilities as described in MSC4098. Signed-off-by: Éloi Rivard <eloi@yaal.coop>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
@reivilibre I think all your comments have been addressed now. Most of them have been fixed in the code and I responded to some with new questions. I am sorry for the many commits, the PR looks like a mess. If it is easier for the review, I can close it and open a new one. Let me know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is looking generally good, but a few comments (and some still open from last time).
The CI is also unhappy but hopefully that's nothing too difficult?
|
||
```yaml | ||
experimental_features: | ||
msc4098: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is no longer tracking an MSC, let's rename this feature to just scim
.
I understand what MatMaul is saying but I would still prefer it in an experimental feature that is disabled by default.
That makes it easier to remove later if for some reason we can't commit to supporting it anymore, plus with MAS around the corner it is quite likely to get dropped from Synapse again.
I'm not against people having a SCIM API as long as they are aware of this though.
``` | ||
DELETE /_matrix/client/unstable/coop.yaal/scim/Users/@bjensen:test | ||
``` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this was noted on the documentation?
@@ -256,6 +256,19 @@ class MSC3866Config: | |||
require_approval_for_new_accounts: bool = False | |||
|
|||
|
|||
@attr.s(auto_attribs=True, frozen=True, slots=True) | |||
class MSC4098Config: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class MSC4098Config: | |
class ScimConfig: |
`picture_https_url`. | ||
""" | ||
if media_repo is None: | ||
logger.info( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger.info( | |
logger.warning( |
@@ -81,6 +81,33 @@ def build_jinja_env( | |||
return env | |||
|
|||
|
|||
def mxc_to_http( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this is no longer functional, given authenticated media means you can't just hotlink to media like this anymore.
This is an implementation of MSC4098. It implements a subset of the SCIM provisioning protocol defined in RFC7643 and RFC7644.
It contains:
synapse/rest/admin/users.py
.The SCIM requires needs python 3.9+ (because of the use of typing.Anotated in scim2-models) and pydantic 2.7.0+
SCIM implementation details
Only a subset of the SCIM endpoints are implemented:
What's implemented:
What is defined in the SCIM specs but not implemented here:
What do you think?
Pull Request Checklist
EventStore
toEventWorkerStore
.".code blocks
.(run the linters)