NOISSUE - Check For Subject During Policy Addition #1826

rodneyosodo · 2023-06-19T19:54:24Z

Signed-off-by: rodneyosodo blackd0t@protonmail.com

What does this do?

On things services check if the subject is a thing or a user.

If the subject is a thing, check the following:

The user adding the policy is the owner of the thing
The user adding the policy has c_share action on the thing

If the subject is a user, check the following:

The user adding the policy is the owner of the user
The user adding the policy has c_share action on the user

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

No issue

List any changes that modify/break current functionality

Ensures that the object column in the policies table references the id column of the groups table.
Check for subject in addition to checking for object

Have you included tests for your changes?

Yes

Did you document any new/modified functionality?

No

Notes

To be merged after https://github.com/mainflux/mainflux/pull/1825

codecov · 2023-06-20T07:49:29Z

Codecov Report

Merging #1826 (ceeaa85) into master (8c17841) will decrease coverage by 0.04%.
The diff coverage is 56.25%.

@@            Coverage Diff             @@
##           master    #1826      +/-   ##
==========================================
- Coverage   67.44%   67.40%   -0.04%     
==========================================
  Files         118      118              
  Lines        9470     9483      +13     
==========================================
+ Hits         6387     6392       +5     
- Misses       2404     2408       +4     
- Partials      679      683       +4

