Projects in Studio #2078

emranemran · 2024-02-27T01:19:03Z

Initial set of changes from myself + @mjh1 to enable "Project" environments in Studio:

Added boiler plate schema changes for project tables
Added project.ts to handle creation/retrieval of projects
Updated asset.ts to use projectId logic where applicable

A few notes:

I'm would like to merge changes as we go so that it's not one giant PR at the end that's difficult to review -- so we're making sure that projectId is not enforced anywhere right now so that we don't break backwards compatibility. Any records with an empty or missing projectId will get categorized as default project in the dashboard.
I'm opening up this PR early to collect feedback in case we're going down the wrong path. Please review the changes and suggest anything that might have been missed.
TODO: fix/add tests...

PS: this is my first large typrscript PR so be gentle :)

vercel · 2024-02-27T01:19:09Z

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

Name	Status	Preview	Comments	Updated (UTC)
livepeer-studio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Mar 7, 2024 9:53pm

packages/api/src/controllers/project.ts

leszko

@emranemran @mjh1 kudos for sending the PR early.

I didn't manage to review it all, but I added some first comments. PTAL.

leszko · 2024-02-27T14:51:16Z

packages/api/src/controllers/asset.ts

@@ -200,6 +202,13 @@ export async function validateAssetPayload(
    }
  }

