SMQ-2670 - Fix Unauthorized User IDs can be added to domain entity role members #2684
Conversation
|
@arvindh123 Please review. |
There was a problem hiding this comment.
Please add the same check to the entity creation middleware, as the entity creation functions CreateDomain, CreateGroup, CreateChannels, and CreateClients will call the roles.AddNewEntitiesRoles function, which also adds members to the entity. Therefore, we need the same check in the entity creation middleware.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2684 +/- ##
==========================================
- Coverage 42.08% 41.73% -0.36%
==========================================
Files 341 24 -317
Lines 47488 5068 -42420
==========================================
- Hits 19987 2115 -17872
+ Misses 25318 2788 -22530
+ Partials 2183 165 -2018 ☔ View full report in Codecov by Sentry. |
channels/middleware/authorization.go
Outdated
| @@ -97,6 +97,10 @@ func (am *authorizationMiddleware) CreateChannels(ctx context.Context, session a | |||
| } | |||
| } | |||
|
|
|||
| if err := am.RoleManagerAuthorizationMiddleware.AuthorizeMembers(ctx, session, []string{session.UserID}); err != nil { | |||
| return []channels.Channel{}, []roles.RoleProvision{}, errors.Wrap(svcerr.ErrAuthorization, err) | |||
There was a problem hiding this comment.
@nyagamunene
I think this is not needed, Because during the authorization the requested user will be check user have access to domain as member.
clients/middleware/authorization.go
Outdated
| @@ -89,6 +89,10 @@ func (am *authorizationMiddleware) CreateClients(ctx context.Context, session au | |||
| } | |||
| } | |||
|
|
|||
| if err := am.RoleManagerAuthorizationMiddleware.AuthorizeMembers(ctx, session, []string{session.UserID}); err != nil { | |||
| return []clients.Client{}, []roles.RoleProvision{}, errors.Wrap(svcerr.ErrAuthorization, err) | |||
There was a problem hiding this comment.
@nyagamunene
I think this is not needed, Because during the authorization the requested user will be check user have access to domain as member.
domains/middleware/authorization.go
Outdated
| @@ -48,6 +50,9 @@ func AuthorizationMiddleware(entityType string, svc domains.Service, authz smqau | |||
| } | |||
|
|
|||
| func (am *authorizationMiddleware) CreateDomain(ctx context.Context, session authn.Session, d domains.Domain) (domains.Domain, []roles.RoleProvision, error) { | |||
| if err := am.RoleManagerAuthorizationMiddleware.AuthorizeMembers(ctx, session, []string{session.UserID}); err != nil { | |||
There was a problem hiding this comment.
@nyagamunene
same here, we can remove this logic
groups/middleware/authorization.go
Outdated
| @@ -95,6 +95,11 @@ func (am *authorizationMiddleware) CreateGroup(ctx context.Context, session auth | |||
| return groups.Group{}, []roles.RoleProvision{}, errors.Wrap(svcerr.ErrUnauthorizedPAT, err) | |||
| } | |||
| } | |||
|
|
|||
| if err := am.RoleManagerAuthorizationMiddleware.AuthorizeMembers(ctx, session, []string{session.UserID}); err != nil { | |||
There was a problem hiding this comment.
@nyagamunene we can remove this logic
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
nyagamunene
force-pushed
the
SMQ-2670
branch
from
329b959 to
7c167a6
Compare
Signed-off-by: nyagamunene <[email protected]>
What type of PR is this?
This is a bug fix because it fixes the following issue: #2670
What does this do?
It add a authorization check for every member provided.
Which issue(s) does this PR fix/relate to?
Have you included tests for your changes?
No
Did you document any new/modified feature?
No
Notes