-
-
Notifications
You must be signed in to change notification settings - Fork 322
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 users endpoint #1199
base: main
Are you sure you want to change the base?
SCIM users endpoint #1199
Conversation
try { | ||
const fakeScope: Scope = { userId: id }; | ||
// FIXME: deleteUser should probably better not requiring a scope. | ||
await this._dbManager.deleteUser(fakeScope, id); |
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.
It feels like a bit dirty to me to create a fake Scope, but I fear it is hard to get rid of it from UsersManager.deleteUser
, especially because the deleteOrg
needs it.
Any opinion/idea on that?
995cd46
to
6ed0e85
Compare
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.
Thanks for this huge job.
I think It could be great to have a documentation/scim.md file documenting at least :
- What part of the SCIM RFCs are not yet implemented,
- What part of the RFCs are "adapted" to work with grist current logic
It could be a huge time saver for administrators of instances when doing the interconnection.
app/server/lib/scim/v2/ScimTypes.ts
Outdated
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.
Is there a clear policy about keeping or removing trailing new-lines in the project ?
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.
The linter says nothing 🤷
Thanks @hexaltation for you feedback!
I am rather planing on documenting the API in the |
85a5ac9
to
8985a91
Compare
0aaf9e7
to
153fa5c
Compare
153fa5c
to
54159ea
Compare
*/ | ||
public async overrideUser(resource: any, data: any, context: RequestContext) { | ||
return this._runAndHandleErrors(context, async () => { | ||
const id = ScimUserController._getIdFromResource(resource); |
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 think we should not authorize the update and the deletion of Special Ids.
There also is a edge case for GRIST_DEFAULT_EMAIL, should we allow the IdP administrator propagate a change of their email (which would the owner of that email would lose the installation admin right)? I guess it's the responsibility of the IdP admin to be sure of the impacts, also it is easy to fix.
Do you have any thoughts about these cases?
*/ | ||
public async deleteUser(resource: any, context: RequestContext) { | ||
return this._runAndHandleErrors(context, async () => { | ||
const id = ScimUserController._getIdFromResource(resource); |
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.
Same problem regarding the Special Ids
export function toUserProfile(scimUser: any, existingUser?: User): UserProfile { | ||
const emailValue = scimUser.emails?.[0]?.value; | ||
if (emailValue && normalizeEmail(emailValue) !== normalizeEmail(scimUser.userName)) { | ||
throw new SCIMMY.Types.Error(400, 'invalidValue', 'Email and userName must be the same'); |
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 tested it against the SCIM plugin for Keycloak (currently being developed). Of course you can have users with a userName and an email different.
A look at the RFCs have not revealed me that the userName is required to be taken into consideration by the service provider, despite being mandatory. I would probably just log a warning telling that both values are different and that the userName is ignored in such case.
I could test my development against an existing IdP with SCIM support, which raised some interesting questions. I expect them to be not very hard to fix. |
Context
As an IT asset administrator, I can create account of my users using a centralized solution like an SSO so they can log on Grist. It's quite convenient because I don't have to worry about creating accounts specifically for them.
Also Grist handles the update of Users when reconnect.
There are things the administrator cannot do though:
owners of an team site
);Proposed solution
SCIM is a standard proposed by the IETF through RFC7644 and RFC7643 which aims to through a simple Rest API provide solution for the above use cases.
Here is the abstract of the RFC7644 which introduces SCIM:
This PR provides the implementation of SCIM for Users Resources (Group will come in a future PR), and supports:
POST /Users/
,PUT /Users/:id
,GET /Users/
,GET /Users/:id
andDELETE /Users/:id
)./Schemas
,/ServiceProviderConfig
,/ResourceTypes
endpoints;/Me
endpoint (it takes advantage of theid
you returned in the authentication middleware);POST /Bulk
endpointPOST /Resources/.search
by using the Filters (actually to use them, you must have to fetch all the Resources from the DB, the filtering is done in JS, which is probably fine for small projects, I would just be cautious when using big databases + an ORM);PATCH /Resources/:id
endpoint! It reads a resource using the egress method, applies the asked changes, and calls the ingress method to update the record ;To do that, I take advantage of two libraries: scimmy and scimmy-routers. Scimmy is lightweight (0 dependency), and scimmy-routers will also be dependency-free be in a future version (reported in that issue and already fixed).
Two variables are introduced:
GRIST_ENABLE_SCIM
to let the administrator enable the scim API (defaults to false);GRIST_SCIM_EMAIL
to let the administrator specify a user that is allowed, they are granted rights to do any operation using SCIM (just like the administrators ofGRIST_DEFAULT_EMAIL
andGRIST_SUPPORT_EMAIL
);Assumption regarding the SCIM implementation
userName
corresponds to the normalized email (logins.email
), the SCIMemails
corresponds to thedisplayEmail
;How to test manually?
I can document the API in grist-help upon request (otherwise I will do that after this PR is merged).
You may:
GRIST_ENABLE_SCIM
:GRIST_SCIM_EMAIL="you@example.com" GRIST_ENABLE_SCIM=1 yarn start
I described some examples of the SCIM API usage here (they need to be adaptated for the context of Grist): https://github.com/scimmyjs/scimmy-routers/blob/8ffa2221b542054c3f0cfb765ea6957f29ebe5e1/example/README.md#play-with-the-scim-server
Limitations of the current implementation
GRIST_DEFAULT_EMAIL
or the support account) or the user withGRIST_SCIM_EMAIL
are allowed to make operations on resources (other user are limited to use/Me
).GRIST_SCIM_EMAIL
) is required to have access to the endpoints, which should have their API key generated. I considered instead having a bearer directly set in an env variable, but I rejected the idea because it would have been rejected by the Authorizer./Me
endpoint implementation seems partial (issue);any
types or some casts until that is fixed (issue and issue);Content-Type
must beapplication/scim+json
, currentlyapplication/json
is not supported (will be fixed in the next scimmy-routers release)Documentation
I opened this PR in draft to start documenting SCIM: gristlabs/grist-help#434
It can be previewed here:
Related issues
It partly implements #870 (Users resource only for now).
Has this been tested?