refactor: token permissions, drop admin-like permissions #4050

nunogois · 2023-06-21T14:37:42Z

https://linear.app/unleash/issue/2-1155/refactor-permissions

Our rbac-middleware now supports multiple OR permissions;
Drops non-specific permissions (e.g. CRUD API token permissions without specifying the token type);
Makes our permission descriptions consistent;
Drops our higher-level permissions that basically mean ADMIN (e.g. ADMIN token permissions) in favor of ADMIN permission in order to avoid privilege escalations;

This PR may help with https://linear.app/unleash/issue/2-1144/discover-potential-privilege-escalations as it may prevent privilege escalations altogether.

There's some UI permission logic around this, but in the future https://linear.app/unleash/issue/2-1156/adapt-api-tokens-creation-ui-to-new-permissions could take it a bit further by adapting the creation of tokens as well.

vercel · 2023-06-21T14:37:47Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
unleash-monorepo-frontend	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 21, 2023 5:29pm

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
unleash-docs	⬜️ Ignored (Inspect)			Jun 21, 2023 5:29pm

gastonfournier

I might be missing some details, I think this looks good but there are a few things I'm struggling to understand, so let's take a look at them tomorrow ;)

gastonfournier · 2023-06-21T14:44:31Z

src/lib/routes/admin-api/api-token.ts

