Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MG-2069 - Remove relation requirement from entity unassignment #2130

Merged
merged 7 commits into from
Apr 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api/openapi/auth.yml
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ paths:
security:
- bearerAuth: []
responses:
"200":
"204":
description: Users successfully unassigned from domain.
"400":
description: Failed due to malformed domain's ID.
Expand Down
6 changes: 6 additions & 0 deletions api/openapi/users.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1787,6 +1787,10 @@ components:
type: string
format: email
description: User email.
host:
type: string
example: examplehost
description: Email host.

PasswordReset:
description: Password reset request data, new password and token that is appended on password reset link received in email.
Expand All @@ -1799,11 +1803,13 @@ components:
type: string
format: password
description: New password.
example: 12345678
minimum: 8
confirm_password:
type: string
format: password
description: New confirmation password.
example: 12345678
minimum: 8
token:
type: string
Expand Down
9 changes: 0 additions & 9 deletions auth/api/http/domains/endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1163,15 +1163,6 @@ func TestUnassignDomainUsers(t *testing.T) {
status: http.StatusBadRequest,
err: apiutil.ErrValidation,
},
{
desc: "unassign domain users with empty relation",
data: fmt.Sprintf(`{"relation": "%s", "user_ids" : ["%s", "%s"]}`, "", validID, validID),
domainID: domain.ID,
contentType: contentType,
token: validToken,
status: http.StatusBadRequest,
err: apiutil.ErrValidation,
},
}

for _, tc := range cases {
Expand Down
4 changes: 0 additions & 4 deletions auth/api/http/domains/requests.go
Original file line number Diff line number Diff line change
Expand Up @@ -205,10 +205,6 @@ func (req unassignUsersReq) validate() error {
return apiutil.ErrMalformedPolicy
}

if req.Relation == "" {
return apiutil.ErrMalformedPolicy
}

return nil
}

