NOISSUE - Add send_invitation permission #2773
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2773 +/- ##
==========================================
- Coverage 41.73% 36.98% -4.76%
==========================================
Files 352 30 -322
Lines 48692 4467 -44225
==========================================
- Hits 20322 1652 -18670
+ Misses 26156 2655 -23501
+ Partials 2214 160 -2054 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
domains/middleware/authorization.go
Outdated
| func (am *authorizationMiddleware) billingPermissionCheck(ctx context.Context, domainID, subj, perm string) error { | ||
| req := authz.PolicyReq{ | ||
| SubjectType: policies.UserType, | ||
| SubjectKind: policies.UsersKind, | ||
| Subject: subj, | ||
| Permission: perm, | ||
| ObjectType: policies.DomainType, | ||
| Object: domainID, | ||
| } | ||
| if err := am.authz.Authorize(ctx, req); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
we do not need a new function, just edit the current permission check
domains/middleware/authorization.go
Outdated
| var ErrMemberExist = errors.New("user is already a member of the domain") | ||
| var ( | ||
| ErrMemberExist = errors.New("user is already a member of the domain") | ||
| ErrPermissionFail = errors.New("billing permission failed") |
There was a problem hiding this comment.
there can be many auth callouts so error will be passed from auth service
domains/middleware/authorization.go
Outdated
| @@ -151,6 +154,10 @@ func (am *authorizationMiddleware) SendInvitation(ctx context.Context, session a | |||
| return err | |||
| } | |||
|
|
|||
| if err := am.extAuthorize(ctx, auth.EncodeDomainUserID(invitation.DomainID, session.UserID), policies.SendInvitationPermission, policies.DomainType, invitation.DomainID); err != nil { | |||
| return errors.Wrap(svcerr.ErrAuthorization, ErrPermissionFail) | |||
There was a problem hiding this comment.
wrap err and authorization error
| @@ -361,7 +361,8 @@ definition domain { | |||
| permission group_add_role_users_permission = group_add_role_users + team->group_add_role_users + organization->admin | |||
| permission group_remove_role_users_permission = group_remove_role_users + team->group_remove_role_users + organization->admin | |||
| permission group_view_role_users_permission = group_view_role_users + team->group_view_role_users + organization->admin | |||
|
|
|||
|
|
|||
| permission send_invitation = admin | |||
There was a problem hiding this comment.
Why don't we reuse the permission admin or add_role_users_permission?,
supermq/docker/spicedb/schema.zed
Line 323 in d842182
Because , invitation is similar to adding user to a role.
There was a problem hiding this comment.
adding user to role is not exactly the same. since invitations can be rejected.
The idea here is with billing if we are to limit the number of users in a workspace. we need to limit sending invitations and accepting invitations at the same time.
some psudocode:
-
Send user inviation
-
Billing callback checks if domain-members limit is exceeded. if not invitation sending is authorized
-
when user is accepting invitation ("adding role user")
-
billing will also check domain members limit. Allow the user to be added to domain or not
There was a problem hiding this comment.
Got it,
Then Can we have separate relation like relation send_invitations: role#member?
and separate permission like permission send_invitation_permission = send_invitations + organization->admin
What do you think about this approach ?
There was a problem hiding this comment.
Not sure if a new relation is needed, since it will add the number of options the user has on actions, but I don't mind if you'd like to add it on mg. Billing only cares about the permission in the auth request
Signed-off-by: WashingtonKK <[email protected]>
Signed-off-by: WashingtonKK <[email protected]>
Signed-off-by: WashingtonKK <[email protected]>
Signed-off-by: WashingtonKK <[email protected]>
Signed-off-by: WashingtonKK <[email protected]>
What type of PR is this?
What does this do?
Which issue(s) does this PR fix/relate to?
Have you included tests for your changes?
Did you document any new/modified feature?
Notes