-
Notifications
You must be signed in to change notification settings - Fork 19
Only allowed users can execute benchmarks #11
base: master
Are you sure you want to change the base?
Conversation
|
||
Add `ALLOWED_USERS` with comma separated list of user's github ids. Eg: | ||
|
||
`ALLOWED_USERS=123,455,234` |
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.
can we not support usernames directly?
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.
I believe it would be possible. One of the reasons that i went with ids was that you can change your username ( although probably not very common thing to do).
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.
I think even if users can change their username, we would prefer this, as it is much easier to audit.
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.
Github IDs are immutable, therefore saner to rely on. You don't risk having someone hijack a username to execute the commands. I don't get why it would be harder to audit - even if that were the case, the stability makes up for it.
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.
Github IDs are immutable, therefore saner to rely on. You don't risk having someone hijack a username to execute the commands. I don't get why it would be harder to audit - even if that were the case, the stability makes up for it.
imo, this is good point.
Obliviously i can change this PR and use usernames instead of ids - so let me know what would you prefer.
Co-authored-by: Shawn Tabrizi <[email protected]>
User @enthusiastmartin, please sign the CLA here. |
I did not forget this MR. I have plans of migrating this repository to another code base (#51) which already implements a similar access control functionality for organizations and we could implement it for users as well, after the refactor is done. |
Added a configurable option to allow only selected users to execute benchmarks.
It is done via ALLOWED_USERS env - details are in README.
If not provided - any user is allowed (original behavior).