-
Notifications
You must be signed in to change notification settings - Fork 662
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-2216 - Rename delete policy function #2218
base: main
Are you sure you want to change the base?
MG-2216 - Rename delete policy function #2218
Conversation
2848916
to
158f23d
Compare
72da806
to
7bc5809
Compare
7bc5809
to
edf248a
Compare
@@ -1394,7 +1394,7 @@ func TestDeletePolicies(t *testing.T) { | |||
} | |||
|
|||
for _, tc := range cases { | |||
repocall := prepo.On("DeletePolicies", mock.Anything, mock.Anything).Return(tc.err) | |||
repocall := prepo.On("DeletePolicies", context.Background(), mock.Anything).Return(tc.err) | |||
err := svc.DeletePolicies(context.Background(), tc.pr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nyagamunene ,Why we are changing from mock.Anything
to context.Background()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When calling page, err := svc.ListAllObjects(context.Background(), tc.pr)
context.Background is passed to the repo call. This is a mistake that was not noted during the mock test
auth/api/grpc/endpoint_test.go
Outdated
deletePolicyReq *magistrala.DeletePolicyReq | ||
deletePolicyRes *magistrala.DeletePolicyRes | ||
deletePolicyReq *magistrala.DeletePolicyFilterReq | ||
deletePolicyRes *magistrala.DeletePolicyFilterRes | ||
err error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err error | |
deletePolicyFilterReq *magistrala.DeletePolicyFilterReq | |
deletePolicyFilterRes *magistrala.DeletePolicyFilterRes |
users/service_test.go
Outdated
@@ -1050,7 +1050,7 @@ func TestUpdateClientRole(t *testing.T) { | |||
desc: "update client role to user role with failed to delete policy", | |||
identifyResponse: &magistrala.IdentityRes{UserId: client.ID}, | |||
authorizeResponse: &magistrala.AuthorizeRes{Authorized: true}, | |||
deletePolicyResponse: &magistrala.DeletePolicyRes{Deleted: false}, | |||
deletePolicyResponse: &magistrala.DeletePolicyFilterRes{Deleted: false}, | |||
updateRoleResponse: mgclients.Client{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateRoleResponse: mgclients.Client{}, | |
deletePolicyFilterResponse: &magistrala.DeletePolicyFilterRes{Deleted: false}, |
users/service_test.go
Outdated
@@ -1059,7 +1059,7 @@ func TestUpdateClientRole(t *testing.T) { | |||
desc: "update client role to user role with failed to delete policy with error", | |||
identifyResponse: &magistrala.IdentityRes{UserId: client.ID}, | |||
authorizeResponse: &magistrala.AuthorizeRes{Authorized: true}, | |||
deletePolicyResponse: &magistrala.DeletePolicyRes{Deleted: false}, | |||
deletePolicyResponse: &magistrala.DeletePolicyFilterRes{Deleted: false}, | |||
updateRoleResponse: mgclients.Client{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateRoleResponse: mgclients.Client{}, | |
deletePolicyFilterResponse: &magistrala.DeletePolicyFilterRes{Deleted: false}, |
users/service_test.go
Outdated
@@ -1071,7 +1071,7 @@ func TestUpdateClientRole(t *testing.T) { | |||
identifyResponse: &magistrala.IdentityRes{UserId: client.ID}, | |||
authorizeResponse: &magistrala.AuthorizeRes{Authorized: true}, | |||
addPolicyResponse: &magistrala.AddPolicyRes{Added: true}, | |||
deletePolicyResponse: &magistrala.DeletePolicyRes{Deleted: true}, | |||
deletePolicyResponse: &magistrala.DeletePolicyFilterRes{Deleted: true}, | |||
updateRoleResponse: mgclients.Client{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateRoleResponse: mgclients.Client{}, | |
deletePolicyFilterResponse: &magistrala.DeletePolicyFilterRes{Deleted: true}, |
users/service_test.go
Outdated
@@ -1083,7 +1083,7 @@ func TestUpdateClientRole(t *testing.T) { | |||
identifyResponse: &magistrala.IdentityRes{UserId: client.ID}, | |||
authorizeResponse: &magistrala.AuthorizeRes{Authorized: true}, | |||
addPolicyResponse: &magistrala.AddPolicyRes{Added: true}, | |||
deletePolicyResponse: &magistrala.DeletePolicyRes{Deleted: false}, | |||
deletePolicyResponse: &magistrala.DeletePolicyFilterRes{Deleted: false}, | |||
updateRoleResponse: mgclients.Client{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateRoleResponse: mgclients.Client{}, | |
deletePolicyFilterResponse: &magistrala.DeletePolicyFilterRes{Deleted: false}, |
auth.proto
Outdated
message DeletePoliciesReq { | ||
repeated DeletePolicyReq deletePoliciesReq = 1; | ||
repeated DeletePolicyFilterReq deletePoliciesReq = 1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets have seperate message for DeletePoliciesReq
, DeletePolicyReq
is different from DeletePolicyFilterReq
In DeletePolicies
all fields are required , but in DeletePolicyFilter
not all the fields are required.
DeletePolicyFilter
filters the relation and then delete.
message DeletePoliciesReq { | |
repeated DeletePolicyReq deletePoliciesReq = 1; | |
repeated DeletePolicyFilterReq deletePoliciesReq = 1; | |
} | |
message DeletePolicyReq { | |
string domain = 1; | |
string subject_type = 2; | |
string subject_relation = 3; | |
string subject_kind = 4; | |
string subject = 5; | |
string relation = 6; | |
string permission = 7; | |
string object = 8; | |
string object_kind = 9; | |
string object_type = 10; | |
} | |
message DeletePoliciesReq { | |
repeated DeletePolicyReq DeletePolicyReq = 1; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I clarification on this is needed. Should I create a a separate DeletePolicyFilterRes
and DeletePolicyFilterReq
to be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
edf248a
to
271320b
Compare
c15818b
to
f0e8c67
Compare
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
Signed-off-by: nyagamunene <[email protected]>
f0e8c67
to
39482cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nyagamunene I will approve this, however upon testing I found out that UnshareThing
does not check whether the provided userID is actually a user in the platform. Additionally, unshare thing with a correct thing ID and a random user ID passes successfully, and it can be done multiple times successfully.
Please test this on your end and create an issue for it so that we do not forget it.
Signed-off-by: nyagamunene <[email protected]>
c8962d2
auth.proto
Outdated
@@ -116,10 +116,10 @@ message DeletePolicyReq { | |||
} | |||
|
|||
message DeletePoliciesReq { | |||
repeated DeletePolicyReq deletePoliciesReq = 1; | |||
repeated DeletePolicyFilterReq deletePoliciesReq = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repeated DeletePolicyFilterReq deletePoliciesReq = 1; | |
message DeletePolicyFilterReq { | |
string domain = 1; | |
string subject_type = 2; | |
string subject_relation = 3; | |
string subject_kind = 4; | |
string subject = 5; | |
string relation = 6; | |
string permission = 7; | |
string object = 8; | |
string object_kind = 9; | |
string object_type = 10; | |
} | |
message DeletePolicyFilterRes { bool deleted = 1; } | |
message DeletePoliciesReq { | |
repeated DeletePolicyReq deletePoliciesReq = 1; | |
} | |
message DeletePolicyReq { | |
string domain = 1; | |
string subject_type = 2; | |
string subject_relation = 3; | |
string subject_kind = 4; | |
string subject = 5; | |
string relation = 6; | |
string permission = 7; | |
string object = 8; | |
string object_kind = 9; | |
string object_type = 10; | |
} | |
Signed-off-by: nyagamunene <[email protected]>
What type of PR is this?
This is a feature that renames the
DeletePolicy
gRPC function toDeletePolicyFilter
policy with relationship filter.What does this do?
It rename gRPC, gRPC request & SpiceDB function
DeletePolicy
toDeletePolicyFilter
Which issue(s) does this PR fix/relate to?
DeletePolicy
gRPC & SpiceDB function toDeletePolicyFilter
and #2216DeletePolicy
gRPC & SpiceDB function toDeletePolicyFilter
and #2216Have you included tests for your changes?
Yes.
Did you document any new/modified feature?
No.
Notes