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

bootstrap/api/requests.go

bootstrap/service.go

bootstrap/api/requests.go

bootstrap/configs.go

bootstrap/service.go

rodneyosodo · 2024-06-05T10:52:39Z

bootstrap/service.go

+ return bs.configs.RetrieveAll(ctx, user.GetDomainId(), "", filter, offset, limit), nil
+ }
+
+ // Check user is admin of domain, if yes then show listing on domain context.


Remove comment

rodneyosodo · 2024-06-05T10:53:44Z

bootstrap/service.go

+ return bs.configs.RetrieveAll(ctx, user.GetDomainId(), "", filter, offset, limit), nil
+ }
+
+ rtids, err := bs.listClientIDs(ctx, user.GetId(), auth.ViewPermission)


Suggested change

rtids, err := bs.listClientIDs(ctx, user.GetId(), auth.ViewPermission)

rtids, err := bs.listClientIDs(ctx, user.GetId())

Since we are using it at one place

rodneyosodo · 2024-06-05T10:53:49Z

bootstrap/service.go

+ return ConfigsPage{}, errors.Wrap(svcerr.ErrNotFound, err)
+ }
+
+ thingIDs, err := bs.filterAllowedThingIDs(ctx, user.GetId(), auth.ViewPermission, rtids)


bootstrap/service.go

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

Signed-off-by: JeffMboya <[email protected]> refactor: add authorization to view method Signed-off-by: JeffMboya <[email protected]>

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

Signed-off-by: JeffMboya <[email protected]> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <[email protected]> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <[email protected]> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <[email protected]> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <[email protected]> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <[email protected]> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <[email protected]> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <[email protected]> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <[email protected]> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <[email protected]> refactor: add authorization checks to UpdateCert, UpdateConnections, and Remove methods Signed-off-by: JeffMboya <[email protected]> refactor: add authorization checks to UpdateCert, and UpdateConnections methods Signed-off-by: JeffMboya <[email protected]>

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

… and Remove methods Signed-off-by: JeffMboya <[email protected]>

…meter Signed-off-by: JeffMboya <[email protected]>

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

…meter Signed-off-by: JeffMboya <[email protected]>

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

…rameter Signed-off-by: JeffMboya <[email protected]>

rodneyosodo · 2024-06-06T14:05:57Z

bootstrap/postgres/configs.go

+
+ if domainID != "" {
+ params = append(params, domainID)
+ queries = append(queries, fmt.Sprintf("domain_id = $%d", len(params)))


Why not?

Suggested change

queries = append(queries, fmt.Sprintf("domain_id = $%d", len(params)))

queries = append(queries, fmt.Sprintf("domain_id = %s", domainID))

rodneyosodo · 2024-06-06T14:06:18Z

bootstrap/service.go

 if err != nil {
 return Config{}, errors.Wrap(svcerr.ErrAuthentication, err)
 }
+ // If domain is disabled , then this authorization will fail for all non-admin domain users.


Remove this comment

rodneyosodo · 2024-06-06T14:07:41Z

bootstrap/service.go

+ return bs.configs.RetrieveAll(ctx, user.GetDomainId(), []string{}, filter, offset, limit), nil
+ }
+
+ _, authErr := bs.authorize(ctx, "", auth.UsersKind, user.GetId(), auth.AdminPermission, auth.DomainType, user.GetDomainId())


Suggested change

_, authErr := bs.authorize(ctx, "", auth.UsersKind, user.GetId(), auth.AdminPermission, auth.DomainType, user.GetDomainId())

