MG-2216 - Rename delete policy function #2218

nyagamunene · 2024-05-06T16:51:07Z

What type of PR is this?

This is a feature that renames the DeletePolicy gRPC function to DeletePolicyFilter policy with relationship filter.

What does this do?

It rename gRPC, gRPC request & SpiceDB function DeletePolicy to DeletePolicyFilter

Which issue(s) does this PR fix/relate to?

Related Issue Feature: Rename DeletePolicy gRPC & SpiceDB function to DeletePolicyFilter and #2216
Resolves Feature: Rename DeletePolicy gRPC & SpiceDB function to DeletePolicyFilter and #2216

Have you included tests for your changes?

Yes.

Did you document any new/modified feature?

No.

Notes

auth/api/logging.go

auth/api/metrics.go

arvindh123 · 2024-05-20T13:08:09Z

auth/service_test.go

@@ -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)


@nyagamunene ,Why we are changing from mock.Anything to context.Background()

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/policies.go

auth/api/logging.go

arvindh123 · 2024-05-20T13:11:27Z

auth/api/grpc/endpoint_test.go

- deletePolicyReq *magistrala.DeletePolicyReq
- deletePolicyRes *magistrala.DeletePolicyRes
+ deletePolicyReq *magistrala.DeletePolicyFilterReq
+ deletePolicyRes *magistrala.DeletePolicyFilterRes
 err error


Suggested change

err error

deletePolicyFilterReq *magistrala.DeletePolicyFilterReq

deletePolicyFilterRes *magistrala.DeletePolicyFilterRes

arvindh123 · 2024-05-20T13:12:55Z

users/service_test.go

@@ -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{},


Suggested change

updateRoleResponse: mgclients.Client{},

deletePolicyFilterResponse: &magistrala.DeletePolicyFilterRes{Deleted: false},

arvindh123 · 2024-05-20T13:12:59Z

users/service_test.go

@@ -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{},


Suggested change

updateRoleResponse: mgclients.Client{},

deletePolicyFilterResponse: &magistrala.DeletePolicyFilterRes{Deleted: false},

arvindh123 · 2024-05-20T13:13:04Z

users/service_test.go

@@ -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{},


Suggested change

updateRoleResponse: mgclients.Client{},

deletePolicyFilterResponse: &magistrala.DeletePolicyFilterRes{Deleted: true},

arvindh123 · 2024-05-20T13:13:11Z

users/service_test.go

@@ -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{},


Suggested change

updateRoleResponse: mgclients.Client{},

deletePolicyFilterResponse: &magistrala.DeletePolicyFilterRes{Deleted: false},

arvindh123 · 2024-05-20T13:18:34Z

auth.proto

 message DeletePoliciesReq {
- repeated DeletePolicyReq deletePoliciesReq = 1;
+ repeated DeletePolicyFilterReq deletePoliciesReq = 1;
 }


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.

Suggested change

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;

}

I clarification on this is needed. Should I create a a separate DeletePolicyFilterRes and DeletePolicyFilterReq to be used?

auth.proto

Signed-off-by: nyagamunene <[email protected]>

rodneyosodo

LGTM

WashingtonKK

@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.

auth/policies.go

Signed-off-by: nyagamunene <[email protected]>

auth/api/grpc/endpoint_test.go

arvindh123 · 2024-05-28T11:41:50Z

auth.proto

@@ -116,10 +116,10 @@ message DeletePolicyReq {
 }

 message DeletePoliciesReq {
- repeated DeletePolicyReq deletePoliciesReq = 1;
+ repeated DeletePolicyFilterReq deletePoliciesReq = 1;


Suggested change

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]>

nyagamunene self-assigned this May 6, 2024

nyagamunene force-pushed the MG-2216-RenameDeletePolicygRPCSpiceDBfunction branch 2 times, most recently from 2848916 to 158f23d Compare May 7, 2024 15:56

nyagamunene marked this pull request as ready for review May 8, 2024 10:28

dborovcanin force-pushed the MG-2216-RenameDeletePolicygRPCSpiceDBfunction branch from 72da806 to 7bc5809 Compare May 13, 2024 09:57

nyagamunene changed the title ~~MG-2216-RenameDeletePolicygRPCSpiceDBfunction~~ MG-2216-RenameDeletePolicyFunction May 13, 2024

nyagamunene requested a review from dborovcanin May 13, 2024 10:16

nyagamunene changed the title ~~MG-2216-RenameDeletePolicyFunction~~ MG-2216-Rename Delete Policy Function May 14, 2024

