Add implied permissions - write permissions need read permissions to be useful #1411
Conversation
googlebot
added
the
cla: yes
mikehelmick
force-pushed
the
issue1408
branch
from
bd34b6d
to
cde9287
Compare
mikehelmick
changed the title
| @@ -63,9 +63,14 @@ func TestUpdate(t *testing.T) { | |||
|
|
|||
| for _, permission := range rbac.NamePermissionMap { | |||
| permission := permission | |||
| target := fmt.Sprintf(`input#permission-%d`, permission) | |||
| targets := []string{fmt.Sprintf(`input#permission-%d`, permission)} | |||
| // We also ned to remove permissions that imply this permission, or it will be added back in. | |||
There was a problem hiding this comment.
| // We also ned to remove permissions that imply this permission, or it will be added back in. | |
| // We also need to remove permissions that imply this permission, or it will be added back in. |
| impliedBy = make(map[Permission][]Permission) | ||
| ) | ||
|
|
||
| // Note: there are multiple init functions in this file. They are organized to be |
There was a problem hiding this comment.
This is very clever and require/implied will always be in-sync.
but would it be easier to read as:
requiredPermission = map[Permission][]Permission{ APIKeyRead: {APIKeyWrite}, SettingsRead: {SettingsWrite}, MobileAppRead: {MobileAppWrite}, UserRead: {UserWrite}, }
and perhaps keep init() in but instead just compare them and Panic if they don't match up. That would also allow for them to be asymmetrical eg.
APIKeyWrite: {APIKeyRead, SuperPowers},
and
APIKeyRead: {APIKeyWrite},
There was a problem hiding this comment.
I realize I'm advocating both duplication and panic, but multiple-init is also weird
There was a problem hiding this comment.
Multi-init is pretty common tbh
There was a problem hiding this comment.
This actually allows for the asymmetry - we just don't have it yet.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mikehelmick, sethvargo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes #1408
Proposed Changes
Release Note