-
Notifications
You must be signed in to change notification settings - Fork 663
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-1955 - Bootstrap service is not synced to the latest changes of access control #2199
base: main
Are you sure you want to change the base?
Conversation
955c48b
to
36871a7
Compare
a419da6
to
1fbba5b
Compare
1c41e02
to
d51ce36
Compare
26959ac
to
2080e0a
Compare
c43e6eb
to
87c7b95
Compare
bootstrap/service.go
Outdated
for _, channel := range cfg.Channels { | ||
if channel.ID == "" || channel.ID == "invalid" { | ||
return Config{}, svcerr.ErrMalformedEntity | ||
} | ||
} |
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.
This can be moved to API layer
bootstrap/service.go
Outdated
return Config{}, errors.Wrap(svcerr.ErrViewEntity, err) | ||
} | ||
|
||
if thing.DomainID != user.GetDomainId() { |
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.
How this case will be possible?
Is there way to add to a thing which is not belongs same domain of user?
bootstrap/service.go
Outdated
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()) |
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.
@JeffMboya Why we are checking the user have edit access to Domain ?
cf44a0a
to
7fb4b09
Compare
repoCall2 := sdk.On("Channel", mock.Anything, mock.Anything).Return(toChannel(tc.config.Channels[0]), errors.NewSDKError(tc.err)) | ||
repoCall3 := boot.On("ListExisting", context.Background(), mock.Anything, mock.Anything).Return(tc.config.Channels, tc.err) | ||
repoCall4 := boot.On("Save", context.Background(), mock.Anything, mock.Anything).Return(mock.Anything, tc.err) | ||
authCall := auth.On("Identify", mock.Anything, &magistrala.IdentityReq{Token: tc.token}).Return(&magistrala.IdentityRes{Id: validID, DomainId: domainID}, tc.identifyErr) |
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.
validID
and domainID
are fixed and for all requests identifyErr
is nil?
repoCall3 := boot.On("ListExisting", context.Background(), mock.Anything, mock.Anything).Return(tc.config.Channels, tc.err) | ||
repoCall4 := boot.On("Save", context.Background(), mock.Anything, mock.Anything).Return(mock.Anything, tc.err) | ||
authCall := auth.On("Identify", mock.Anything, &magistrala.IdentityReq{Token: tc.token}).Return(&magistrala.IdentityRes{Id: validID, DomainId: domainID}, tc.identifyErr) | ||
authCall1 := auth.On("Authorize", context.Background(), mock.Anything).Return(tc.authResponse, tc.authorizeErr) |
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.
All the times authorizeErr
is nil
repoCall4 := boot.On("Save", context.Background(), mock.Anything, mock.Anything).Return(mock.Anything, tc.err) | ||
authCall := auth.On("Identify", mock.Anything, &magistrala.IdentityReq{Token: tc.token}).Return(&magistrala.IdentityRes{Id: validID, DomainId: domainID}, tc.identifyErr) | ||
authCall1 := auth.On("Authorize", context.Background(), mock.Anything).Return(tc.authResponse, tc.authorizeErr) | ||
sdkCall := sdk.On("Thing", tc.config.ThingID, tc.token).Return(mgsdk.Thing{ID: tc.config.ThingID, Credentials: mgsdk.Credentials{Secret: tc.config.ThingKey}}, errors.NewSDKError(tc.authorizeErr)) |
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.
The error here shouldn't be authorizeErr
authCall1 := auth.On("Authorize", context.Background(), mock.Anything).Return(tc.authResponse, tc.authorizeErr) | ||
sdkCall := sdk.On("Thing", tc.config.ThingID, tc.token).Return(mgsdk.Thing{ID: tc.config.ThingID, Credentials: mgsdk.Credentials{Secret: tc.config.ThingKey}}, errors.NewSDKError(tc.authorizeErr)) | ||
sdkCall1 := sdk.On("Channel", channel.ID, tc.token).Return(toChannel(tc.config.Channels[0]), errors.NewSDKError(tc.channelErr)) | ||
svcCall := boot.On("ListExisting", context.Background(), domainID, mock.Anything).Return(tc.config.Channels, tc.listErr) |
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.
This is a repoCall
sdkCall := sdk.On("Thing", tc.config.ThingID, tc.token).Return(mgsdk.Thing{ID: tc.config.ThingID, Credentials: mgsdk.Credentials{Secret: tc.config.ThingKey}}, errors.NewSDKError(tc.authorizeErr)) | ||
sdkCall1 := sdk.On("Channel", channel.ID, tc.token).Return(toChannel(tc.config.Channels[0]), errors.NewSDKError(tc.channelErr)) | ||
svcCall := boot.On("ListExisting", context.Background(), domainID, mock.Anything).Return(tc.config.Channels, tc.listErr) | ||
svcCall1 := boot.On("Save", context.Background(), mock.Anything, mock.Anything).Return(mock.Anything, tc.saveErr) |
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.
Same to this
sdkCall.Unset() | ||
sdkCall1.Unset() | ||
svcCall.Unset() | ||
svcCall1.Unset() | ||
} | ||
} | ||
|
||
func TestView(t *testing.T) { |
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.
What are we testing here, because we don't check the emitted event is what we expect?
sdkCall1 := sdk.On("Channel", ch.ID, validToken).Return(toChannel(c.Channels[0]), nil) | ||
svcCall := boot.On("ListExisting", context.Background(), domainID, mock.Anything).Return(c.Channels, nil) | ||
svcCall1 := boot.On("Save", context.Background(), mock.Anything, mock.Anything).Return(mock.Anything, nil) | ||
|
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.
There is no need to simulate Add
operation because we are not persisting the value. This applies to everywhere
identifyErr error | ||
updateErr error | ||
err error | ||
event map[string]interface{} |
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.
Add the following cases this applies to other places too
- update with failed to identify a user
- update with failed to authorize user
- update with failed to fetch thing
- update with failed to fetch channel
- update with failed to fetch config
- update with failed to list existing
- update with failed to update connections
repoCall1.Unset() | ||
authCall.Unset() | ||
authCall1.Unset() | ||
svcCall.Unset() | ||
} | ||
} | ||
|
||
func TestList(t *testing.T) { |
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.
What are we testing here since we are not checking the emitted event
676d006
to
36be3ba
Compare
func (cr configRepository) rollback(tx *sqlx.Tx) { | ||
if err := tx.Rollback(); err != nil { | ||
cr.log.Error(fmt.Sprintf("Failed to rollback due to %s", err)) | ||
} | ||
} |
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.
Wrap and return the error , Don't login the error here.
|
||
cr.rollback("Failed to insert a Config", tx) | ||
cr.rollback(tx) | ||
return "", errors.Wrap(repoerr.ErrCreateEntity, e) | ||
} | ||
|
||
if err := insertChannels(ctx, cfg.Owner, cfg.Channels, tx); err != nil { | ||
cr.rollback("Failed to insert Channels", tx) | ||
if err := insertChannels(cfg.DomainID, cfg.Channels, tx); err != nil { | ||
cr.rollback(tx) | ||
return "", errors.Wrap(errSaveChannels, err) | ||
} | ||
|
||
if err := insertConnections(ctx, cfg, chsConnIDs, tx); err != nil { | ||
cr.rollback("Failed to insert connections", tx) | ||
cr.rollback(tx) | ||
return "", errors.Wrap(errSaveConnections, err) | ||
} | ||
|
||
if err := tx.Commit(); err != nil { | ||
cr.rollback("Failed to commit Config save", tx) | ||
cr.rollback(tx) |
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.
Please call cr.rollback
inside defer
instead of mutiple cr.rollback
} | ||
|
||
func (cr configRepository) rollback(content string, tx *sqlx.Tx) { | ||
func (cr configRepository) rollback(tx *sqlx.Tx) { | ||
if err := tx.Rollback(); err != nil { |
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.
Please provide content
string, This helps to identity at which step error has occured during debug.
if err := tx.Rollback(); err != nil { | |
func (cr configRepository) rollback(content string, tx *sqlx.Tx) { | |
if err := tx.Rollback(); err != nil { |
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>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
… and Remove methods Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
… and Remove methods Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
…meter Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
…meter Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
…meter Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
…meter Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
…meter Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
…rameter Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
…rameter Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
…rameter Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
…rameter Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
…rameter Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
…function Signed-off-by: JeffMboya <jangina.mboya@gmail.com>
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?
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.