-
Notifications
You must be signed in to change notification settings - Fork 0
Feature: Add Auto-Approve Env Vars for Group and Event Creation #38
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
Conversation
This adds two new feature flags to auto-approve groups and events. We already had an auto-approve flag for events, but it was tied to the Networks table. Instead of adding a new column to the Networks table for groups, we will extracct both as env variables.
… events This adds some security to the auto-approve feature. Only Restarter users and higher will be auto-approved for creating groups. I should note a Restarter user will automatically be elevated to a Host user once they create a group; however, their Host role is only tied to the group they created. Therefore, they will still not be able to create or auto-approve events for other groups.
mlahargou
left a comment
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.
CR 📱
|
|
||
| return $autoapprove; | ||
| // Events are auto-approved based on environment configuration | ||
| return env('FEATURE__AUTO_APPROVE_EVENTS', false); |
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.
If FEATURE__AUTO_APPROVE_EVENTS is false, do we want to keep the previous behavior?
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.
The whole network feature is similar to multiple instances of Restarters.
We don't really use the Network feature so having to check the group on all networks would not apply for us.
| if ($autoapprove) { | ||
| Log::info("Auto-approve event $idParty"); | ||
| // Only auto-approve if the feature is enabled AND the user has privileged role (Root, Admin, Host) | ||
| if ($autoapprove && $user->role <= ROLE::HOST) { |
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 is probably right, but funky lol. I would have assumed ROOT was a higher level than HOST. But I'm assuming they are just numbers starting with 1 for highest permission.
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.
But I'm assuming they are just numbers starting with 1 for highest permission.
Correct
restarters/app/Models/Role.php
Lines 11 to 16 in 1092162
| const ROOT = 1; | |
| const ADMINISTRATOR = 2; | |
| const HOST = 3; | |
| const RESTARTER = 4; | |
| const GUSET = 5; | |
| const NETWORK_COORDINATOR = 6; |
Description
This adds two new feature flags to auto-approve groups and events. We already had an auto-approve flag for events, but it was tied to the Networks table. Instead of adding a new column to the Networks table for groups, we will extract both as env variables.
CR Notes
I added some additional security around the auto-approval features by tying it with the user roles. Only Host roles and above will be auto-approved for group and event creation.
I should note that new users start of with the
RESTARTERrole then are elevated to theHOSTrole once they create a group. However, there is already checks in place to prevent these users from creating events for other groups. Therefore, this will not cause issues with current perm functionality.qa_req 0