Expand Down
23 changes: 13 additions & 10 deletions auth/postgres/domains.go
Original file line number Diff line number Diff line change
Expand Up @@ -370,16 +370,19 @@ func (repo domainRepo) DeletePolicies(ctx context.Context, pcs ...auth.Policy) (

for _, pc := range pcs {
q := `
DELETE FROM
policies
WHERE
subject_type = :subject_type
AND subject_id = :subject_id
AND subject_relation = :subject_relation
AND relation = :relation
AND object_type = :object_type
AND object_id = :object_id
;`
DELETE FROM
WashingtonKK marked this conversation as resolved.
Show resolved Hide resolved
policies
WHERE
WashingtonKK marked this conversation as resolved.
Show resolved Hide resolved
subject_type = :subject_type
AND subject_id = :subject_id
AND subject_relation = :subject_relation
AND object_type = :object_type
AND object_id = :object_id
`
if pc.Relation != "" {
q = fmt.Sprintf("%s AND relation = :relation", q)
Copy link
Contributor

@arvindh123 arvindh123 Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WashingtonKK There will be only one relation in the Domain.
So no need to consider relation here

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WashingtonKK Please check this one and send a follow-up PR for it.

}
q = fmt.Sprintf("%s;", q)

dbpc := toDBPolicy(pc)
row, err := tx.NamedQuery(q, dbpc)
Expand Down
11 changes: 11 additions & 0 deletions auth/postgres/domains_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,17 @@ func TestDeletePolicyCopy(t *testing.T) {
},
err: nil,
},
{
desc: "delete a policy with empty relation",
pc: auth.Policy{
SubjectType: "unknown",
SubjectID: "unknown",
Relation: "",
ObjectType: "unknown",
ObjectID: "unknown",
},
err: nil,
},
}

for _, tc := range cases {
Expand Down
56 changes: 41 additions & 15 deletions auth/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -718,31 +718,57 @@ func (svc service) AssignUsers(ctx context.Context, token, id string, userIds []
}

func (svc service) UnassignUsers(ctx context.Context, token, id string, userIds []string, relation string) error {
if err := svc.Authorize(ctx, PolicyReq{
pr := PolicyReq{
Subject: token,
SubjectType: UserType,
SubjectKind: TokenKind,
Object: id,
ObjectType: DomainType,
Permission: SharePermission,
}); err != nil {
}
if err := svc.Authorize(ctx, pr); err != nil {
return err
}

if err := svc.Authorize(ctx, PolicyReq{
Subject: token,
SubjectType: UserType,
SubjectKind: TokenKind,
Object: id,
ObjectType: DomainType,
Permission: SwitchToPermission(relation),
}); err != nil {
return err
if relation != "" {
pr.Permission = SwitchToPermission(relation)
if err := svc.Authorize(ctx, pr); err != nil {
return err
}

if err := svc.removeDomainPolicies(ctx, id, relation, userIds...); err != nil {
return errors.Wrap(errRemovePolicies, err)
}
return nil
}
pr.Permission = AdminPermission
if err := svc.Authorize(ctx, pr); err != nil {
// User is not admin.
var ids []string
for _, userID := range userIds {
if err := svc.Authorize(ctx, PolicyReq{
Subject: userID,
SubjectType: UserType,
SubjectKind: UsersKind,
Permission: AdminPermission,
Object: id,
ObjectType: DomainType,
}); err != nil {
// Append all non-admins to ids.
ids = append(ids, userID)
}
}

userIds = ids
}

if err := svc.removeDomainPolicies(ctx, id, relation, userIds...); err != nil {
return errors.Wrap(errRemovePolicies, err)
for _, rel := range []string{MemberRelation, ViewerRelation, EditorRelation} {
// Remove only non-admins.
if err := svc.removeDomainPolicies(ctx, id, rel, userIds...); err != nil {
return errors.Wrap(errRemovePolicies, err)
}
}

return nil
}

Expand Down Expand Up @@ -911,8 +937,8 @@ func (svc service) removeDomainPolicies(ctx context.Context, domainID, relation
if err := svc.agent.DeletePolicies(ctx, prs); err != nil {
return errors.Wrap(errRemovePolicies, err)
}
err = svc.domains.DeletePolicies(ctx, pcs...)
if err != nil {

if err = svc.domains.DeletePolicies(ctx, pcs...); err != nil {
return errors.Wrap(errRemovePolicies, err)
}
return err
Expand Down
4 changes: 0 additions & 4 deletions things/api/http/requests.go
Original file line number Diff line number Diff line change
Expand Up @@ -245,10 +245,6 @@ func (req unassignUsersRequest) validate() error {
return apiutil.ErrBearerToken
}

if req.Relation == "" {
return apiutil.ErrMissingRelation
}

if req.groupID == "" {
return apiutil.ErrMissingID
}
Expand Down
2 changes: 1 addition & 1 deletion things/api/http/requests_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -603,7 +603,7 @@ func TestUnassignUsersRequestValidate(t *testing.T) {
UserIDs: []string{validID},
Relation: "",
},
err: apiutil.ErrMissingRelation,
err: nil,
},
}
for _, c := range cases {
Expand Down
3 changes: 0 additions & 3 deletions users/api/requests.go
Original file line number Diff line number Diff line change
Expand Up @@ -330,9 +330,6 @@ func (req unassignUsersReq) validate() error {
return apiutil.ErrMissingID
}

if req.Relation == "" {
return apiutil.ErrMissingRelation
}
if len(req.UserIDs) == 0 {
return apiutil.ErrEmptyList
}
Expand Down
2 changes: 1 addition & 1 deletion users/api/requests_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -816,7 +816,7 @@ func TestUnassignUsersRequestValidate(t *testing.T) {
UserIDs: []string{validID},
Relation: "",
},
err: apiutil.ErrMissingRelation,
err: nil,
},
}
for _, c := range cases {
Expand Down
Loading