Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add projectId scoping to streams/sessions/webhook #2103

Merged
merged 17 commits into from May 10, 2024
Merged

add projectId scoping to streams/sessions/webhook #2103

merged 17 commits into from May 10, 2024

Conversation

emranemran
Copy link
Contributor

@emranemran emranemran commented Mar 13, 2024 •
edited

  • Adding projectId scoping for stream/session/webhook.ts.
  • TODO: add/fix tests

@vercel Vercel
Copy link

vercel bot commented Mar 13, 2024 •
edited

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 May 10, 2024 2:43pm

github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 13, 2024
View reviewed changes
packages/api/src/controllers/project.ts Outdated
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.
@emranemran emranemran changed the base branch from master to emran/projects-project-assets-redo March 18, 2024 01:23
@emranemran emranemran changed the base branch from emran/projects-project-assets-redo to emran/projects-assets-apitoken-redo March 18, 2024 01:24
@emranemran emranemran marked this pull request as ready for review March 18, 2024 14:12
@emranemran emranemran requested a review from a team as a code owner March 18, 2024 14:12
@emranemran emranemran changed the title WIP: add projectId scoping to streams/sessions add projectId scoping to streams/sessions/webhook Mar 18, 2024
leszko
leszko approved these changes Mar 18, 2024
View reviewed changes
Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it maybe be possible to add some unit tests?

victorges
victorges requested changes Mar 19, 2024
View reviewed changes
Copy link
Member

@victorges victorges left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes due to the admin APIs/queries being potentially broken. Anything else is nit/optional.

packages/api/src/controllers/session.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/session.ts Show resolved Hide resolved
packages/api/src/controllers/stream.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/stream.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/stream.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/stream.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/stream.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/stream.ts Outdated Show resolved Hide resolved
Base automatically changed from emran/projects-assets-apitoken-redo to master March 25, 2024 13:41
victorges
victorges approved these changes May 7, 2024
View reviewed changes
Copy link
Member

@victorges victorges left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, but I think there are a few other places that could use the new helpers.

There are also a couple comments from the past reviews that didn't get a reply nor fix. Could you make sure there's nothing important left behind?

packages/api/src/controllers/session.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/signing-key.test.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/stream.test.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/stream.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/stream.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/stream.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/stream.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/stream.ts Outdated Show resolved Hide resolved
packages/api/src/controllers/webhook.ts Outdated Show resolved Hide resolved
@gioelecerati gioelecerati merged commit 21dbe39 into master May 10, 2024
8 checks passed
@gioelecerati gioelecerati deleted the emran/projects-streams branch May 10, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@victorges victorges victorges approved these changes

@leszko leszko leszko approved these changes

@mjh1 mjh1 mjh1 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@emranemran @victorges @leszko @mjh1 @gioelecerati