if _, err := bs.authorize(ctx, "", auth.UsersKind, user.GetId(), auth.AdminPermission, auth.DomainType, user.GetDomainId()); err != nil {

rodneyosodo · 2024-06-06T14:08:43Z

bootstrap/service.go

 ctx, cancel := context.WithTimeout(ctx, time.Second)
 defer cancel()

 res, err := bs.auth.Identify(ctx, &magistrala.IdentityReq{Token: token})
 if err != nil {
- return "", errors.Wrap(svcerr.ErrAuthentication, err)
+ return nil, svcerr.ErrAuthentication


wrap the error

Suggested change

return nil, svcerr.ErrAuthentication

return nil, errors.Wrap(svcerr.ErrAuthentication, err)

rodneyosodo · 2024-06-06T14:09:13Z

bootstrap/service.go

+ }
+ res, err := bs.auth.Authorize(ctx, req)
+ if err != nil {
+ return "", svcerr.ErrAuthorization


same

Suggested change

return "", svcerr.ErrAuthorization

return "", errors.Wrap(svcerr.ErrAuthorization, err)

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 e691dd7 to b4fc010 Compare May 26, 2024 07:44

rodneyosodo requested changes May 27, 2024

View reviewed changes

rodneyosodo requested changes May 28, 2024

View reviewed changes

bootstrap/api/requests.go Outdated Show resolved Hide resolved

bootstrap/service.go Outdated Show resolved Hide resolved

bootstrap/service.go Outdated Show resolved Hide resolved

bootstrap/service.go Outdated Show resolved Hide resolved

bootstrap/service.go Outdated Show resolved Hide resolved

arvindh123 requested changes May 29, 2024

View reviewed changes

bootstrap/service.go Outdated Show resolved Hide resolved

bootstrap/service.go Outdated Show resolved Hide resolved

rodneyosodo requested changes Jun 5, 2024

View reviewed changes

JeffMboya added 15 commits June 5, 2024 14:15

refactor: add authorization to view method

1a34377

Signed-off-by: JeffMboya <[email protected]> refactor: add authorization to view method Signed-off-by: JeffMboya <[email protected]>

refactor: add authorization to update method

01a1a61

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

refactor: use configID in list and view methods

0c4d248

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

refactor: replace TokenKind with UsersKind

7814304

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

fix: failing cases in TestView

1dd4dae

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

fix: failing cases in TestView

6afd020

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

refactor: add authorization checks for UpdateCert, UpdateConnections,…

484d629

… and Remove methods Signed-off-by: JeffMboya <[email protected]>

refactor: add authorization checks for UpdateCert, UpdateConnections,…

df71835

… and Remove methods Signed-off-by: JeffMboya <[email protected]>

refactor: update RetrieveAll method signature to include thingID para…

343e563

…meter Signed-off-by: JeffMboya <[email protected]>

refactor: replace if with switch

d4a6bca

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

refactor: update RetrieveAll method signature to include thingID para…

d7ca23b

…meter Signed-off-by: JeffMboya <[email protected]>

refactor: update RetrieveAll method signature to include thingID para…

fbe0d17

…meter Signed-off-by: JeffMboya <[email protected]>

refactor: update RetrieveAll method signature to include thingID para…

8238e8d

…meter Signed-off-by: JeffMboya <[email protected]>

JeffMboya force-pushed the MG-1955 branch from b0ee5e6 to 8238e8d Compare June 5, 2024 11:15

JeffMboya added 4 commits June 5, 2024 17:12

refactor: update RetrieveAll method signature to include thingID para…

e6513c9

…meter Signed-off-by: JeffMboya <[email protected]>

refactor: Added early return in List if thingIDs is empty

e1a168e

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

refactor: Update List method to include empty string for domain ID pa…

e3073de

…rameter Signed-off-by: JeffMboya <[email protected]>

refactor: Update List method to include empty string for domain ID pa…

af40e10

…rameter Signed-off-by: JeffMboya <[email protected]>

rodneyosodo requested changes Jun 6, 2024

View reviewed changes

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 27, 2024

rodneyosodo Jun 5, 2024

rodneyosodo Jun 5, 2024

rodneyosodo Jun 5, 2024

rodneyosodo Jun 6, 2024

rodneyosodo Jun 6, 2024

rodneyosodo Jun 6, 2024

rodneyosodo Jun 6, 2024

rodneyosodo Jun 6, 2024

	rtids, err := bs.listClientIDs(ctx, user.GetId(), auth.ViewPermission)
	rtids, err := bs.listClientIDs(ctx, user.GetId())

	queries = append(queries, fmt.Sprintf("domain_id = $%d", len(params)))
	queries = append(queries, fmt.Sprintf("domain_id = %s", domainID))

	_, authErr := bs.authorize(ctx, "", auth.UsersKind, user.GetId(), auth.AdminPermission, auth.DomainType, user.GetDomainId())
	if _, err := bs.authorize(ctx, "", auth.UsersKind, user.GetId(), auth.AdminPermission, auth.DomainType, user.GetDomainId()); err != nil {

	return nil, svcerr.ErrAuthentication
	return nil, errors.Wrap(svcerr.ErrAuthentication, err)

	return "", svcerr.ErrAuthorization
	return "", errors.Wrap(svcerr.ErrAuthorization, err)

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