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.

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

Add user verification for server API #37

Closed
wants to merge 14 commits into from
Closed

Conversation

kyeah
Copy link
Collaborator

@kyeah kyeah commented Aug 13, 2024

Changes

  • Send Firebase user token to the server for admin endpoints
  • Reject server API calls without proper authentication
  • Reject stream webhook calls without proper authentication (from stream)

Context

Provides some additional protections since the server is now being relied on for more administrative features (e.g. streaming directly from the browser.)

This was a bit lower on our priority list, but I realized it would be fairly straightforward to implement following the Firebase authentication guidance.

Testing

Verified that I get a forbidden error without auth:

curl -X GET http://localhost:4000/stream-keys/soft
Forbidden

curl -X GET -H "Authorization: Bearer 123" http://localhost:4000/stream-key/soft
Forbidden

Verified that Admin page still showed stream keys when I am an admin for a given room.

Relevant Issues

Closes #35

Copy link

netlify bot commented Aug 13, 2024

Deploy Preview for is-this-thing-on ready!

Name Link
🔨 Latest commit 5eb0b13
🔍 Latest deploy log https://app.netlify.com/sites/is-this-thing-on/deploys/674aa742a1ee310008de16d7
😎 Deploy Preview https://deploy-preview-37--is-this-thing-on.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 51 (🔴 down 43 from production)
Accessibility: 82 (🟢 up 1 from production)
Best Practices: 92 (no change from production)
SEO: 73 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

setRooms(rooms);
let [rooms, userToken] = await Promise.all([
getRoomsWhereUserISAdmin(uid),
auth.currentUser?.getIdToken(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to use the user from UserStore at first, but for some reason it couldn't find the getIdToken method.

@kyeah kyeah requested a review from bhaviksingh August 21, 2024 04:21
@kyeah
Copy link
Collaborator Author

kyeah commented Sep 14, 2024

@bhaviksingh This needs a server deployment, so will defer to you to merge this PR and deploy since I don't have those permissions I think :D

Copy link
Collaborator

@bhaviksingh bhaviksingh left a comment

Choose a reason for hiding this comment

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

Ahhh completely missed this. Do you still want to try and merge it? My suggestion would be to merge into #55 , and then merge together?

@kyeah
Copy link
Collaborator Author

kyeah commented Nov 30, 2024

Hmm, let's try to keep them separate if possible but I'll get this branch up to date to avoid conflicts. After that:

  1. Merge Migrate Stream admin functionality into main admin panel #55
  2. Merge this and get the server changes deployed

@bhaviksingh
Copy link
Collaborator

Hey - at this point, I may recommend just deprecating this branch :(There's lots of stuff in here that is no longer relevant. The roomManagement UI (that this effects, with RTMPS URLs / reset stream key functionality) has pretty much entirely changed and not really needed anymore

If you think it's important for the experience for the show to have the server / stream authenticated (which seems smart), maybe we do it in a new branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

protections for server API
2 participants