[services] Switch to role based auth #15200

cjllanwarne · 2025-11-20T23:02:04Z

Change Description

Updates UI and backend to update to use the system_roles table for system-level UI and functionality auth.

NB this leaves the previous is_developer field in the database, in case we need to revert back, but hopefully we can remove that too in an upcoming PR.

Security Assessment

This change potentially impacts the Hail Batch instance as deployed by Broad Institute in GCP

Impact Rating

This change has a high security impact

Impact Description

Refactors the simple is_developer auth model into one based on roles and permissions. Allows intermediate roles between "everything" and "end user", eg for accessing billing pages or developer namespaces without being a full system admin.

Appsec Review

Required: The impact has been assessed and approved by appsec

cjllanwarne · 2025-11-21T17:18:50Z

auth/sql/012-refactor-system-permissions-2.sql

This should be merged with 011 and done in one go. It's a side effect of development milestones and not worth preserving in the final commit

Copilot

Pull request overview

This PR refactors the authentication and authorization system from a simple is_developer boolean flag to a comprehensive role-based access control (RBAC) system using system roles and permissions. This enables more granular access control with intermediate roles between full system admin and end user.

Key Changes:

Introduces new database tables for system_roles, system_permissions, system_role_permissions, and users_system_roles
Replaces is_developer checks throughout the codebase with permission-based checks
Updates API endpoints to use new permission decorators (e.g., @auth.authenticated_users_with_permission())
Migrates UI templates to conditionally render elements based on user permissions

Reviewed changes

Copilot reviewed 36 out of 36 changed files in this pull request and generated 17 comments.

Show a summary per file

File	Description
auth/sql/011-refactor-system-permissions.sql	Adds new CI and deployed system state permissions, creates sysadmin-readonly role, and migrates existing users to new role system
auth/sql/estimated-current.sql	Updates schema to include new system_roles, system_permissions, and relationship tables
auth/auth/auth.py	Refactors user creation and management to use system roles instead of is_developer flag; adds permission checking endpoints
auth/auth/driver/driver.py	Updates user creation/deletion logic to check for access_developer_environments permission
auth/auth/templates/users.html	Updates user management UI to support multiple system roles via checkboxes
auth/auth/templates/roles.html	Adds conditional rendering for role assignment buttons based on assign_system_roles permission
auth/auth/templates/openapi.yaml	Updates API spec to reflect system_roles array replacing is_developer boolean
auth/auth/exceptions.py	Adds InvalidRole exception for unknown role validation
gear/gear/auth.py	Replaces authenticated_developers_only decorator with authenticated_users_with_permission; updates UserData type
gear/gear/init.py	Exports SystemPermission (duplicate entry)
gear/gear/system_permissions.py	Adds new permission types for CI, deployed system state, and developer activities
web_common/web_common/web_common.py	Updates Jinja context retrieval to check READ_PRERENDERED_JINJA2_CONTEXT permission
web_common/web_common/templates/new_header.html	Updates navigation menu to show/hide sections based on user permissions
web_common/web_common/templates/header.html	Updates legacy header to use permission-based visibility
batch/batch/front_end/front_end.py	Replaces is_developer checks with permission-based authorization for billing operations
batch/batch/front_end/templates/billing*.html	Conditionally renders billing management UI based on user permissions
batch/batch/driver/main.py	Updates driver endpoints to require specific system permissions
batch/batch/driver/templates/*.html	Adds permission guards for system state modification UI elements
ci/ci/ci.py	Replaces authenticated_developers_only with permission-based decorators for CI operations
ci/ci/templates/*.html	Conditionally renders CI management controls based on manage_ci permission
ci/bootstrap_create_accounts.py	Updates bootstrap user creation to use system_roles instead of is_developer
monitoring/monitoring/monitoring.py	Requires VIEW_MONITORING_DASHBOARDS permission for billing endpoints
gateway/envoy.yaml	Updates authorization path to verify access_developer_environments permission
grafana/deployment.yaml	Updates authorization path to verify view_monitoring_dashboards permission
prometheus/prometheus.yaml	Updates authorization path to verify view_monitoring_dashboards permission
hail/python/hailtop/auth/auth.py	Updates async_create_user to accept system_roles list instead of is_developer boolean
hail/python/hailtop/hailctl/auth/*.py	Updates CLI commands to support system_roles parameter
hail/python/hailtop/hail_logging.py	Logs system_permissions instead of is_developer
devbin/dev_proxy.py	Updates dev proxy to set system_roles instead of is_developer
build.yaml	Adds migration step for refactor-system-permissions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-05T20:43:51Z

auth/auth/auth.py

            """
-INSERT INTO users (state, username, login_id, is_developer, is_service_account, hail_identity, hail_credentials_secret_name)
+INSERT INTO users (state, username, login_id, is_service_account, hail_identity, hail_credentials_secret_name)
 VALUES (%s, %s, %s, %s, %s, %s, %s);


The INSERT statement is missing a value placeholder. There are 7 placeholders (%s) but only 6 values in the tuple (missing the value for is_service_account between login_id and hail_identity). The statement should have 6 placeholders to match the 6 columns.

Suggested change

VALUES (%s, %s, %s, %s, %s, %s, %s);

VALUES (%s, %s, %s, %s, %s, %s);

Copilot · 2025-12-05T20:43:52Z

ci/ci/ci.py

    )
-    app[AppKeys.DEVELOPERS] = [u for u in users if u['is_developer'] == 1 and u['state'] == 'active']
+    app[AppKeys.DEVELOPERS] = [
+        u for u in users if SystemPermission.MANAGE_CI in u['system_permissions'] and u['state'] == 'active'


Checking for membership of SystemPermission.MANAGE_CI in u['system_permissions'] is incorrect. Based on the UserData type definition and how permissions are fetched in auth.py, system_permissions is a dictionary mapping permission names to booleans (e.g., {'manage_ci': True}), not a list of SystemPermission enums. This should be:

u for u in users if u['system_permissions'].get('manage_ci', False) and u['state'] == 'active'

or

u for u in users if u['system_permissions'].get(SystemPermission.MANAGE_CI, False) and u['state'] == 'active'

Suggested change

u for u in users if SystemPermission.MANAGE_CI in u['system_permissions'] and u['state'] == 'active'

u for u in users if u['system_permissions'].get('manage_ci', False) and u['state'] == 'active'

Copilot · 2025-12-05T20:43:52Z

web_common/web_common/templates/header.html

  {% endif %}

-  {% if userdata['is_developer'] == 1 %}
+  {% if userdata['system_permissions']['view_monitoring_dashboards'] %}


Missing null check for userdata. Line 59 checks userdata['system_permissions'][...] without first verifying userdata exists. This should include a null check:

{% if userdata and userdata['system_permissions']['view_monitoring_dashboards'] %}

Copilot · 2025-12-05T20:43:52Z

auth/auth/auth.py

    db = request.app[AppKeys.DB]
-    users = [x async for x in db.select_and_fetchall('SELECT * FROM users;')]
+    users = await _get_users(db)
    page_context = {'users': users}


The page context is missing the system_roles variable that is used in the template at line 29. The template expects system_roles to be available to render the role checkboxes in the create user form. You should add something like:

system_roles = [x async for x in db.select_and_fetchall('SELECT name FROM system_roles ORDER BY name;')] page_context = {'users': users, 'system_roles': [r['name'] for r in system_roles]}

Suggested change

page_context = {'users': users}

system_roles = [x async for x in db.select_and_fetchall('SELECT name FROM system_roles ORDER BY name;')]

page_context = {'users': users, 'system_roles': [r['name'] for r in system_roles]}

Copilot · 2025-12-05T20:43:52Z

auth/auth/auth.py

+    system_roles_raw = post.get('system_roles', [])
+    if isinstance(system_roles_raw, list):
+        system_roles = [str(role) for role in system_roles_raw]
+    elif system_roles_raw:
+        raise web.HTTPBadRequest(text='Invalid system_roles value. Must be a list of role name strings.')
+    else:
+        system_roles = []


The handling of system_roles from POST data needs adjustment. HTML forms with checkboxes send the name multiple times for multiple selections, so post.get('system_roles') won't work correctly. You should use post.getall('system_roles[]') to get all selected values. The current code will only get one value or a list depending on how the form is submitted.

Suggested change

system_roles_raw = post.get('system_roles', [])

if isinstance(system_roles_raw, list):

system_roles = [str(role) for role in system_roles_raw]

elif system_roles_raw:

raise web.HTTPBadRequest(text='Invalid system_roles value. Must be a list of role name strings.')

else:

system_roles = []

system_roles_raw = post.getall('system_roles[]')

system_roles = [str(role) for role in system_roles_raw]

Copilot · 2025-12-05T20:43:55Z

auth/auth/driver/driver.py

        ident_token = f'{username}-{token}'

-    if user['is_developer'] == 1 or user['is_service_account'] == 1 or username == 'test':
+    if 'access_developer_environments' in user['system_roles'] or user['is_service_account'] == 1 or username == 'test':


The condition checks 'access_developer_environments' in user['system_roles'] but based on the _users_in_state_with_roles function in driver.py, user['system_roles'] is a list of role names (e.g., ['developer', 'billing_manager']), not permission names. To check for a permission, you need to check if the user has a role that grants that permission. Consider using a permission-based check or verifying that the 'developer' role is in user['system_roles'] instead.

Copilot · 2025-12-05T20:43:55Z

auth/auth/driver/driver.py

    # auth services in test namespaces cannot/should not be creating and deleting namespaces
    if namespace_name is not None and namespace_name != DEFAULT_NAMESPACE and not is_test_deployment:
-        assert user['is_developer'] == 1
+        if 'access_developer_environments' not in user['system_roles']:


Same issue: checking for 'access_developer_environments' in user['system_roles'] when system_roles contains role names, not permission names.

Copilot · 2025-12-05T20:43:55Z

web_common/web_common/web_common.py

 ) -> web.Response:
    if request.headers.get('x-hail-return-jinja-context'):
-        if userdata and userdata['is_developer']:
+        if userdata and userdata['system_permissions'][SystemPermission.READ_PRERENDERED_JINJA2_CONTEXT]:


Missing null check for userdata before accessing system_permissions. While other conditionals in this file check userdata and userdata['system_permissions'][...], this line only checks the permission without first verifying userdata exists. This should be:

if userdata and userdata['system_permissions'][SystemPermission.READ_PRERENDERED_JINJA2_CONTEXT]:

Copilot · 2025-12-05T20:43:56Z

web_common/web_common/templates/header.html

  </a>

-  {% if userdata['is_developer'] == 1 %}
+  {% if userdata['system_permissions']['read_system_roles'] or userdata['system_permissions']['read_users'] %}


Missing null check for userdata. Line 6 checks userdata['system_permissions'][...] but other similar checks in header.html check userdata['system_permissions'][...] without first verifying userdata exists. This should be:

{% if userdata and userdata['system_permissions']['read_system_roles'] or userdata and userdata['system_permissions']['read_users'] %}

Copilot · 2025-12-05T20:43:56Z

web_common/web_common/templates/header.html

      <div class="header-dropdown-menu">
        <div class="batch-caret header-dropdown-menu-caret"></div>
-        {% if userdata['is_developer'] == 1 %}
+        {% if userdata['system_permissions']['read_deployed_system_state'] %}


Missing null check for userdata. Line 28 checks userdata['system_permissions'][...] without first verifying userdata exists, unlike line 31 and other conditionals. This should include a null check:

{% if userdata and userdata['system_permissions']['read_deployed_system_state'] %}

cjllanwarne added 18 commits November 19, 2025 11:13

switch to role based ui header menus

5c32cac

status=>state

488abe4

gate users page display behind permissions

1c19683

read/write permissions on roles page

2cc4c7a

Update driver info page with readonly permissions

997c338

readonly pools UI

eaee805

permissioning on job private pools and billing projects

91ba718

permissioning on ci pages

935000e

system state management for admins not devs

3c605d3

system state management for admins not devs

b825ffd

monitoring is an admin action

b4deb89

namespace management is an admin permission

ed1122b

consolidate migrations

e13ae88

WIP

be8cb72

WIP

abbe606

WIP

5855097

WIP

ec0c001

no more is_developer

b0c0e4d

cjllanwarne requested a review from a team as a code owner November 20, 2025 23:02

cjllanwarne added 2 commits November 21, 2025 11:37

fix bootstrap, and correct driver user creation

496bbbd

avoid mutable default value

8e45974

cjllanwarne commented Nov 21, 2025

View reviewed changes

cjllanwarne added 8 commits November 21, 2025 17:43

fix ticks

fda94a4

unused userdat

0877cac

merge refactor-system-permission changesets

b0b05a5

oops

877ff66

fix migration script

f6aba00

switch away from verify_dev

aa4b9dc

fix tupling issue for a single value

a0d0806

fixup permission verifying endpoints again

3e5dafd

cjllanwarne added 5 commits December 5, 2025 14:26

support empty userdata in header

6142a06

fix table name

fa4779d

fix users query

c5b6307

fix UI billing project lookup permissions

4d2bf71

fix permission access check

781f11f

cjllanwarne requested a review from Copilot December 5, 2025 20:33

Copilot started reviewing on behalf of cjllanwarne December 5, 2025 20:33 View session

Copilot AI reviewed Dec 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[services] Switch to role based auth #15200

[services] Switch to role based auth #15200

cjllanwarne commented Nov 20, 2025

Uh oh!

cjllanwarne Nov 21, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Copilot AI Dec 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	VALUES (%s, %s, %s, %s, %s, %s, %s);
	VALUES (%s, %s, %s, %s, %s, %s);

	u for u in users if SystemPermission.MANAGE_CI in u['system_permissions'] and u['state'] == 'active'
	u for u in users if u['system_permissions'].get('manage_ci', False) and u['state'] == 'active'

	page_context = {'users': users}
	system_roles = [x async for x in db.select_and_fetchall('SELECT name FROM system_roles ORDER BY name;')]
	page_context = {'users': users, 'system_roles': [r['name'] for r in system_roles]}

[services] Switch to role based auth #15200

Are you sure you want to change the base?

[services] Switch to role based auth #15200

Conversation

cjllanwarne commented Nov 20, 2025

Change Description

Security Assessment

Impact Rating

Impact Description

Appsec Review

Uh oh!

cjllanwarne Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant