SCIM: Restrict user fields on the UI

[vdavid]

• Builds on https://github.com/sourcegraph/sourcegraph/pull/48727
• Closes https://github.com/sourcegraph/sourcegraph/issues/48427

There are three user attributes on the profile and emails pages that SCIM interferes with:

• username
• display name
• email addresses

These fields are currently editable. But for SCIM-controlled users, the identity provider is the source of truth, and these fields shouldn't be editable on the UI.
This PR disables these fields for SCIM-controlled users and adds a note that that attributes are SCIM-controlled.

For the design reviewer: check out the screenshots below. I want to make sure that I'd using the right style of a notification (blue/info, rather than yellow/warning or other), that the text sounds right, and that the disabled fields are fine. I can include more screenshots or a short vid to show the cursors, etc.

Profile page for a non-SCIM-controlled vs. SCIM-controlled user—left side: (unchanged) enabled controls, right side: disabled controls:

Emails page for a non-SCIM-controlled vs. SCIM-controlled user—left side: (unchanged) enabled controls, right side: disabled controls:

The changes:

• Added a SCIMControlled (bool) field on the user model
• Populating the field based on the user_external_accounts content
• Added new field to the GraphQL query
• Requesting the new field in the user admin page query
• Disabling the UI based on the value

Test plan

• Design review
• CI should pass

[sg-e2e-regression-test-bob]

Bundle size report 📦

Initial size	Total size	Async size	Modules
0.00% (0.00 kb)	0.01% (+1.73 kb)	0.02% (+1.73 kb)	0.00% (0)

Look at the Statoscope report for a full comparison between the commits 34600cb and 012d347 or learn more.

Open explanation

• Initial size is the size of the initial bundle (the one that is loaded when you open the page)
• Total size is the size of the initial bundle + all the async loaded chunks
• Async size is the size of all the async loaded chunks
• Modules is the number of modules in the initial bundle

[vdavid]

This is similarly controversial to https://github.com/sourcegraph/sourcegraph/pull/48727 (see PR desc.), but I think this is the right way to do it. Being SCIM-controlled is an inherent-enough property of a user to justify having a boolean about it on its model. Besides, this is a decision that's easy to change later, so I'd rather move fast with it.

[chwarwick]

Is there a follow up item/PR to disallow these edits server side? While disabling them via the UI is good, the mutations are still able to be directly called and are easily discoverable and documented.

[vdavid]

@chwarwick

Is there a follow up item/PR to disallow these edits server side? While disabling them via the UI is good, the mutations are still able to be directly called and are easily discoverable and documented.

I see no huge harm done if that happens. I mean, practically, users with the right privileges can already make whatever changes they want to users (though updateUser, addUserEmail, setUserEmailVerified, and so on). I see no huge harm done if this is also allowed for SCIM-controlled users.

[chwarwick]

I do not see any issues with these code changes. I do continue to believe that only applying these restrictions via UI is problematic, and that they should be enforced server side as well.

[kopancek]

Added a comment with a suggestion for different approach, let me know what you think

[kopancek]

I'm wondering if we could use the user tags for this? E.g. if the user is scim controlled, we can add a tag named scim to the user in the database. Every time we read the user object, the tags will be populated. It would make the blast radius much smaller as well and we already did conditional treatment of users with different tags in the past. WDYT?

[vdavid]

Hmm, I like that it decreases the interface of the User model, but my problem with it is that it denormalizes data (source of truth for "is scim-controlled" becomes ambiguous). Maybe if we just add the additional, fake "scim" tag to the user model when loading it based on the status of user_external_accounts, but then it's rather hacky. :/ I think from these options, I prefer the current one (honestly, without status quo bias).

[vdavid]

@chwarwick, I've added it to the nice-to-haves to restrict user field editing on the back end from SCIM-controlled users.

[pjlast]

Wondering if you can simplify this to not do a SQL query on every row, something like:

Suggested change

	SELECT u.id,
	u.username,
	u.display_name,
	u.avatar_url,
	u.created_at,
	u.updated_at,
	u.site_admin,
	u.passwd IS NOT NULL,
	u.tags,
	u.invalidated_sessions_at,
	u.tos_accepted,
	u.searchable,
	(SELECT COUNT(user_id) FROM user_external_accounts WHERE user_id=u.id AND service_type = 'scim') >= 1 AS scim_controlled
	FROM users u %s`, query)
	SELECT u.id,
	u.username,
	u.display_name,
	u.avatar_url,
	u.created_at,
	u.updated_at,
	u.site_admin,
	u.passwd IS NOT NULL,
	u.tags,
	u.invalidated_sessions_at,
	u.tos_accepted,
	u.searchable,
	COUNT(uea.service_type) > 0 AS scim_controlled
	FROM users u
	LEFT JOIN user_external_accounts uea
	ON u.id = uea.user_id
	AND uea.service_type = 'scim'
	%s`, query)

[vdavid]

Fair point, I’ll take care of it in a follow-up PR!

[almeidapaulooliveira]

Some insights for the design:

• I'm unsure if the 'site-admin' Alert is part of this scope, but I didn't see that message as a warning. We also can use a simple component than an Alert. I quickly sketched here:
  

[almeidapaulooliveira]

I want to make sure that I'd using the right style of a notification (blue/info, rather than yellow/warning or other), that the text sounds right, and that the disabled fields are fine.

All looks fine.

[vdavid]

No, the site admin alert is not part of the scope. Please create a separate front-end issue about it if you think someone should fix it later.

Thanks for the review @almeidapaulooliveira!