-
Notifications
You must be signed in to change notification settings - Fork 31
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.
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
accessControl: Add /gate endpoint for Catalyst integration #1317
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -8,4 +14,59 @@ | |||
accessControl.use("/signing-key", signingKeyApp); | |||
app.use("/access-control", accessControl); | |||
|
|||
accessControl.post( | |||
"/gate", | |||
authorizer({ anyAdmin: true }), |
Check failure
Code scanning / CodeQL
Missing rate limiting
…t to gated contents
Codecov Report
@@ Coverage Diff @@
## master #1317 +/- ##
===================================================
+ Coverage 51.60416% 51.88341% +0.27924%
===================================================
Files 68 68
Lines 4426 4460 +34
Branches 822 833 +11
===================================================
+ Hits 2284 2314 +30
- Misses 1849 1851 +2
- Partials 293 295 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I have some specific suggestions to make the code a little more maintainable specifically regarding security. Overall, we should default to (gate) "closed" instead of "open" in unexpected code paths or inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-LGTM!
(havent checked the code again but the replies to comment make full sense)
|
||
const user = await db.user.get(content.userId); | ||
|
||
if (user.suspended) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be here. It is common logic for any kind of playback policy. If the user is suspended, playback should be invariably blocked.
So this should go to before any branch on the playback policy, right after fetching the content itself.
Also, you should also check || content.suspended
which is available for stream objects. Given assets don't have that field, you might need to do || ("suspended" in content && content.suspended)
for TypeScript happiness.
What does this pull request do? Explain your changes. (required)
Creates a
/access-control/gate
endpoint for Catalyst integrationSpecific updates (required)
stream
or anasset
playbackId to the endpoint2XX
or4XX
depending on the existence of the content, whether the publicKey is valid and owned by the sameUser
that own the contentHow did you test each of these updates (required)
yarn test
Does this pull request close any open issues?
Fixes #1285
Partially Fixes #1288
Screenshots (optional):
Checklist: