-
Notifications
You must be signed in to change notification settings - Fork 154
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
don't add bind and dial permissions to services for admins #1782
Conversation
a0b6efb
to
0e4ef2b
Compare
} | ||
} | ||
// service permissions for admin & non-admin identities will be set according to policies | ||
service, err = result.manager.ReadForNonAdminIdentityInTx(tx, key, result.identityId) |
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.
I'm not sure if this is valid when result.identityId
== "" ?
@@ -143,10 +133,14 @@ func (self *EdgeServiceManager) ReadForIdentityInTx(tx *bbolt.Tx, id string, ide | |||
|
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.
Can we merge ReadForNonAdminIdentityInTx into ReadForIdentityInTx, as the 'NonAdmin' name is no longer accurate and the first method is now pretty short?
} | ||
} | ||
// service permissions for admin & non-admin identities will be set according to policies | ||
service, err = result.manager.ReadForIdentityInTx(tx, key, result.identityId, result.configTypes) |
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 will now happen when result.identityId
== "", and I'm not really sure what that means or if it's ok.
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.
I'm not sure it can happen, but we could short-circuit it in queryServices. If the identityId
is ""
we could return the service list result before we run the query.
Can you add a note to the changelog as well, please? |
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.
Changelog needs to be uypdated, since version numbers have changed, but otherwise, I think it looks good 👍
unable to rebase due to unsigned commits in release-next. recreating PR as #1919. |
fixes #1781