chore: improve access service #4689

gastonfournier · 2023-09-13T15:06:14Z

About the changes

This enables us to use names instead of permission ids across all our APIs at the computational cost of searching for the ids in the DB but improving the API user experience

Open topics

We're using methods that are test-only and circumvent our business logic. This makes our test to rely on assumptions that are not always true because these assumptions are not validated frequently.

i.e. We are expecting that after removing a permission it's no longer there, but to test this, the permission has to be there before:

unleash/src/test/e2e/services/access-service.e2e.test.ts

Lines 367 to 375 in 78273e4

    
           await accessService.removePermissionFromRole( 
        
               editRole.id, 
        
               permissions.CREATE_FEATURE, 
        
               '*', 
        
           ); 
        
           expect( 
        
               await accessService.hasPermission(user, CREATE_FEATURE, 'some-project'), 
        
           ).toBe(false);

But it seems that's not the case.

We'll look into improving this later.

vercel · 2023-09-13T15:06:20Z

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

2 Ignored Deployments

Name	Status	Preview	Comments	Updated (UTC)
unleash-docs	⬜️ Ignored (Inspect)	Visit Preview		Sep 19, 2023 8:40am
unleash-monorepo-frontend	⬜️ Ignored (Inspect)	Visit Preview		Sep 19, 2023 8:40am

nunogois

Makes sense to me 🤷

src/test/e2e/services/access-service.e2e.test.ts

chriswk

Seems reasonable.

gastonfournier · 2023-09-15T09:59:26Z

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

                    await accessService.addPermissionToRole(
-                        role.id,
+                        builtInRole.id,


The test is adding a permission to the builtInRole, something that is not possible because of some validations at the service layer. Because the test is also creating a new role, I have the impression that the intention of the test is not to use the builtin role but the newly created one.

gastonfournier · 2023-09-15T10:00:03Z

src/lib/services/access-service.ts

                );
            } else {
+                // this branch uses id permissions


This is the reason why we're making this change, we want to be able to use names instead of ids for permissions

gastonfournier · 2023-09-15T10:00:38Z

src/lib/schema/role-schema.ts

+        id: joi.number(),
+        name: joi.string(),


This validation now expects that either you provide the id or the name (or both)

src/lib/db/access-store.ts

gastonfournier · 2023-09-15T10:09:04Z

src/lib/db/access-store.ts

+            environment: permissions.find(
+                (p) => p.name === (row.permission as string),
+            )?.environment,


I don't like the performance of this... a find inside a map is not ideal

Is it a problem? O(n*m) isn't an issue when n and m are small. Should be easy enough to build a Map earlier in the function and get O(1) lookups

Exactly what I had in mind

Assume positive intentions here, I'm struggling with formulating the question.
Why are we fetching ids again? Wasn't the goal to change to names as the primary key?

Wasn't the goal to change to names as the primary key?

Yes, but our OpenAPI spec states that ids are required while names are optional. Adding the name as required is a breaking change having to wait until the next major release. I think because of this reason we can't drop support for id's (customers might be using them already).

On another note, I think I solved the n*m problem with a map

I think this is correct, we can trade off some performance for correctness for a while. When we make a breaking change we can clean this up

A lot changed since this review

gastonfournier · 2023-09-15T14:30:27Z

src/test/fixtures/no-logger.ts

+// use for debugging only, try not to commit tests with verbose set to true
+noLoggerProvider.setVerbose = (beVerbose: boolean) => {
+    verbose = beVerbose;
+};
+


Unrelated, but I thought it might be useful

gastonfournier · 2023-09-15T15:54:31Z

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

-            req.user = await userService.createUser({
-                email: '[email protected]',
-                rootRole: role.id,
-            });


I've refactored this method, not sure what we were trying to do here:

Get Viewer built-in role

Create a user A

Create a new role "token creator" without permissions

Add CREATE_PROJECT permission to the Viewer role (shouldn't be possible to add permissions programmatically)

Add user A to the "token creator role" in project 'default' (now A has the root role Viewer and "token creator role" in 'default')

Create a new user B with root role Viewer (that now has the new permission)

Associate req.user to the newly created user B

Then the test tries to create an API token using the newly created user B.

Why did we take all those efforts to setup user A if we're not using it in the test? Apparently the only important steps are the ones in bold. At least taking into account the title of the test: A role with only CREATE_PROJECT_API_TOKEN can create project tokens

Guessing copy paste bug. Something something don't trust tests you haven't seen fail

gastonfournier · 2023-09-15T15:57:13Z

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

-                email: '[email protected]',
-                rootRole: role.id,
-            });
+            req.user = user;


On this side:

Get viewer role

Create a user with Viewer role as root role

Create a new role with CREATE_PROJECT_API_TOKEN permission

Add the user to the role created in default project

Associate req.user to the user created

Later we validate if this user can create an API token in the project where they have the role with that specific permission

src/lib/db/access-store.ts

src/lib/db/access-store.test.ts

gastonfournier · 2023-09-19T08:00:02Z

src/lib/db/access-store.test.ts

