Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

WIP: remove dependency on users.site_admin column when checking for site_admin status#58114

Draft
BolajiOlajide wants to merge 3 commits into
mainfrom
bo/cc/rbac-site-admin-check
Draft

WIP: remove dependency on users.site_admin column when checking for site_admin status#58114
BolajiOlajide wants to merge 3 commits into
mainfrom
bo/cc/rbac-site-admin-check

Conversation

@BolajiOlajide

@BolajiOlajide BolajiOlajide commented Nov 6, 2023

Copy link
Copy Markdown
Contributor

Closes #51153

#43276 introduced a better way to assign user roles on a Sourcegraph instance. Before #43276, we only had two types of uses:

  • Site Admins
  • Non-Site Admins

With a proper RBAC system in place, we can have different types of roles assigned to a user; however, we've been relying on the site_admin column on the users table, which isn't the right way to determine the SiteAdmin status of a user.

In this PR, I've removed all references to the users.site_admin column and ensured that site_admin status is computed based on the user's roles. This PR doesn't remove the site_admin column from the users table (this will be done in a follow-up PR).

The query for fetching user information has been updated to reference the user_roles and roles table to figure out the site_admin status of a user. That means the query for fetching a user changed from

SELECT  
	id, 
	username, 
	avatar_url, 
	deleted_at, 
	site_admin 
FROM users 
WHERE deleted_at IS NULL;

to

SELECT 
	id, 
	username, 
	avatar_url, 
	deleted_at, 
	EXISTS(
		SELECT 1 FROM user_roles 
		JOIN roles ON roles.id = user_roles.role_id 
		WHERE user_roles.user_id = users.id AND roles.name = 'SITE_ADMINISTRATOR'
	) AS site_admin 
FROM users 
WHERE deleted_at IS NULL;

This ensures we remove the dependency on users.site_admin.

Test plan

  • Ensure all existing tests are passing.
  • Temporarily removed the users.site_admin column and ensured tests were passing.
  • Manually tested on my local instance.

Follow-up:

There'll be a follow-up PR to remove the users.site_admin column in the next non-patch release.

@camdencheek

Copy link
Copy Markdown
Member

@BolajiOlajide where are we at on this PR? IIRC, it was pretty close where we left offa

@BolajiOlajide

Copy link
Copy Markdown
Contributor Author

@BolajiOlajide where are we at on this PR? IIRC, it was pretty close where we left offa

I just need to fix the tests. I'll prioritize this before I leave for vacation.

@BolajiOlajide BolajiOlajide force-pushed the bo/cc/rbac-site-admin-check branch from 1bb5820 to eef8ee6 Compare December 19, 2023 18:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deprecate user.site_admin row in favor of SITE_ADMINISTRATOR read-only role

2 participants