Skip to content
This repository has been archived by the owner on Apr 19, 2023. It is now read-only.

allowing custom groups to upload videos #318

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

pedramp20
Copy link

@pedramp20 pedramp20 commented Oct 11, 2021

Issue #, if available:
#316
Description of changes:
Removing the hard coded admin group and letting users choose the group(s) they want to grant permission to upload/delete videos.

Since, the option of "any authenticated user can upload video" is provided, there is no need to create an admin group. Basically, user should take care of group creation using aws cli and here we just give them option to select groups

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@pedramp20
Copy link
Author

pedramp20 commented Oct 11, 2021

I realised that the s3 bucket name is used in the policy and video plugin is changing cognito pool groups, which is not ideal and would fail if user switches the environment. So, I completely changed the way permissions are granted. The policy which is being attached to the authrole is used and attached to existing group roles.
Please note that no user is granted the DeleteObject permission for security reasons. I believe a separate step should be implemented that the permissions for each group is asked from the user. (Could be added to the project board)

@wizage
Copy link
Contributor

wizage commented Oct 18, 2021

I can handle the Lint issues, but @pedramp20 can you remove the package-lock.json file from your commit. We don't accept any changes to our package.json or package-lock.json as we do security reviews of them

@pedramp20
Copy link
Author

@wizage Done!

@wmccracken
Copy link

Is there a plan to merge this? We are using cognito for auth with our amplify project and user accounts are not able to upload video files (403 errors from s3).

@wizage
Copy link
Contributor

wizage commented Nov 11, 2021

Yes there is plans. We are currently heads down for re:invent with some big changes to how the API integration works so this will get merged in as we get close to releasing v4.0.

The entire Auth and API has been moved out of vod-push into it's own file to handle those calls and to use headless mode for Auth and API. We went this route to support IVS with APIs as well. Once the full migration is done and 4.0 is ready to release (which includes this code base) we will close this.

@wmccracken
Copy link

Yes there is plans. We are currently heads down for re:invent with some big changes to how the API integration works so this will get merged in as we get close to releasing v4.0.

The entire Auth and API has been moved out of vod-push into it's own file to handle those calls and to use headless mode for Auth and API. We went this route to support IVS with APIs as well. Once the full migration is done and 4.0 is ready to release (which includes this code base) we will close this.

@wizage - thanks for the update. Is there currently a way to allow cognito authorized users to upload to the import bucket or are only admin accounts able to upload video at this time?

@wizage
Copy link
Contributor

wizage commented Nov 11, 2021

Yes there is plans. We are currently heads down for re:invent with some big changes to how the API integration works so this will get merged in as we get close to releasing v4.0.
The entire Auth and API has been moved out of vod-push into it's own file to handle those calls and to use headless mode for Auth and API. We went this route to support IVS with APIs as well. Once the full migration is done and 4.0 is ready to release (which includes this code base) we will close this.

@wizage - thanks for the update. Is there currently a way to allow cognito authorized users to upload to the import bucket or are only admin accounts able to upload video at this time?

This has already been enabled through the existing questions. It asks to define your permission schema. Whether Admins can only upload, or Any Authenticated user or both.

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

Successfully merging this pull request may close these issues.

3 participants