Files Changed	Coverage Δ
things/groups/service.go	`74.57% <ø> (ø)`
things/policies/policies.go	`77.27% <ø> (ø)`
users/groups/service.go	`67.96% <ø> (ø)`
things/clients/service.go	`73.82% <50.00%> (ø)`
users/clients/service.go	`59.84% <50.00%> (ø)`
things/policies/service.go	`53.43% <55.55%> (-1.65%)`	⬇️
users/policies/policies.go	`88.63% <100.00%> (ø)`
users/policies/service.go	`65.06% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

dborovcanin

Please resolve conflicts and remove duplicate APIs (ShareClient = AddPolicy). Don't forget to update API docs accordingly.

rodneyosodo · 2023-06-20T10:46:38Z

Please resolve conflicts and remove duplicate APIs (ShareClient = AddPolicy). Don't forget to update API docs accordingly.

That is done at https://github.com/mainflux/mainflux/pull/1825

dborovcanin · 2023-06-20T11:10:52Z

things/policies/service.go

@@ -101,7 +107,7 @@ func (svc service) Authorize(ctx context.Context, ar AccessRequest) (Policy, err
 //  1. The client is admin
 //
 //  2. The client has `g_add` action on the object or is the owner of the object.
-func (svc service) AddPolicy(ctx context.Context, token string, p Policy) (Policy, error) {
+func (svc service) AddPolicy(ctx context.Context, token, client string, p Policy) (Policy, error) {


Let's make the client param bool flag external. At the moment, external indicates anything that's not a Thing, and later we'll add verification if it's a user.

Don't forget to document this param, it's very important.

things/policies/service.go

dborovcanin · 2023-06-29T22:20:15Z

things/policies/service.go

+
+		return errors.ErrAuthorization
+	default:
+		return errors.New("invalid client")


Extract the error to a new exported var.

dborovcanin · 2023-06-29T22:26:24Z

users/policies/policies.go

-var PolicyTypes = []string{"g_add", "g_delete", "g_update", "g_list", "c_delete", "c_update", "c_list", "m_write", "m_read"}
+//
+// Sharing policies
+//  11. c_share - share a client


Elaborate: Allowed to add members of the group to the other groups.

arvindh123 · 2023-07-05T10:36:23Z

LGTM

SammyOina

Tested, LGTM

dborovcanin · 2023-07-10T13:39:08Z

things/policies/service.go

+	ReadAction        = "m_read"
+	WriteAction       = "m_write"
+	addPolicyAction   = "g_add"
+	sharePolicyAction = "c_share"
+	ClientEntityType  = "client"
+	GroupEntityType   = "group"
+	ThingEntityType   = "thing"
+	thingsObjectKey   = "things"


Separate logically.

pkg/sdk/go/things_test.go

dborovcanin · 2023-08-02T10:54:33Z

things/clients/service.go

@@ -16,15 +16,21 @@ import (
 )

 const (
-	MyKey             = "mine"
-	thingsObjectKey   = "things"
+	MyKey = "mine"


Is there a reason to export this (except for tests)? If not, please don't export.

dborovcanin · 2023-08-02T10:55:20Z

things/clients/service.go

 )

-var AdminRelationKey = []string{updateRelationKey, listRelationKey, deleteRelationKey}
+var AdminRelationKey = []string{updateRelationKey, listRelationKey, deleteRelationKey, shareRelationKey}


Comment exported vars. Also, rename to AdminRelationKeys.

dborovcanin · 2023-08-02T14:49:10Z

things/policies/service.go

+	switch external {
+	case false:
+		ar := AccessRequest{Subject: userID, Object: p.Subject, Action: sharePolicyAction}
+		if _, err := svc.policies.EvaluateThingAccess(ctx, ar); err != nil {
+			return err
+		}
+	case true:
+		if err := svc.usersAuthorize(ctx, userID, p.Subject, sharePolicyAction, ClientEntityType); err != nil {
+			return err
+		}
+	}
+
+	return nil


Suggested change

switch external {

case false:

ar := AccessRequest{Subject: userID, Object: p.Subject, Action: sharePolicyAction}

if _, err := svc.policies.EvaluateThingAccess(ctx, ar); err != nil {

return err

}

case true:

if err := svc.usersAuthorize(ctx, userID, p.Subject, sharePolicyAction, ClientEntityType); err != nil {

return err

}

}

return nil

switch {

case external:

ar := AccessRequest{Subject: userID, Object: p.Subject, Action: sharePolicyAction}

if _, err := svc.policies.EvaluateThingAccess(ctx, ar); err != nil {

return err

}

default:

if err := svc.usersAuthorize(ctx, userID, p.Subject, sharePolicyAction, ClientEntityType); err != nil {

return err

}

}

return nil

This looks a little bit more elegant. Or use simple if instead.

dborovcanin · 2023-08-02T14:52:56Z

users/clients/service.go

@@ -17,12 +17,15 @@ import (
 )

 const (
-	MyKey             = "mine"
-	clientsObjectKey  = "clients"
+	MyKey = "mine"


Same as previous Things remarks.

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

drasko

LGTM

* Check For Subject During Adding Policies Signed-off-by: rodneyosodo <blackd0t@protonmail.com> * Make Object to be Group ID Signed-off-by: rodneyosodo <blackd0t@protonmail.com> * Fix Tests Signed-off-by: rodneyosodo <blackd0t@protonmail.com> * Change from string to bool Signed-off-by: rodneyosodo <blackd0t@protonmail.com> * Update Tests Signed-off-by: rodneyosodo <blackd0t@protonmail.com> * When it is Admin Don't Check Subject Signed-off-by: rodneyosodo <blackd0t@protonmail.com> * Minor Refractoring Signed-off-by: rodneyosodo <blackd0t@protonmail.com> * Group Constants Signed-off-by: rodneyosodo <blackd0t@protonmail.com> * Test if User Doesn't Have Policy Signed-off-by: rodneyosodo <blackd0t@protonmail.com> * Combine Share Things Cases Signed-off-by: rodneyosodo <blackd0t@protonmail.com> * Remove Unnecessary Case Signed-off-by: rodneyosodo <blackd0t@protonmail.com> * Check For Non NIL error Signed-off-by: rodneyosodo <blackd0t@protonmail.com> * Remove 3 Cases From Bool Signed-off-by: rodneyosodo <blackd0t@protonmail.com> * Add Listing Actions Incase of Sharing Signed-off-by: rodneyosodo <blackd0t@protonmail.com> * Capitalize comments Signed-off-by: rodneyosodo <blackd0t@protonmail.com> * Move AdminRelationKeys to Tests Signed-off-by: rodneyosodo <blackd0t@protonmail.com> * Fix Tests After Rebase Signed-off-by: rodneyosodo <blackd0t@protonmail.com> * Rename myKey Signed-off-by: rodneyosodo <blackd0t@protonmail.com> * Simplify checkSubject Signed-off-by: rodneyosodo <blackd0t@protonmail.com> --------- Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

rodneyosodo marked this pull request as ready for review June 20, 2023 08:03

rodneyosodo requested a review from a team as a code owner June 20, 2023 08:03

dborovcanin requested changes Jun 20, 2023

View reviewed changes

rodneyosodo force-pushed the NOISSUE-checksub branch from 8606ca8 to 87d82b5 Compare June 20, 2023 10:45

dborovcanin requested changes Jun 20, 2023

View reviewed changes

rodneyosodo force-pushed the NOISSUE-checksub branch 5 times, most recently from 473df09 to c02a4fd Compare June 26, 2023 15:37

dborovcanin requested changes Jun 29, 2023

View reviewed changes

things/policies/service.go Outdated Show resolved Hide resolved

dborovcanin requested changes Jun 29, 2023

View reviewed changes

rodneyosodo force-pushed the NOISSUE-checksub branch 4 times, most recently from 292e3b7 to 97363bc Compare July 10, 2023 12:30

SammyOina approved these changes Jul 10, 2023

View reviewed changes

rodneyosodo mentioned this pull request Jul 11, 2023

NOISSUE - Unify group and clients implementations on things and users #1793

Merged

rodneyosodo force-pushed the NOISSUE-checksub branch 8 times, most recently from 9abfa50 to fba79ab Compare July 13, 2023 10:56

dborovcanin requested changes Jul 13, 2023

View reviewed changes

dborovcanin requested changes Aug 2, 2023

View reviewed changes

rodneyosodo force-pushed the NOISSUE-checksub branch 2 times, most recently from e90ae3e to ca5b8b5 Compare August 2, 2023 13:29

dborovcanin requested changes Aug 2, 2023

View reviewed changes

rodneyosodo added 17 commits August 2, 2023 17:20

Check For Subject During Adding Policies

9eb43c2

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Make Object to be Group ID

f499ed9

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Fix Tests

1875fce

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Change from string to bool

b20ee0a

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Update Tests

9e2c385

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

When it is Admin Don't Check Subject

e2bed20

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Minor Refractoring

d6688f1

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Group Constants

dace17e

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Test if User Doesn't Have Policy

ae3aff4

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Combine Share Things Cases

0518ed8

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Remove Unnecessary Case

f460bc6

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Check For Non NIL error

1aadb43

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Remove 3 Cases From Bool

b3b6ce9

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Add Listing Actions Incase of Sharing

5d0fdb1

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Capitalize comments

7e27e61

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Move AdminRelationKeys to Tests

6934a81

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Fix Tests After Rebase

1964595

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

dborovcanin force-pushed the NOISSUE-checksub branch from f70caa1 to 8458de1 Compare August 2, 2023 15:20

Rename myKey

102b2c8

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

rodneyosodo force-pushed the NOISSUE-checksub branch from 8458de1 to 102b2c8 Compare August 2, 2023 16:09

Simplify checkSubject

ceeaa85

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

dborovcanin approved these changes Aug 2, 2023

View reviewed changes

drasko approved these changes Aug 2, 2023

View reviewed changes

drasko merged commit 1d80301 into absmach:master Aug 2, 2023
1 of 3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NOISSUE - Check For Subject During Policy Addition #1826

NOISSUE - Check For Subject During Policy Addition #1826

rodneyosodo commented Jun 19, 2023 •

edited

Loading

codecov bot commented Jun 20, 2023 •

edited

Loading

dborovcanin left a comment

rodneyosodo commented Jun 20, 2023

dborovcanin Jun 20, 2023

dborovcanin Jun 20, 2023

dborovcanin Jun 29, 2023

dborovcanin Jun 29, 2023

arvindh123 commented Jul 5, 2023

SammyOina left a comment

dborovcanin Jul 10, 2023

dborovcanin Aug 2, 2023

dborovcanin Aug 2, 2023

dborovcanin Aug 2, 2023

dborovcanin Aug 2, 2023

drasko left a comment

NOISSUE - Check For Subject During Policy Addition #1826

NOISSUE - Check For Subject During Policy Addition #1826

Conversation

rodneyosodo commented Jun 19, 2023 • edited Loading

What does this do?

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

List any changes that modify/break current functionality

Have you included tests for your changes?

Did you document any new/modified functionality?

Notes

codecov bot commented Jun 20, 2023 • edited Loading

Codecov Report

dborovcanin left a comment

Choose a reason for hiding this comment

rodneyosodo commented Jun 20, 2023

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

arvindh123 commented Jul 5, 2023

SammyOina left a 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

drasko left a comment

Choose a reason for hiding this comment

rodneyosodo commented Jun 19, 2023 •

edited

Loading

codecov bot commented Jun 20, 2023 •

edited

Loading