@@ -106,7 +91,7 @@ const tokenTypeToUpdatePermission: (tokenType: ApiTokenType) => string = (
 ) => {
    switch (tokenType) {
        case ApiTokenType.ADMIN:
-            return UPDATE_ADMIN_API_TOKEN;
+            return ADMIN;


This allowed customers to have fine-grained control over permissions, I'm just trying to play devil's advocate here. But before admins could remove the admin permission to update an admin token, but now it's all or nothing, right? I'm wondering if we should keep the fine-grained permissions

This permission was only introduced in #4019 (yesterday) so it's not like these are breaking changes.
I don't think there's a use case for someone that has access to admin tokens but is not an admin themselves, as they could simply create an admin API token and override the specific permissions anyways.

Basically what I meant by:

Drops our higher-level permissions that basically mean ADMIN (e.g. ADMIN token permissions) in favor of ADMIN permission in order to avoid privilege escalations;

gastonfournier · 2023-06-21T15:06:59Z

src/migrations/20230621141239-refactor-api-token-permissions.js

+    DELETE FROM permissions WHERE permission IN ('CREATE_API_TOKEN', 'UPDATE_API_TOKEN', 'DELETE_API_TOKEN', 'READ_API_TOKEN');
+    DELETE FROM permissions WHERE permission = 'UPDATE_ROLE';
+    DELETE FROM permissions WHERE permission IN ('CREATE_ADMIN_API_TOKEN', 'UPDATE_ADMIN_API_TOKEN', 'DELETE_ADMIN_API_TOKEN', 'READ_ADMIN_API_TOKEN');


I would not delete these permissions just yet to prevent potential backward compatibility problems. Note that the old version will still be running while these permissions are deleted

This was only added a few versions back 3acb116#diff-d9d1a3ae5564ee96665ff5c5d42f10debedce779340e421d55dc2f3de9b3117fR4-R19 but this could be already deployed to customers. I'd just open a new PR after this one gets merged and deployed

Reminder that ADMIN overrides this anyways and custom root roles are not available for customers yet, but if we feel safer only adding a migration with these deletions after we make sure the code changes are there, we can do that.

Addressed in: dd2ac48

We should add this in a later PR:

DELETE FROM permissions WHERE permission IN ('CREATE_API_TOKEN', 'UPDATE_API_TOKEN', 'DELETE_API_TOKEN', 'READ_API_TOKEN'); DELETE FROM permissions WHERE permission = 'UPDATE_ROLE'; DELETE FROM permissions WHERE permission IN ('CREATE_ADMIN_API_TOKEN', 'UPDATE_ADMIN_API_TOKEN', 'DELETE_ADMIN_API_TOKEN', 'READ_ADMIN_API_TOKEN');

gastonfournier · 2023-06-21T15:16:29Z

frontend/src/component/admin/apiToken/CreateApiToken/CreateApiToken.tsx

@@ -107,7 +105,7 @@ export const CreateApiToken = ({ modal = false }: ICreateApiTokenProps) => {
                handleSubmit={handleSubmit}
                handleCancel={handleCancel}
                mode="Create"
-                actions={<CreateButton name="token" permission={permission} />}
+                actions={<CreateButton name="token" permission={ADMIN} />}


Does this apply for all types of API tokens? Or just admin tokens?

All types of tokens currently. It's essentially the same behavior as before: Only ADMINs are able to create API tokens, except now we're being specific about it instead of pointing to a permission that only ADMINs have anyways.

Of course with the new permissions and custom root roles you're now able to create tokens of a specific type if you have that permission, but only on the API level, it's not reflected on the UI yet.

This is what I meant in:

There's some UI permission logic around this, but in the future https://linear.app/unleash/issue/2-1156/adapt-api-tokens-creation-ui-to-new-permissions could take it a bit further by adapting the creation of tokens as well.

It's essentially the same behavior as before: Only ADMINs are able to create API tokens, except now we're being specific about it instead of pointing to a permission that only ADMINs have anyways.

Thanks! Cristal clear!

gastonfournier · 2023-06-21T15:18:47Z

frontend/src/component/admin/apiToken/ApiTokenPage/ApiTokenPage.tsx

@@ -67,7 +88,7 @@ export const ApiTokenPage = () => {
                                    />
                                    <PageHeader.Divider />
                                    <CreateApiTokenButton
-                                        permission={CREATE_API_TOKEN}
+                                        permission={ADMIN}


Is only admin able to create new ones? What if we want to assign the permission of creating new client tokens to a service account?

They can through the API. Same as #4050 (comment)

src/lib/routes/admin-api/api-token.ts

gastonfournier · 2023-06-21T15:22:56Z

src/test/e2e/api/admin/api-token.auth.e2e.test.ts

@@ -490,11 +465,11 @@ describe('Fine grained API token permissions', () => {
                });
            await destroy();
        });
-        test('READ_ADMIN_API_TOKEN should be able to see ADMIN tokens', async () => {
+        test('ADMIN should be able to see ADMIN tokens', async () => {


I think we can add the opposite test as well: VIEWER or EDITOR (you pick) cannot see ADMIN tokens

Sure, good point 🙂

Addressed in 42280e9

Co-authored-by: Gastón Fournier <gaston@getunleash.io>

gastonfournier

Well done! Especially clarifying doubts 🎯 and adding more tests 🤖! LGTM!

nunogois requested a review from gastonfournier June 21, 2023 14:37

github-actions bot assigned nunogois Jun 21, 2023

gastonfournier reviewed Jun 21, 2023

View reviewed changes

nunogois requested a review from chriswk June 21, 2023 16:34

vercel bot deployed to Preview – unleash-monorepo-frontend June 21, 2023 16:49 View deployment

nunogois and others added 2 commits June 21, 2023 17:51

refactor: token permissions, drop admin-like permissions

c7fc111

Update src/lib/routes/admin-api/api-token.ts

2080cdd

Co-authored-by: Gastón Fournier <gaston@getunleash.io>

nunogois force-pushed the refactor-permissions branch from 798828e to 2080cdd Compare June 21, 2023 16:51

vercel bot deployed to Preview – unleash-monorepo-frontend June 21, 2023 16:54 View deployment

test: adapt read > admin, add read > editor

42280e9

vercel bot deployed to Preview – unleash-monorepo-frontend June 21, 2023 17:10 View deployment

fix: remove DB deletes for now

dd2ac48

vercel bot deployed to Preview – unleash-monorepo-frontend June 21, 2023 17:11 View deployment

test: fix NoAccessError tests

f19597f

vercel bot deployed to Preview – unleash-monorepo-frontend June 21, 2023 17:24 View deployment

test: fix checkRbac permission argument

d52aedc

vercel bot deployed to Preview – unleash-monorepo-frontend June 21, 2023 17:29 View deployment

gastonfournier approved these changes Jun 22, 2023

View reviewed changes

nunogois merged commit 7e9069e into main Jun 22, 2023
17 checks passed

nunogois deleted the refactor-permissions branch June 22, 2023 07:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: token permissions, drop admin-like permissions #4050

refactor: token permissions, drop admin-like permissions #4050

nunogois commented Jun 21, 2023 •

edited

vercel bot commented Jun 21, 2023 •

edited

gastonfournier left a comment

gastonfournier Jun 21, 2023

nunogois Jun 21, 2023

gastonfournier Jun 21, 2023

gastonfournier Jun 21, 2023

nunogois Jun 21, 2023

nunogois Jun 21, 2023

gastonfournier Jun 21, 2023

nunogois Jun 21, 2023

gastonfournier Jun 22, 2023

gastonfournier Jun 21, 2023

nunogois Jun 21, 2023

gastonfournier Jun 21, 2023

nunogois Jun 21, 2023

nunogois Jun 21, 2023

gastonfournier left a comment

refactor: token permissions, drop admin-like permissions #4050

refactor: token permissions, drop admin-like permissions #4050

Conversation

nunogois commented Jun 21, 2023 • edited

vercel bot commented Jun 21, 2023 • edited

gastonfournier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gastonfournier left a comment

Choose a reason for hiding this comment

nunogois commented Jun 21, 2023 •

edited

vercel bot commented Jun 21, 2023 •

edited