SCIM: Add "is SCIM controlled" flag to site user, and "SCIM" badge on UI#48727
Conversation
There was a problem hiding this comment.
This is for live users
There was a problem hiding this comment.
This is for deleted users
There was a problem hiding this comment.
It cannot be >1 (a unique index below prevents that), but using >= rather than = here adds an extra layer of security in case we add multiple SCIM accounts later.
There was a problem hiding this comment.
We had no index on user_id and service_type, so this could be slow. I've added an index—see below.
Bundle size report 📦
Look at the Statoscope report for a full comparison between the commits 7ec15a5 and eccbef8 or learn more. Open explanation
|
There was a problem hiding this comment.
We need this to ensure performance, but it has the additional benefit of making SCIM accounts unique.
There was a problem hiding this comment.
Oh lol I think it requires ON to be on the same line. If you'd like to fix up the regex (read.go L232) so that it accepts newlines as the space charset you should be able to pass the migration as-is (unless I'm completely off base).
There was a problem hiding this comment.
I think \s matches the newline in the Go implementation of regexp (I'm pretty sure it usually matches newlines), so it should work, but I'll try.
There was a problem hiding this comment.
You can slam a newline in one of the test cases and see if it still passes - if not you have the first 1/3d of TDD done!
85faae3 to
8152bd4
Compare
| """ | ||
| Whether the user is SCIM enabled. | ||
| """ | ||
| scimEnabled: Boolean! |
There was a problem hiding this comment.
I like the SCIMControlled naming you used in the PR description a little better, but thats about as minor as it gets.
There was a problem hiding this comment.
I've changed the naming to "controlled" everywhere, thanks!
| {scimEnabled && ( | ||
| <Tooltip content="This user is SCIM-enabled—an external system controls some of its attributes."> | ||
| <Badge variant="primary" className="mr-1"> | ||
| SCIM |
There was a problem hiding this comment.
nit/question - Is it expected that users will know what SCIM means? I hadn't heard of it until recently. Not what would be a better replacement that is short enough to not take over that space.
There was a problem hiding this comment.
Good point! Added a link to explain it. Thanks!
kopancek
left a comment
There was a problem hiding this comment.
I'm approving to unblock.
But I'm not completely sure adding a one-off for SCIM is the best way to go. Maybe we can instead show a column for externalAccounts in the users list? 🤔
I was thinking in a similar way about it being tied to SCIM, but generally I think we can say it will always be True/False if the user account is controlled by some external system (in this case scim) and that is an indication of what can or can't be done with the sg ui. |
8f432b4 to
9e34fd8
Compare
9e34fd8 to
be2af57
Compare
4ee585b to
4b18e75
Compare
|
Sorry I'm late to this @vdavid. There are some issues with the design:
Can we move the badge to the end of the name, right aligned to the table row and use the secondary or info badge instead?
|
|
Absolutely, @rrhyne, will do, and ping you with a screenshot. |
|
Ideally this should be a collumn, but the layout is too narrow for that. I also agree that the badge works after the name |
|
Agree with @danielmarquespt but didn't want to make that change last minute. @vdavid DM me when complete!!! |
|
It's complete, see https://github.com/sourcegraph/sourcegraph/pull/49114 |
Adds a "SCIMControlled" flag to
SiteUser, and fetches that via GraphQL, then displays a small "SCIM" badge if the user is SCIM-enabled. It looks like this:(To the design reviewer: check out the blue "SCIM" badges next to some users. It's new. It indicates that the user is created/modified through SCIM, from an external system. My question is whether this looks good or I should indicate this differently.)
And with the tooltip over the new badge:
This is a small PR because I want to get early feedback to make sure no one in IAM has problems with extending
SiteUserfor this purpose.Note: I may want to add the same field to
Userlater today, depending on UI requirements. Feedback is welcome on that thought as well! (I don't know if it's too dirty to mention "SCIM" in our general User model to the extent of an added boolean. It'd make things very easy, though!)Test plan