+  if (payload.projectId) {
+    const project = await getProject(req);


I wonder if we shouldn't add project to req in

studio/packages/api/src/middleware/auth.ts

Line 79 in 95e12f9

function authenticator(): RequestHandler {

My thinking is that now, authenticator() assigns user to each request. But from now on, the request (api token) will be associated always with one project, so maybe we should inject it in authenticator() and then there would be no need to call getProject() everywhere?

@victorges and I discussed something like that but Victor suggested we need to maintain compatibility with the SDK I believe. I'm still a bit on the fence on this b/c from the dashboard perspective, project is basically a "filter" to categorize the assets, streams, etc. But maybe philosophically what you're saying makes sense. wdyt @victorges?

I think we can still maintain compatibility but having the projectId added to the request as Rafal suggests will just neaten the code up a bit. User and project would both exist on the request object then, and we can maintain compatibility by checking both in the controllers.

The other thing is we don't want projectId to be part of the payload, we should be taking it from the api key being used. We could potentially leave that for a later PR though and leave this one mainly as just adding the projects controller.
Edit: I'm working on this bit locally atm, including the authenticator() rafal mentioned (branched off this), let me know if you want to pair etc WIP: #2081

@mjh1: I saw your edit comment too late and started making simliar changes to my branch - but I scrapped those and just cherry-picked yours here instead. Could we use this branch to pair for integration? I added another commit to make some edits but currently a bit broken when trying to create new api-keys -- see my comment here: #2078 (comment)

Guys, I think you're on the right track!

Wrt what @mjh1 wrote, I think that we should not send projectId in the payload, because projectId is derived from API TOKEN. The same way, we don't send userId in every request.

Ok cool yep let's just use this branch

leszko · 2024-02-27T14:54:45Z

packages/api/src/controllers/asset.ts

@@ -654,6 +674,14 @@ app.get("/", authorizer({}), async (req, res) => {
    query.push(sql`asset.data->>'deleted' IS NULL`);
  }

+  if (projectId) {


If we filter by projectId, do we also need to filter by userId in the line 683?

Yes, don't we? We're trying to get all assets beloniging to a specific userId broken down by a specific projectId.

Doesn't one project belong to one user only? Then, filtering by projectId should be enough.

I don't think projectId should be its own queryparam like this, we can add it to the fieldsMap instead. I also think we should treat it similar to userId in that only admins are allowed to use it. For non-admins we should just take the projectId associated with the api key being used.

leszko · 2024-02-27T15:22:47Z

packages/api/src/controllers/project.ts

+    if (count) {
+      fields = fields + ", count(*) OVER() AS count";
+    }
+    const from = `project left join users on project.data->>'userId' = users.id`;


Do you think it would be possible to put the SQL queries inside the table? I know it's the same in other controllers, but I wonder if it would make sense to abstract it and put inside the store/*-table.ts?

We could do that - I just followed the existing patterns in other files.

Ok, that's probably fine, we can try to clean it up for all controllers in some other PR.

leszko

Looks good from my end. You can decide with @victorges on the open discussions.

Plus, please add unit tests.

packages/api/src/store/asset-table.ts

packages/api/src/controllers/project.ts

emranemran · 2024-02-29T04:28:11Z

@mjh1 @leszko: a few questions:

Let's assume a project exists in the dashboard. Now the user creates an api-key which does a POST. Now looking at authorizer, Bearer method is not supported for creation of new api-keys. Tried sending a jwt request in box (using params in config/full-stack.json) but it fails when trying to retrieve a user object. Any ideas why?
For the migration case when a current user logs in, the authorizer will get an empty or missing projectId field via getRequest for every GET/POST/etc. Should a default project be created on first login after we migrate?

emranemran · 2024-02-29T04:30:12Z

packages/api/src/controllers/project.ts

+    project = await db.project.get(projectId);
+    if (!project || project.deleted) {
+      return {};
+      //throw new NotFoundError(`project not found`);


Not sure yet how to handle this case yet....just returning empty object for now.

Why doesn't the throwing of NotFoundError work?

leszko · 2024-02-29T09:18:10Z

Let's assume a project exists in the dashboard. Now the user creates an api-key which does a POST. Now looking at authorizer, Bearer method is not supported for creation of new api-keys. Tried sending a jwt request in box (using params in config/full-stack.json) but it fails when trying to retrieve a user object. Any ideas why?

Yeah, I think it make sense, that Bearer is not supported for creating api tokens, because you always need to first create them from the livepeer dashboard (read: jwt). Why doesn't it work in the box? No idea, unfortunately 😞

For the migration case when a current user logs in, the authorizer will get an empty or missing projectId field via getRequest for every GET/POST/etc. Should a default project be created on first login after we migrate?

This is something we started to discuss at some point. And I think we still have these 3 options:

Option 1: Add projectId for all users with the DB migration (then you don't have the case you described)
Option 2: Tweak it in the authorizer(), so in the authorizer, if a user does not have a project, then return some default project
Option 3: What you described, so create a project on-a-fly

I think that we discussed that the Option 3 probably makes the least sense. But you know better the code now, so hard to suggest what's the best.

emranemran · 2024-03-01T04:41:02Z

packages/api/src/middleware/auth.ts

@@ -26,7 +27,7 @@ export const EMAIL_VERIFICATION_CUTOFF_DATE = 1695765600000; // 2023-09-26T22:00
 * are set to expire after a short time and the client manages using a refresh
 * token for keeping the user logged in.
 */
-export const NEVER_EXPIRING_JWT_CUTOFF_DATE = 1709251200000; // 2024-03-01T00:00:00.000Z
+export const NEVER_EXPIRING_JWT_CUTOFF_DATE = 1740820816000; // 2024-03-01T00:00:00.000Z


note: just a HACK for now since I wasn't sure why we have this cutoff date.

cc @victorges @leszko

AFAIK you don't need to change it. It was only a cutoff for the "old" JWT mechanism, and we're using now the "new" JWT mechanism. But Victor knows best since he updated "old" => "new" :)

Yeah this doesn't need to change! Here's the doc on how the migration works: https://www.notion.so/livepeer/Studio-dashboard-authentication-310a74802de54f538e0ecb633f94ee8e?pvs=4

victorges

preliminary LGTM! Still missing projectId on a ton of resources and checks on the APIs

packages/api/src/controllers/api-token.js

packages/api/src/controllers/asset.ts

packages/api/src/controllers/project.ts

victorges · 2024-03-01T21:02:28Z

packages/api/src/controllers/project.ts

+    project = await db.project.get(projectId);
+    if (!project || project.deleted) {
+      return {};
+      //throw new NotFoundError(`project not found`);


Why doesn't the throwing of NotFoundError work?

packages/api/src/controllers/project.ts

packages/api/src/middleware/auth.ts

victorges · 2024-03-01T21:10:22Z

packages/api/src/middleware/auth.ts

@@ -149,6 +155,9 @@ function authenticator(): RequestHandler {
    req.token = tokenObject;
    req.user = user;

+    project = await getProject(req, projectId);


This probably deserves some caching like user and api key above

victorges · 2024-03-01T21:11:11Z

packages/api/src/middleware/auth.ts

@@ -149,6 +155,9 @@ function authenticator(): RequestHandler {
    req.token = tokenObject;
    req.user = user;

+    project = await getProject(req, projectId);
+    req.project = project;


Can you add this project field to our Request type extensions here?

Already have it here actually:
https://github.com/livepeer/studio/pull/2078/files#diff-2ce1dfa11aee18127c26c8ff761f0ca88d79f008f9ec5a4c10c3bc39d8cd489bR45

Did you mean this or something else?

Oh yeah I meant that. Not sure how i missed it

victorges · 2024-03-01T21:12:12Z

packages/api/src/schema/api-schema.yaml

@@ -1153,6 +1172,10 @@ components:
            Name of the asset. This is not necessarily the filename, can be a
            custom name or title
          example: filename.mp4
+        projectId:


Seems like we still miss a bunch of these. They are probably readOnly too 🤔

emranemran · 2024-03-04T19:26:56Z

preliminary LGTM! Still missing projectId on a ton of resources and checks on the APIs

So the plan with this PR is to get it ready and merged w/o breaking existing dashboard usage so that @suhailkakar can get started on frontend work. Then follow-up PRs will be used for streams/webhooks/etc and update those code paths for projectId handling.

packages/api/src/controllers/project.ts

+  return res.json(output);
+});
+
+app.get("/:id", authorizer({}), async (req, res) => {


leszko

LGTM

leszko · 2024-03-08T11:12:38Z

packages/api/src/controllers/project.ts

+    res.status(403);
+    return res.json({ errors: ["project not found"] });
+  }
+  console.log("YYY here:", project, req.params.id);


leszko · 2024-03-08T11:12:42Z

packages/api/src/controllers/project.ts

+  const project = await db.project.get(req.params.id, {
+    useReplica: false,
+  });
+  console.log("YYY :", project, req.params.id);


leszko · 2024-03-08T11:12:50Z

packages/api/src/controllers/project.ts

+      "user can only request information on their own projects"
+    );
+  }
+  console.log("YYY here here:", project, req.params.id);


This reverts commit 6c20637.

This reverts commit df62e49.

…ollers (#2102) * Revert "Revert "Projects in Studio (#2078)" (#2096)" This reverts commit df62e49. * asset/api-token: update sql logic to correctly select projectId The previous queries were selecting records where projectId field in the JSONB data column was missing.

emranemran requested review from victorges, leszko and mjh1 February 27, 2024 01:19

emranemran requested a review from a team as a code owner February 27, 2024 01:19

github-advanced-security bot found potential problems Feb 27, 2024

View reviewed changes

packages/api/src/controllers/project.ts Dismissed Show dismissed Hide dismissed

packages/api/src/controllers/project.ts Fixed Show fixed Hide fixed

packages/api/src/controllers/project.ts Dismissed Show dismissed Hide dismissed

leszko reviewed Feb 27, 2024

View reviewed changes

leszko reviewed Feb 28, 2024

View reviewed changes

mjh1 reviewed Feb 28, 2024

View reviewed changes

packages/api/src/store/asset-table.ts Show resolved Hide resolved

vercel bot had a problem deploying to Preview February 29, 2024 04:18 Failure

github-advanced-security bot found potential problems Feb 29, 2024

View reviewed changes

packages/api/src/controllers/project.ts Fixed Show fixed Hide fixed

emranemran commented Feb 29, 2024

View reviewed changes

vercel bot deployed to Preview February 29, 2024 15:07 View deployment

vercel bot deployed to Preview February 29, 2024 15:31 View deployment

emranemran force-pushed the emran/projects-project-assets branch from 2ed9d17 to 1fa5344 Compare March 1, 2024 04:31

vercel bot had a problem deploying to Preview March 1, 2024 04:32 Failure

emranemran commented Mar 1, 2024

View reviewed changes

vercel bot had a problem deploying to Preview March 1, 2024 18:34 Failure

vercel bot had a problem deploying to Preview March 1, 2024 18:49 Failure

victorges reviewed Mar 1, 2024

View reviewed changes

vercel bot had a problem deploying to Preview March 4, 2024 21:31 Failure

vercel bot deployed to Preview March 5, 2024 04:15 View deployment

vercel bot deployed to Preview March 5, 2024 04:35 View deployment

vercel bot deployed to Preview March 5, 2024 18:53 View deployment

vercel bot deployed to Preview March 5, 2024 20:37 View deployment

vercel bot had a problem deploying to Preview March 6, 2024 21:46 Failure

github-advanced-security bot found potential problems Mar 6, 2024

View reviewed changes

packages/api/src/controllers/project.ts

return res.json(output);

});

app.get("/:id", authorizer({}), async (req, res) => {

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs

authorization

, but is not rate-limited.

vercel bot had a problem deploying to Preview March 6, 2024 23:05 Failure

vercel bot had a problem deploying to Preview March 7, 2024 06:14 Failure

vercel bot deployed to Preview March 7, 2024 21:13 View deployment

emranemran added 5 commits March 7, 2024 13:34

schema: add boiler plate to handle projectId logic

21fa11b

project: add handlers to get/post projects

5401dd1

auth: add projectId support for api-token and jwt methods

dce1b96

api-token: add projectId support in api-key handlers

ef012cb

asset: add projectId support in asset handlers

e15955e

emranemran force-pushed the emran/projects-project-assets branch from 429c1d8 to e15955e Compare March 7, 2024 21:52

vercel bot deployed to Preview March 7, 2024 21:53 View deployment

leszko approved these changes Mar 8, 2024

View reviewed changes

emranemran merged commit 6c20637 into master Mar 11, 2024
12 of 13 checks passed

emranemran deleted the emran/projects-project-assets branch March 11, 2024 00:54

mjh1 added a commit that referenced this pull request Mar 11, 2024

Revert "Projects in Studio (#2078)"

a992add

This reverts commit 6c20637.

mjh1 added a commit that referenced this pull request Mar 11, 2024

Revert "Projects in Studio (#2078)" (#2096)

df62e49

This reverts commit 6c20637.

emranemran added a commit that referenced this pull request Mar 13, 2024

Revert "Revert "Projects in Studio (#2078)" (#2096)"

901239a

This reverts commit df62e49.

Projects in Studio #2078

Projects in Studio #2078

Conversation

emranemran commented Feb 27, 2024 • edited

vercel bot commented Feb 27, 2024 • edited

leszko 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

mjh1 Feb 28, 2024 • edited

Choose a reason for hiding this comment

emranemran Feb 29, 2024 • edited

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

leszko left a comment

Choose a reason for hiding this comment

emranemran commented Feb 29, 2024

emranemran Feb 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leszko commented Feb 29, 2024 • edited

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

victorges 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

emranemran commented Mar 4, 2024 • edited

leszko 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

emranemran commented Feb 27, 2024 •

edited

vercel bot commented Feb 27, 2024 •

edited

mjh1 Feb 28, 2024 •

edited

emranemran Feb 29, 2024 •

edited

emranemran Feb 29, 2024 •

edited

leszko commented Feb 29, 2024 •

edited

emranemran commented Mar 4, 2024 •

edited