nyagamunene changed the title ~~MG-2216-Rename Delete Policy Function~~ MG-2216-Rename delete policy function May 14, 2024

dborovcanin force-pushed the MG-2216-RenameDeletePolicygRPCSpiceDBfunction branch from 7bc5809 to edf248a Compare May 15, 2024 12:27

rodneyosodo requested changes May 17, 2024

View reviewed changes

auth/api/logging.go Show resolved Hide resolved

auth/api/metrics.go Show resolved Hide resolved

arvindh123 mentioned this pull request May 20, 2024

NOISSUE - Remove redundant relation check #2197

Open

arvindh123 requested changes May 20, 2024

View reviewed changes

nyagamunene force-pushed the MG-2216-RenameDeletePolicygRPCSpiceDBfunction branch from edf248a to 271320b Compare May 20, 2024 22:02

nyagamunene changed the title ~~MG-2216-Rename delete policy function~~ MG-2216 - Rename delete policy function May 21, 2024

rodneyosodo requested changes May 21, 2024

View reviewed changes

auth.proto Outdated Show resolved Hide resolved

rodneyosodo requested a review from JeffMboya May 22, 2024 10:14

nyagamunene force-pushed the MG-2216-RenameDeletePolicygRPCSpiceDBfunction branch from c15818b to f0e8c67 Compare May 23, 2024 10:09

nyagamunene added 9 commits May 23, 2024 13:11

Renaming Delete Policy in spiceDB

b6a1d70

Signed-off-by: nyagamunene <[email protected]>

Renaming Delete Policy in gRPC

577ea8e

Signed-off-by: nyagamunene <[email protected]>

Renaming Delete Policy in gRPC 2

7b5063c

Signed-off-by: nyagamunene <[email protected]>

Renaming Delete Policy in gRPC 3

cec51cd

Signed-off-by: nyagamunene <[email protected]>

Updated proto file and tests

169e552

Signed-off-by: nyagamunene <[email protected]>

Debugged internals test

c6da68a

Signed-off-by: nyagamunene <[email protected]>

changed auth.proto messages

75ed44b

Signed-off-by: nyagamunene <[email protected]>

Updated test

e269619

Signed-off-by: nyagamunene <[email protected]>

Updated the proto file

39482cf

Signed-off-by: nyagamunene <[email protected]>

nyagamunene force-pushed the MG-2216-RenameDeletePolicygRPCSpiceDBfunction branch from f0e8c67 to 39482cf Compare May 23, 2024 10:11

rodneyosodo previously approved these changes May 23, 2024

View reviewed changes

WashingtonKK previously approved these changes May 23, 2024

View reviewed changes

JeffMboya previously approved these changes May 23, 2024

View reviewed changes

arvindh123 requested changes May 24, 2024

View reviewed changes

auth/policies.go Outdated Show resolved Hide resolved

Fixed comments to be more clear

c8962d2

Signed-off-by: nyagamunene <[email protected]>

nyagamunene dismissed stale reviews from JeffMboya, WashingtonKK, and rodneyosodo via c8962d2 May 24, 2024 08:36

arvindh123 requested changes May 28, 2024

View reviewed changes

auth/api/grpc/endpoint_test.go Show resolved Hide resolved

arvindh123 requested changes May 28, 2024

View reviewed changes

Added delete policy request

4cb789b

Signed-off-by: nyagamunene <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MG-2216 - Rename delete policy function #2218

MG-2216 - Rename delete policy function #2218

nyagamunene commented May 6, 2024 •

edited

arvindh123 May 20, 2024

nyagamunene May 20, 2024

arvindh123 May 20, 2024

arvindh123 May 20, 2024

arvindh123 May 20, 2024

arvindh123 May 20, 2024

arvindh123 May 20, 2024

arvindh123 May 20, 2024

nyagamunene May 23, 2024

arvindh123 May 24, 2024

rodneyosodo left a comment

WashingtonKK left a comment •

edited

arvindh123 May 28, 2024

	err error
	deletePolicyFilterReq *magistrala.DeletePolicyFilterReq
	deletePolicyFilterRes *magistrala.DeletePolicyFilterRes

	updateRoleResponse: mgclients.Client{},
	deletePolicyFilterResponse: &magistrala.DeletePolicyFilterRes{Deleted: false},

-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;
+}

MG-2216 - Rename delete policy function #2218

Are you sure you want to change the base?

MG-2216 - Rename delete policy function #2218

Conversation

nyagamunene commented May 6, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodneyosodo left a comment

Choose a reason for hiding this comment

WashingtonKK left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyagamunene commented May 6, 2024 •

edited

WashingtonKK left a comment •

edited