+    async (permissions) => {
+        const access = db.stores.accessStore as AccessStore;
+        const result = await access.resolvePermissions(permissions);
+        expect(result).toStrictEqual(permissions);


I'd love to say: no query to the DB here

gastonfournier · 2023-09-19T09:02:10Z

src/lib/db/access-store.ts

+        const rows = await this.db
+            .select('id', 'permission')
+            .from(T.PERMISSIONS)
+            .whereIn('permission', permissionNames);


Same query as here: https://github.com/Unleash/unleash/pull/4689/files#diff-5cb4eca71e6a03ec3d123986d657368a3386f80e986dfef2e3feb0abb0673d87L706-L709 but now it can be used in all scenarios of adding permissions to a role

sighphyre

LGTM, I think with the tests and timers this is good enough to try

This reverts commit 2186e2b.

Reverts #4689 temporarily to figure out what's the problem with the failing test

## About the changes In #4689 I forgot to add backward compatibility for a public method that was being used in Enterprise.

test: why does it fail?

4e0ba5a

github-actions bot assigned gastonfournier Sep 13, 2023

nunogois previously approved these changes Sep 13, 2023

View reviewed changes

nunogois reviewed Sep 13, 2023

View reviewed changes

src/test/e2e/services/access-service.e2e.test.ts Outdated Show resolved Hide resolved

Fix test

45c285e

chriswk previously approved these changes Sep 13, 2023

View reviewed changes

gastonfournier added 2 commits September 15, 2023 11:38

Add support for named permissions

f3aa636

Merge remote-tracking branch 'origin/main' into improve-access-service

7ae3f98

gastonfournier requested review from chriswk and nunogois September 15, 2023 09:52

Remove some test logs

16c6aac

vercel bot deployed to Preview – unleash-monorepo-frontend September 15, 2023 10:02 View deployment

gastonfournier commented Sep 15, 2023

View reviewed changes

Fix incompatibility with fake store

dd54d05

vercel bot deployed to Preview – unleash-monorepo-frontend September 15, 2023 10:05 View deployment

gastonfournier commented Sep 15, 2023

View reviewed changes

gastonfournier added 3 commits September 15, 2023 12:11

Fix compilation issue

c7b12a7

Merge remote-tracking branch 'origin/main' into improve-access-service

8583c22

Adapt to new interface

45dacbc

gastonfournier marked this pull request as ready for review September 15, 2023 10:24

gastonfournier requested a review from sighphyre September 15, 2023 10:25

Add tests to resolve permissions

7a61c4a

vercel bot deployed to Preview – unleash-monorepo-frontend September 15, 2023 14:24 View deployment

gastonfournier commented Sep 15, 2023

View reviewed changes

Remove unnecessary comment

67fcced

vercel bot deployed to Preview – unleash-monorepo-frontend September 15, 2023 17:48 View deployment

Merge remote-tracking branch 'origin/main' into improve-access-service

a90c83d

gastonfournier commented Sep 19, 2023

View reviewed changes

gastonfournier added 2 commits September 19, 2023 10:08

Update src/lib/db/access-store.test.ts

ca01f42

Update src/lib/db/access-store.test.ts

7b6596e

vercel bot deployed to Preview – unleash-monorepo-frontend September 19, 2023 08:12 View deployment

gastonfournier added 3 commits September 19, 2023 10:22

Merge remote-tracking branch 'origin/main' into improve-access-service

6d8831b

lint

84a17f9

Add timer to transform permission names to ids

d5c3402

gastonfournier commented Sep 19, 2023

View reviewed changes

sighphyre approved these changes Sep 19, 2023

View reviewed changes

gastonfournier merged commit 2186e2b into main Sep 19, 2023
7 checks passed

gastonfournier deleted the improve-access-service branch September 19, 2023 09:36

gastonfournier added a commit that referenced this pull request Sep 19, 2023

Revert "chore: improve access service (#4689)"

3b91a1b

This reverts commit 2186e2b.

gastonfournier mentioned this pull request Sep 19, 2023

Revert "chore: improve access service" #4773

Merged

gastonfournier added a commit that referenced this pull request Sep 19, 2023

Revert "chore: improve access service" (#4773)

12d9297

Reverts #4689 temporarily to figure out what's the problem with the failing test

gastonfournier mentioned this pull request Sep 19, 2023

chore: Improve access service iter 2 #4779

Merged

gastonfournier added a commit that referenced this pull request Sep 19, 2023

chore: Improve access service iter 2 (#4779)

bed0a29

## About the changes In #4689 I forgot to add backward compatibility for a public method that was being used in Enterprise.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: improve access service #4689

chore: improve access service #4689

gastonfournier commented Sep 13, 2023 •

edited

Loading

vercel bot commented Sep 13, 2023 •

edited

Loading

nunogois left a comment

chriswk left a comment

gastonfournier Sep 15, 2023

gastonfournier Sep 15, 2023

gastonfournier Sep 15, 2023

gastonfournier Sep 15, 2023

sighphyre Sep 15, 2023

gastonfournier Sep 15, 2023

chriswk Sep 15, 2023

gastonfournier Sep 15, 2023 •

edited

Loading

sighphyre Sep 19, 2023

gastonfournier Sep 15, 2023

gastonfournier Sep 15, 2023

sighphyre Sep 19, 2023

gastonfournier Sep 15, 2023

gastonfournier Sep 19, 2023

gastonfournier Sep 19, 2023

sighphyre left a comment

	await accessService.removePermissionFromRole(
	editRole.id,
	permissions.CREATE_FEATURE,
	'*',
	);

	expect(
	await accessService.hasPermission(user, CREATE_FEATURE, 'some-project'),
	).toBe(false);

chore: improve access service #4689

chore: improve access service #4689

Conversation

gastonfournier commented Sep 13, 2023 • edited Loading

About the changes

Open topics

vercel bot commented Sep 13, 2023 • edited Loading

nunogois left a comment

Choose a reason for hiding this comment

chriswk 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

gastonfournier Sep 15, 2023 • edited Loading

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

sighphyre left a comment

Choose a reason for hiding this comment

gastonfournier commented Sep 13, 2023 •

edited

Loading

vercel bot commented Sep 13, 2023 •

edited

Loading

gastonfournier Sep 15, 2023 •

edited

Loading