feat: Implement multi token support for client tokens #1476

sighphyre · 2022-04-06T06:20:03Z

This adds support for multi project tokens to be created. Backward compatibility is handled at 3 different layers here:

The API is made backwards compatible though a permissive data type that accepts either a project?: string or projects?: string[] property, validation is done through JOI here, which ensures that projects and project are not set together. In the case of neither, this defaults to the previous default of ALL_PROJECTS
The service layer method to handle adding tokens has been made tolerant to either of the above case and has been deprecated, a new method supporting only the new structure of using projects has been added
Existing compatibility for consumers of Unleash as a library should not be affected either, the ApiUser constructor is now tolerant to the the first input and will internally map to the new cleaned structure

This adds support for multi project tokens to be created. Backward compatibility is handled at 3 different layers here: - The API is made backwards compatible though a permissive data type that accepts either a project?: string or projects?: string[] property, validation is done through JOI here, which ensures that projects and project are not set together. In the case of neither, this defaults to the previous default of ALL_PROJECTS - The service layer method to handle adding tokens has been made tolerant to either of the above case and has been deprecated, a new method supporting only the new structure of using projects has been added - Existing compatibility for consumers of Unleash as a library should not be affected either, the ApiUser constructor is now tolerant to the the first input and will internally map to the new cleaned structure

vercel · 2022-04-06T06:20:06Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/unleash-team/unleash-docs/6ESBYF9gkHZTqwJCAAA1LVDmJ2mQ
✅ Preview: https://unleash-docs-git-feat-multi-project-tokens-unleash-team.vercel.app

github-actions · 2022-04-06T06:25:33Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟢	Statements	91.19%	5094/5586
🟡	Branches	79.88%	794/994
🟢	Functions	86.23%	1202/1394
🟢	Lines	91.12%	4978/5463

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / api-token.ts	97.67% (-2.33% 🔻)	95.45% (-4.55% 🔻)	100%	97.3% (-2.7% 🔻)
🟢	`...` / api-token-service.ts	90%	57.14% (+1.59% 🔼)	94.74%	89.86%

Test suite run success

842 tests passing in 121 suites.

Report generated by 🧪jest coverage report action from e889d8e

ivarconr · 2022-04-07T19:04:40Z

src/migrations/20220331085057-add-api-link-table.js

+            secret     text NOT NULL,
+            project    text NOT NULL,
+            FOREIGN KEY (secret) REFERENCES api_tokens (secret) ON DELETE CASCADE,
+            FOREIGN KEY (project) REFERENCES projects(id) ON DELETE CASCADE


will this automatically remove the project from the token if the project is removed?
If yes: Awesome!

Yeah it will!

Cascade does rule :)

ivarconr · 2022-04-07T19:12:19Z

src/lib/db/api-token-store.ts

+                'seen_at',
+                'environment',
+                'token_project_link.project',
+            );


I am pretty sure this query can be written to actually return the list of projects as a string array directly from postgres using the "ARRAY_AGG" function. I am not sure if it is more efficient, but would at least avoid the need for the reducer processing the items.

Oh cool, I didn't actually know about ARRAY_AGG, that's super cool. I'll have to shuffle the query around but this shouldn't be a very hot path/large dataset so the readability wins out here for me anyway, thanks!

ivarconr · 2022-04-07T19:17:15Z

src/lib/db/api-token-store.ts

+                        [newToken.secret, project],
+                    );
+                });
+            await Promise.all(updateProjectTasks);


Yo do not need to create a bunch of insert request. You just need to to map the projects in it to a list of "objects" matching the "API_LINK_TABLE" shape and perform a simple knex insert on all the values.

What Ivar said, knex already has efficient batch insert, write the query, give a list of tuples with the same amount of args as your sql insert. See src/lib/db/clientMetricsStore-v2.ts:101 for instance.

Though this seems to have the advantage that it's transactional and as such will rollback the LINK_TABLE inserts if any of them fail.

that can be achieved anyway, and yes the transaction stuff is the important part here!

src/lib/schema/api-token-schema.test.ts

src/lib/types/api-user.ts

chriswk

Great thorough PR, I think Ivar's comments warrants a check to see if it's more efficient, but this LGTM :)

chriswk · 2022-04-08T07:08:55Z

