Conversation
Signed-off-by: Hassan Abouelela <hassan@hassanamr.com>
Adds discord role support to the pre-existing scopes system to power more complex access permissions. Signed-off-by: Hassan Abouelela <hassan@hassanamr.com>
Adds a new property on forms to declare which roles are authorized to access form responses. Signed-off-by: Hassan Abouelela <hassan@hassanamr.com>
Adds the ability to specify who can edit forms using discord roles. Signed-off-by: Hassan Abouelela <hassan@hassanamr.com>
Signed-off-by: Hassan Abouelela <hassan@hassanamr.com>
HassanAbouelela
added
area: authentication
Signed-off-by: Hassan Abouelela <hassan@hassanamr.com>
Co-authored-by: Bluenix <bluenixdev@gmail.com> Signed-off-by: Hassan Abouelela <hassan@hassanamr.com>
HassanAbouelela
force-pushed
the
roles
branch
from
1fc0660 to
1e5cdea
Compare
Co-authored-by: Bluenix <bluenixdev@gmail.com> Signed-off-by: Hassan Abouelela <hassan@hassanamr.com>
kosayoda
left a comment
kosayoda
left a comment
There was a problem hiding this comment.
Cursory testing works fine so far, but I've haven't had the time to properly review
| ) | ||
|
|
||
| @requires(["authenticated", "admin"]) | ||
| @requires(["authenticated", "Helpers"]) |
There was a problem hiding this comment.
I'm not sure how this works, and I can't seem to find a reference. Is this tied to a role helper somewhere?
There was a problem hiding this comment.
It isn't tied to a constant, but rather the new access system fetches all the user's roles, and adds their names as scopes.
There was a problem hiding this comment.
Can we move this to a constant at the top of the file or in the constants file so that it is easily changed?
There was a problem hiding this comment.
Does it need to be easily changed, or changed at all for that matter?
Signed-off-by: Hassan Abouelela <hassan@hassanamr.com>
|
I tested this on my computer and it worked nicely! Not approving until the previous comments have been addressed though. One thing I do want to bring up is perhaps using a prefix for the role scopes? I feel like we'll eliminate a potential issue with a role and an unrelated scope having matching names or similar. For example, just formatting |
kosayoda
left a comment
kosayoda
left a comment
There was a problem hiding this comment.
GET/PATCHing a form both through the frontend and through making requests manually to the backend fails with a 404 for me, which is fixed through the suggested changes and verified by checking the state of the form in the database. Not sure if this is just an issue on my end, since it seems to work fine in @Bluenix2's testing
Signed-off-by: Hassan Abouelela <hassan@hassanamr.com>
Co-authored-by: Kieran Siek <kieransiek@protonmail.com>
I used to have similar issues. See this message in our internal channel where I touch on this. It just magically started working later on (with no code change) which made it possible for me to continue and test it. I shrugged it off as issues with me setting up the project.
|
kosayoda
left a comment
kosayoda
left a comment
There was a problem hiding this comment.
Although I still haven't figured out why the Form id field aliasing does not work on my machine, after 27b0c38 it is no longer a blocker.
Namespacing discord roles with role: (or even discord:) suggested by @Bluenix2 could improve the robustness of the starlette permission handling, but it also isn't a blocker for merging this PR.
3 participants
This PR overhauls the forms access system to be more dynamic and malleable using discord roles. The PR adds utilities to interact with the discord API to fetch roles and member objects, as well as modified preexisting resources to work with the new system.
Important highlights of modified resources:
editorsin the form object (except the everyone role).response_readersin the form object (except the everyone role).