MG-1955 - Bootstrap service is not synced to the latest changes of access control #2199

JeffMboya · 2024-04-25T11:04:47Z

What type of PR is this?

This PR is a bug fix.

What does this do?

This PR ensures that users can only view bootstrap configurations of the things within their current domain. It also allows domain members with appropriate permissions to view configurations for things within the domain, regardless of who created the configuration.

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

Resolves Bug: Bootstrap service is not synced to the latest changes of access control #1955

Have you included tests for your changes?

Yes, tests have been included in this PR.

Did you document any new/modified feature?

No new features were documented in this PR.

bootstrap/postgres/init.go

bootstrap/configs.go

bootstrap/postgres/configs.go

bootstrap/postgres/init.go

bootstrap/service.go

arvindh123 · 2024-05-20T11:23:22Z

bootstrap/service.go

+	for _, channel := range cfg.Channels {
+		if channel.ID == "" || channel.ID == "invalid" {
+			return Config{}, svcerr.ErrMalformedEntity
+		}
+	}


This can be moved to API layer

arvindh123 · 2024-05-20T11:26:44Z

bootstrap/service.go

+		return Config{}, errors.Wrap(svcerr.ErrViewEntity, err)
+	}
+
+	if thing.DomainID != user.GetDomainId() {


How this case will be possible?
Is there way to add to a thing which is not belongs same domain of user?

arvindh123 · 2024-05-20T12:01:47Z

bootstrap/service.go

 	if err != nil {
 		return errors.Wrap(svcerr.ErrAuthentication, err)
 	}
+	_, err = bs.authorize(ctx, "", auth.UserType, auth.UsersKind, user.GetId(), auth.EditPermission, auth.DomainType, user.GetDomainId())


@JeffMboya Why we are checking the user have edit access to Domain ?

bootstrap/api/endpoint.go

bootstrap/service.go

bootstrap/postgres/configs.go

bootstrap/service.go

bootstrap/api/requests_test.go

bootstrap/service.go

rodneyosodo · 2024-05-23T08:43:17Z

bootstrap/service.go

 	if err != nil {
 		return Config{}, errors.Wrap(svcerr.ErrAuthentication, err)
 	}
-	cfg, err := bs.configs.RetrieveByID(ctx, owner, id)
+
+	// Retrieve the bootstrap configuration.


Same as things service we need to authorize the users has ViewPermission to the thing if the boostrap config has thingID

This also applies to update operations EditPermission

Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain access control Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain access control Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain access control Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain access control Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain access control Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add authorization to identify method Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add authorize method Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add authorize method Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add authorize method Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add policies Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Remove policies Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorization Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Add domain level authorizaton Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Fix: add error return value to identify function Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Fix: add error return value to identify function Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Refactor: replace domain_id with domainID Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Refactor: replace domain_id with domainID Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Refactor: replace domain_id with domainID Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Refactor: replace 'Owner' with 'DomainID' Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Refactor: replace 'Owner' with 'DomainID' Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Fix (configs.go): remove unused context Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Feature: add configs with same name in different domains Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Feature: add configs with same name in different domains Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Fix: failing test dues to renaming Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Refactor: rename domainid to domain_id Signed-off-by: JeffMboya <jangina.mboya@gmail.com> fix: handle malformed channel in addEndpoint Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: update authorization method parameters Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: remove unnecessary authorization checks Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: remove unnecessary authorization checks Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: remove unnecessary authorization checks Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: remove unnecessary authorization checks Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: remove unnecessary authorization checks Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: remove unnecessary authorization checks Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: remove unnecessary authorization checks Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: remove unnecessary authorization checks Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: remove unnecessary authorization checks Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Fix: failing tests Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Fix: failing tests Signed-off-by: JeffMboya <jangina.mboya@gmail.com> Fix: failing tests Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: add testsutil package for generating UUIDs in tests Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: add empty channel tests Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: add valid request Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: remove comment Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: add authorization to view method Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: add authorization checks to UpdateCert, and UpdateConnections methods Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

rodneyosodo · 2024-05-27T07:40:47Z

bootstrap/service.go

 	if err != nil {
 		return errors.Wrap(svcerr.ErrAuthentication, err)
 	}

-	cfg.Owner = owner
+	cfg.DomainID = user.GetDomainId()
+	_, err = bs.authorize(ctx, "", auth.TokenKind, token, auth.EditPermission, auth.DomainType, cfg.DomainID)


Since we have userID from identify we can user usersKind and userID

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

… and Remove methods Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

rodneyosodo · 2024-05-28T13:02:47Z

bootstrap/api/requests.go

@@ -36,6 +36,16 @@ func (req addReq) validate() error {
 		return apiutil.ErrBearerKey
 	}

+	if len(req.Channels) == 0 {
+		return apiutil.ErrMissingID


Suggested change

return apiutil.ErrMissingID

return apiutil.ErrEmptyList

rodneyosodo · 2024-05-28T13:04:22Z

bootstrap/service.go

 	if err != nil {
 		return Config{}, errors.Wrap(svcerr.ErrAuthentication, err)
 	}
-	cfg, err := bs.configs.RetrieveByID(ctx, owner, id)
+	_, err = bs.authorize(ctx, user.GetDomainId(), auth.UsersKind, user.GetId(), auth.ViewPermission, auth.ThingType, id)


We should authorize if the bootstrap has a thingID

bootstrap/service.go

rodneyosodo · 2024-05-28T13:04:54Z

bootstrap/service.go

 	if err != nil {
 		return Config{}, errors.Wrap(svcerr.ErrAuthentication, err)
 	}
-	cfg, err := bs.configs.UpdateCert(ctx, owner, thingID, clientCert, clientKey, caCert)
+	_, err = bs.authorize(ctx, user.GetDomainId(), auth.UsersKind, user.GetId(), auth.EditPermission, auth.ThingType, thingID)


rodneyosodo · 2024-05-28T13:07:55Z

bootstrap/service.go

+
+	j := 0
+	for _, config := range configsPage.Configs {
+		_, err := bs.authorize(ctx, user.GetDomainId(), auth.UsersKind, user.GetId(), auth.ViewPermission, auth.ThingType, config.ThingID)


This doesn't seem good since we will make n calls to auth depending on the list of configs. You can check something like this #2168. So if they aren't domain admins they get basic information. @arvindh123 what do you think?

Yes, This should be like, first we need to get all thingIDs from spiceDB for user and then pass the thingIDs to config repo to retrieve the bootstrap config

arvindh123 · 2024-05-29T06:52:18Z

bootstrap/service.go

+
+	j := 0
+	for _, config := range configsPage.Configs {
+		_, err := bs.authorize(ctx, user.GetDomainId(), auth.UsersKind, user.GetId(), auth.ViewPermission, auth.ThingType, config.ThingID)


Yes, This should be like, first we need to get all thingIDs from spiceDB for user and then pass the thingIDs to config repo to retrieve the bootstrap config

bootstrap/service.go

…meter Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

JeffMboya changed the title ~~MG-1955 - Implement domains access control~~ MG-1955 - Bootstrap service is not synced to the latest changes of access control Apr 25, 2024

JeffMboya force-pushed the MG-1955 branch 2 times, most recently from 955c48b to 36871a7 Compare April 30, 2024 07:40

rodneyosodo requested changes Apr 30, 2024

View reviewed changes

bootstrap/postgres/init.go Outdated Show resolved Hide resolved

JeffMboya force-pushed the MG-1955 branch from 19333a1 to b722f52 Compare April 30, 2024 12:53

JeffMboya self-assigned this May 2, 2024

JeffMboya force-pushed the MG-1955 branch 3 times, most recently from a419da6 to 1fbba5b Compare May 6, 2024 08:17

JeffMboya marked this pull request as ready for review May 7, 2024 07:36

JeffMboya force-pushed the MG-1955 branch 2 times, most recently from 1c41e02 to d51ce36 Compare May 8, 2024 08:13

rodneyosodo requested changes May 8, 2024

View reviewed changes

bootstrap/configs.go Outdated Show resolved Hide resolved

bootstrap/configs.go Outdated Show resolved Hide resolved

bootstrap/postgres/configs.go Outdated Show resolved Hide resolved

bootstrap/postgres/init.go Show resolved Hide resolved

bootstrap/service.go Outdated Show resolved Hide resolved

JeffMboya force-pushed the MG-1955 branch 5 times, most recently from 26959ac to 2080e0a Compare May 16, 2024 09:38

JeffMboya force-pushed the MG-1955 branch 2 times, most recently from c43e6eb to 87c7b95 Compare May 20, 2024 11:59

arvindh123 requested changes May 20, 2024

View reviewed changes

arvindh123 force-pushed the MG-1955 branch from 517275a to 2d66e7c Compare May 20, 2024 13:26

JeffMboya force-pushed the MG-1955 branch from 13ed215 to cd96b97 Compare May 21, 2024 08:33

rodneyosodo requested changes May 21, 2024

View reviewed changes

JeffMboya force-pushed the MG-1955 branch 3 times, most recently from cf44a0a to 7fb4b09 Compare May 21, 2024 17:43

rodneyosodo requested a review from nyagamunene May 22, 2024 10:19

JeffMboya force-pushed the MG-1955 branch from 7fb4b09 to 7f0278b Compare May 22, 2024 18:16

rodneyosodo requested changes May 23, 2024

View reviewed changes

JeffMboya force-pushed the MG-1955 branch 5 times, most recently from e08a890 to 3a810ae Compare May 26, 2024 07:27

JeffMboya added 3 commits May 26, 2024 10:29

refactor: add authorization to view method

7dd8f77

Signed-off-by: JeffMboya <jangina.mboya@gmail.com> refactor: add authorization to view method Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

refactor: add authorization to update method

f4e60b2

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

JeffMboya force-pushed the MG-1955 branch from 3a810ae to 441f352 Compare May 26, 2024 07:29

JeffMboya force-pushed the MG-1955 branch from e691dd7 to b4fc010 Compare May 26, 2024 07:44

refactor: use configID in list and view methods

33fc537

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

rodneyosodo requested changes May 27, 2024

View reviewed changes

JeffMboya added 5 commits May 27, 2024 18:35

refactor: replace TokenKind with UsersKind

7204243

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

fix: failing cases in TestView

2b4dc92

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

fix: failing cases in TestView

5f144fa

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

refactor: add authorization checks for UpdateCert, UpdateConnections,…

859c799

… and Remove methods Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

refactor: add authorization checks for UpdateCert, UpdateConnections,…

5dde84c

… and Remove methods Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

rodneyosodo requested changes May 28, 2024

View reviewed changes

arvindh123 requested changes May 29, 2024

View reviewed changes

JeffMboya added 2 commits May 31, 2024 16:58

refactor: update RetrieveAll method signature to include thingID para…

fdee715

…meter Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

refactor: replace if with switch

af1c0ae

Signed-off-by: JeffMboya <jangina.mboya@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MG-1955 - Bootstrap service is not synced to the latest changes of access control #2199

MG-1955 - Bootstrap service is not synced to the latest changes of access control #2199

JeffMboya commented Apr 25, 2024 •

edited

arvindh123 May 20, 2024

arvindh123 May 20, 2024

arvindh123 May 20, 2024

rodneyosodo May 23, 2024

rodneyosodo May 23, 2024

rodneyosodo May 27, 2024

rodneyosodo May 28, 2024

rodneyosodo May 28, 2024

rodneyosodo May 28, 2024

rodneyosodo May 28, 2024

arvindh123 May 29, 2024

arvindh123 May 29, 2024

MG-1955 - Bootstrap service is not synced to the latest changes of access control #2199

Are you sure you want to change the base?

MG-1955 - Bootstrap service is not synced to the latest changes of access control #2199

Conversation

JeffMboya commented Apr 25, 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?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffMboya commented Apr 25, 2024 •

edited