src/lib/services/api-token-service.ts

        const randomStr = crypto.randomBytes(28).toString('hex');
-        return `${project}:${environment}.${randomStr}`;
+        if (projects.length > 1) {
+            return `[]:${environment}.${randomStr}`;


So multi project tokens now gets resolved from the db, fair enough :)

As opposed to not being supported at all, don't get me wrong :)

Yeah, it's a big tradeoff here. All the other token types can be determined just from their secret signature, this is the first time they're no longer stateful. Means we can't yeet them from Unleash instance to Unleash instance unfortunately but it keeps the token small

chriswk · 2022-04-08T07:11:28Z

src/migrations/20220331085057-add-api-link-table.js

+    );
+};
+
+//This is a lossy down migration, tokens with multiple projects are discarded


That's fair, I'd be a bit surprised if I had multple project tokens and then ended up with separate tokens for each project if I reverted the thing that allowed me to have a token accessing multiple projects.

My thought too! I absolutely don't want them getting more permissive, and if you make them less permissive it's just weird. Clearly to destroy for everyone involved

chriswk · 2022-04-08T07:22:58Z

src/lib/create-config.ts

@@ -198,7 +202,7 @@ const loadTokensFromString = (tokenString: String, tokenType: ApiTokenType) => {
            type: tokenType,
            username: 'admin',
        };
-        validateApiToken(token);
+        validateApiToken(mapLegacyToken(token));


Ok, so, we won't have support for adding multiproject tokens from environment variables for this release.

That is correct! We need to come up with a separate format for the env vars, currently they're determined from the secret signature in this code path. The secret doesn't contain all the info needed for a multiproject. I think something like this would work:

We can read that string, calculate the projects and then strip them

chriswk · 2022-04-08T07:27:35Z

src/lib/db/api-token-store.ts

+                        [newToken.secret, project],
+                    );
+                });
+            await Promise.all(updateProjectTasks);


What Ivar said, knex already has efficient batch insert, write the query, give a list of tuples with the same amount of args as your sql insert. See src/lib/db/clientMetricsStore-v2.ts:101 for instance.

Though this seems to have the advantage that it's transactional and as such will rollback the LINK_TABLE inserts if any of them fail.

chriswk · 2022-04-08T07:29:45Z

src/migrations/20220331085057-add-api-link-table.js

+            secret     text NOT NULL,
+            project    text NOT NULL,
+            FOREIGN KEY (secret) REFERENCES api_tokens (secret) ON DELETE CASCADE,
+            FOREIGN KEY (project) REFERENCES projects(id) ON DELETE CASCADE


Cascade does rule :)

sighphyre requested review from ivarconr and chriswk April 6, 2022 06:21

ivarconr reviewed Apr 7, 2022

View reviewed changes

src/lib/schema/api-token-schema.test.ts Show resolved Hide resolved

ivarconr reviewed Apr 7, 2022

View reviewed changes

src/lib/types/api-user.ts Show resolved Hide resolved

chriswk approved these changes Apr 8, 2022

View reviewed changes

sighphyre merged commit c9b44b6 into main Apr 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement multi token support for client tokens #1476

feat: Implement multi token support for client tokens #1476

sighphyre commented Apr 6, 2022

vercel bot commented Apr 6, 2022 •

edited

github-actions bot commented Apr 6, 2022

ivarconr Apr 7, 2022

sighphyre Apr 8, 2022

chriswk Apr 8, 2022

ivarconr Apr 7, 2022

sighphyre Apr 8, 2022

ivarconr Apr 7, 2022

chriswk Apr 8, 2022

ivarconr Apr 8, 2022

chriswk left a comment

chriswk Apr 8, 2022

chriswk Apr 8, 2022

sighphyre Apr 8, 2022

chriswk Apr 8, 2022

sighphyre Apr 8, 2022

chriswk Apr 8, 2022

sighphyre Apr 8, 2022

chriswk Apr 8, 2022

chriswk Apr 8, 2022

feat: Implement multi token support for client tokens #1476

feat: Implement multi token support for client tokens #1476

Conversation

sighphyre commented Apr 6, 2022

vercel bot commented Apr 6, 2022 • edited

github-actions bot commented Apr 6, 2022

Coverage report

Test suite run success

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Apr 6, 2022 •

edited