Skip to content
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.

Sign up for GitHub

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

change capabilities logic for sudo prefixes #5647

Merged
merged 6 commits into from
Oct 31, 2018
Merged

change capabilities logic for sudo prefixes #5647

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
94b42bf
add test for revoking lease with update capabilities, change capabili…
Oct 30, 2018
b8e34fe
Merge branch 'master' into lease-capabilities
madalynrose Oct 30, 2018
8873339
add a couple more tests for capabilities function
Oct 30, 2018
f797aee
Merge branch 'lease-capabilities' of github.com:hashicorp/vault into …
Oct 30, 2018
be82e78
Merge branch 'master' into lease-capabilities
madalynrose Oct 30, 2018
3ae8d52
Merge branch 'master' into lease-capabilities
madalynrose Oct 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ui/app/models/capabilities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ const computedCapability = function(capability) {
return false;
}
// if the path is sudo protected, they'll need sudo + the appropriate capability
if (SUDO_PATHS.includes(path) || SUDO_PATH_PREFIXES.find(item => item.startsWith(path))) {
if (SUDO_PATHS.includes(path) || SUDO_PATH_PREFIXES.find(item => path.startsWith(item))) {
return capabilities.includes('sudo') && capabilities.includes(capability);
}
return capabilities.includes(capability);
Expand Down
14 changes: 14 additions & 0 deletions ui/tests/unit/models/capabilities-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,18 @@ module('Unit | Model | capabilities', function(hooks) {
assert.notOk(model.get('canDelete'));
assert.notOk(model.get('canList'));
});

test('it does not require sudo on sys/leases/revoke if update capability is present', function(assert) {
let model = run(() =>
this.owner.lookup('service:store').createRecord('capabilities', {
path: 'sys/leases/revoke',
capabilities: ['update', 'read'],
})
);
assert.ok(model.get('canRead'));
assert.notOk(model.get('canCreate'), 'sudo requires the capability to be set as well');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this doesn't have to do with sudo, just that create wasn't in the capabilities list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's in every test so I just left it

assert.ok(model.get('canUpdate'), 'should not require sudo if it has update');
assert.notOk(model.get('canDelete'));
assert.notOk(model.get('canList'));
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add another test for the paths in the SUDO_PREFIX arg too to make sure they are requiring sudo?

So for path: 'sys/leases/revoke-prefix/aws' with capabilities "update", canUpdate should be false, but with "update", "sudo", canUpdate should be true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a couple. Do they look right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yassss - perfect!

});