Introduce type and RPC definitions for permission API methods #256
Conversation
|
I'm planning on keeping this PR in draft and making iterative changes to it as I work on the backend implementation or as new frontend requirements are discovered |
proto/lekko/bff/v1beta1/bff.proto
Outdated
| string team_name = 1; | ||
| string role_name = 2; | ||
| Role updated_role = 3; |
There was a problem hiding this comment.
nit: team name and role name are composed within Role, so the params are duplicative. you could dedup or make it clear what the first two params are for.
There was a problem hiding this comment.
Ah that's definitely true for team name. But the reason I split out role_name was to allow updating the names of roles without having to send in the role ID as part of the updated_role object since we don't really use ids in the frontend at all for any of our other BFF data
| string username = 1; | ||
| string team_name = 2; |
There was a problem hiding this comment.
is the intent to view roles for other users?
if this is for the authenticated user, these two params are embedded in the auth object (as part of the JWT)
There was a problem hiding this comment.
Yeah I thought about that. The primary use case for this request was for users to see their own roles like you said, but I figured we might as well leave it open if we end up wanting UI for a team manager viewing individual members' roles. What do you think?
There was a problem hiding this comment.
yeah it might be useful to view someone else's role, so username is valid.
proto/lekko/bff/v1beta1/bff.proto
Outdated
| message UpdateRoleUsersRequest { | ||
| string team_name = 1; | ||
| string role_name = 2; | ||
| repeated string usernames = 3; | ||
| } |
There was a problem hiding this comment.
whats the thought process behind designing this rpc as a massive set difference operation? I could imagine this being broken up into
[Bulk]AddUserToRole
[Bulk]RemoveUserFromRole
where the UI is more prescriptive about what is being added and removed. That would be similar to team memberships, where you can add members and remove members individually
There was a problem hiding this comment.
Yeah this is definitely open to discussion. I thought it might be nicer UX for the person making the changes to be able to draft a bunch of additions/removals together, then view the overall comparison, and hit save to push those changes at once.
With the broken up bulk methods, addition and removal would require different flows which I thought might be cumbersome. Happy to talk about this more though
There was a problem hiding this comment.
yeah the choice definitely depends on UX. i do think the drafting+comparison UX is more difficult to build. Also, you could argue we need the same UX for team members, but we don't - the simplicity has benefits.
There was a problem hiding this comment.
Yep, big +1 on the simplicity part. One thing to note is our invite and member management flow will eventually be converted to be based on users with multiple roles, so whichever approach we choose will replace what we have currently.
There was a problem hiding this comment.
The reason I gravitated to the drafting + comparison UX initially was to make it easy to extend to a potential approval flow that we might want in the future, but as an initial version I'm happy to keep it simple here haha
| message UpsertRolePermissionRequest { | ||
| string team_name = 1; | ||
| string role_name = 2; | ||
| string resource_path = 3; |
There was a problem hiding this comment.
i'm curious if you're weighing the options around how to represent resource path in protobuf. I could imagine it as a decomposed struct, or a tree, instead of a raw string here.
There was a problem hiding this comment.
The overall reasoning mostly came down to simplicity - the aspect that would benefit the most from managing a full tree is keeping track of a role or a user's set of permissions. So the tree or struct representations might make sense in responses. But in requests, we never need to handle more than a single path at once, so I figured strings would be good enough for the API methods.
Context
Task with more context on use cases for each method: Notion
Details