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.

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

Fix setting a role with non-existing controller or action #1535

Open
wants to merge 53 commits into
base: 2-dev
from

Conversation

@Yoann-Abbes
Copy link
Contributor

Yoann-Abbes commented Nov 28, 2019

What does this PR do ?

This PR checks controller and actions of the role which is being set and throw an error if the controller or the action does not exist

Before this PR:
When you set a right on a non-existing controller or action, Kuzzle allow you to do that without any warning.

Now:
We throw an error when user try to set rights to non-existing controller or action of the Kuzzle API.

Concerning plugin API, it could be convenient to define roles on plugin controller that will be deployed in a near futur.
To do that we have now a force option that allows users to write invalid plugin role.

In the case force is set to true we notify the user that his role is created but there might be some error.
In the case force is not set or set to false we throw a proper error.

Also, a sanity check is added at kuzzle startup to notify every invalid existing plugin role.

Jira ticket

How should this be manually tested?

  • Step 1 :
  • Step 2 :
  • Step 3 :
    ...

Other changes

searchRole returned an array randomly sorted
Now it returns a sorted array by _id

Boyscout

Yoann-Abbes added 2 commits Nov 28, 2019
Copy link
Contributor

Aschen left a comment

Missing functional tests and also security code page generation

});
}
else {
if (!this.kuzzle.funnel.isNativeController(

This comment has been minimized.

Copy link
@Aschen

Aschen Nov 29, 2019

Contributor

I think this method signature should be modified to take only the controller name as an argument.
Also, if the role define rights for a plugin controller, this method will throw (eg: s3/upload ). You should not check anything if the controller isn't a native one

});
}
});
return Promise.resolve();

This comment has been minimized.

Copy link
@Aschen

Aschen Nov 29, 2019

Contributor

This methods does not need to returns a promise

* @param {Role} role
* @returns Promise
*/
checkRoleControllersAndActions(role) {

This comment has been minimized.

Copy link
@Aschen

Aschen Nov 29, 2019

Contributor

Nit/proposal: could be renamed because it should checks only for native action existence

return deleteResponse;
});
.then(
deleteResponse => {

This comment has been minimized.

Copy link
@Aschen

Aschen Nov 29, 2019

Contributor

Why did you change the indent here?

@Yoann-Abbes Yoann-Abbes added the wip label Dec 4, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 4, 2019

Codecov Report

Merging #1535 into 2-dev will decrease coverage by 0.05%.
The diff coverage is 88.7%.

Impacted file tree graph

@@            Coverage Diff             @@
##            2-dev    #1535      +/-   ##
==========================================
- Coverage   93.55%   93.49%   -0.06%     
==========================================
  Files         101      101              
  Lines        6855     6891      +36     
==========================================
+ Hits         6413     6443      +30     
- Misses        442      448       +6
Impacted Files Coverage Δ
lib/core/janitor.js 82.16% <ø> (ø) ⬆️
lib/core/storage/indexStorage.js 100% <ø> (ø) ⬆️
lib/config/httpRoutes.js 100% <ø> (ø) ⬆️
lib/core/plugins/context.js 89.6% <0%> (ø) ⬆️
lib/core/storage/clientAdapter.js 95.55% <100%> (ø) ⬆️
lib/api/controllers/server.js 91.89% <100%> (+0.58%) ⬆️
lib/api/controllers/collection.js 88.57% <50%> (-4.22%) ⬇️
lib/services/elasticsearch.js 91.82% <96%> (+0.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa953e1...0d93d6a. Read the comment docs.

Yoann-Abbes added 6 commits Dec 4, 2019
doc
})
.then(() => this.checkRoleNativeRights(role))
.then(err => {
err = this.checkRolePluginsRights(role);

This comment has been minimized.

Copy link
@kuzzle

kuzzle Dec 6, 2019

Contributor

MINOR Introduce a new variable instead of reusing the parameter "err". rule

// Meaning this is not a plugin controller. So we skip it.
return;
}
const pluginName = roleController.split('/')[0];

This comment has been minimized.

Copy link
@kuzzle

kuzzle Dec 6, 2019

Contributor

MINOR Use destructuring syntax for these assignments from "roleController.split('/')". rule

@kuzzle

This comment has been minimized.

Copy link
Contributor

kuzzle commented Dec 6, 2019

SonarQube analysis reported 2 issues

  • MINOR 2 minor

Watch the comments in this conversation to review them.

@Yoann-Abbes

This comment has been minimized.

Copy link
Contributor Author

Yoann-Abbes commented Dec 12, 2019

Functional tests are OK with this branch of the SDK JS : kuzzleio/sdk-javascript#476

@Yoann-Abbes Yoann-Abbes requested a review from Aschen Dec 12, 2019
@Yoann-Abbes Yoann-Abbes removed the wip label Dec 12, 2019
features-sdk/SecurityController.feature Outdated Show resolved Hide resolved
features-sdk/step_definitions/security-steps.js Outdated Show resolved Hide resolved
features-sdk/step_definitions/security-steps.js Outdated Show resolved Hide resolved
features-sdk/step_definitions/security-steps.js Outdated Show resolved Hide resolved
features-sdk/step_definitions/security-steps.js Outdated Show resolved Hide resolved
lib/core/models/repositories/roleRepository.js Outdated Show resolved Hide resolved
lib/core/models/repositories/roleRepository.js Outdated Show resolved Hide resolved
lib/core/models/repositories/roleRepository.js Outdated Show resolved Hide resolved
lib/core/models/repositories/roleRepository.js Outdated Show resolved Hide resolved
lib/kuzzle.js Outdated Show resolved Hide resolved
…ights-on-role
doc/2/api/controllers/security/create-role/index.md Outdated Show resolved Hide resolved
doc/2/api/controllers/security/update-role/index.md Outdated Show resolved Hide resolved
doc/2/api/essentials/errors/codes/index.md Outdated Show resolved Hide resolved
doc/2/api/essentials/errors/codes/index.md Outdated Show resolved Hide resolved
const roleRightsError = errorsManager.wrap('security', 'role');
Object.keys(role.controllers).forEach(roleController => {
if (roleController !== '*' && !this.kuzzle.funnel.controllers.has(roleController)) {
if (roleController.split('/').length > 1) {

This comment has been minimized.

Copy link
@scottinet

scottinet Dec 17, 2019

Member

Also I find this code brittle: it's not the responsibility of the role repository to know the difference between a native controller and a plugin one.

Instead, I would either use this.kuzzle.funnel.getController(<Request>), or create a new dedicated funnel getter named this.kuzzle.funnel.exists(controller, action). That new getter code would take its code after the existing getController function.

Either way, the funnel is the API entrypoint, and it alone should know whether a controller/action pair exists or not: keep a clean separation of concern

lib/core/models/repositories/roleRepository.js Outdated Show resolved Hide resolved
lib/core/models/repositories/roleRepository.js Outdated Show resolved Hide resolved
lib/core/models/repositories/roleRepository.js Outdated Show resolved Hide resolved
lib/core/models/repositories/roleRepository.js Outdated Show resolved Hide resolved
scottinet and others added 4 commits Dec 20, 2019
…io/kuzzle into KZL-1356-set-wrong-rights-on-role
…io/kuzzle into KZL-1356-set-wrong-rights-on-role
scottinet added a commit to kuzzleio/sdk-javascript that referenced this pull request Jan 7, 2020
<!--
  This template is optional.
  It simply serves to provide a guide to allow a better review of pull requests.
-->

<!--
  IMPORTANT
  Don't forget to add the corresponding "changelog:xxx" label to your PR.
  This is part of our release process in order to generate the change log.
-->


## What does this PR do?

This PR adds a new `force` option to routes who write a role.
It permits not to throw in case you want to set rights about a plugin which is not available yet, or controllers/action that are not described yet in the plugin.

⚠️  This PR goes along with kuzzleio/kuzzle#1535
As you can see, this option is only effective if it concerns plugin rights. 
⚠️ 
<!-- Please fulfill this section -->

<!--
  Please include a summary of the change and which issue is fixed.
  Please also include relevant motivation and context.
  List any dependencies that are required for this change.
-->

### How should this be manually tested?

⚠️ you should test it with Kuzzle instantiated on this branch kuzzleio/kuzzle#1535 ⚠️

- Try creating this role
```
const body = { 
controller: {
    unavailable_plugin/test: {
            actions: {
                  create: true,
                  update: true
               }
          }
     }
}

this.sdk.security.createRole('newRole', body) will throw.
this.sdk.security.createRole('newRole', body, {force: true}) will work
```

<!--
  Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.
  Please also list any relevant details for your test configuration
-->
  - Step 1 :
  - Step 2 :
  - Step 3 :  
  ...

### Other changes

<!--
  Please describe here all changes not directly linked to the main issue, but made because of it.
  For instance: issues spotted during this PR and fixed on-the-fly, dependencies update, and so on
-->

### Boyscout

<!--
  Describe here minor improvements in the code base and not directly linked to the main changes:
  typos fixes, better/new comments, small code simplification, new debug messages, and so on.
-->
Yoann-Abbes added 4 commits Jan 8, 2020
doc
@Yoann-Abbes Yoann-Abbes requested review from Aschen and scottinet Jan 9, 2020
Yoann-Abbes added 2 commits Jan 9, 2020

@security
Scenario: Create a role with invalid API rights
When I can not "create" a role "test-role" with the following API rights:

This comment has been minimized.

Copy link
@xbill82

xbill82 Jan 9, 2020

Member

Nitpicking: You are writing a When step that makes assertions about the result of its action.
When a steps performs an action that is supposed to trigger an error, I suggest that it states

When I try to... blah blah blah
Then I get an error matching "blah blah blah"

This comment has been minimized.

Copy link
@Yoann-Abbes

Yoann-Abbes Jan 9, 2020

Author Contributor

That is why just below there is

 Then I should receive an error matching:
    | id | "security.role.unknown_controller" |
lib/core/models/repositories/roleRepository.js Outdated Show resolved Hide resolved
lib/core/models/repositories/roleRepository.js Outdated Show resolved Hide resolved
checkRolePluginsRights(role, { force=false } = {}) {
Object.keys(role.controllers).forEach(roleController => {
if ( !(roleController !== '*'
&& !this.kuzzle.funnel.isNativeController(roleController))

This comment has been minimized.

Copy link
@xbill82

xbill82 Jan 9, 2020

Member

This doesn't feel right to me. You're checking if the controller exists in plugin, right? So it can't be a native one...

This comment has been minimized.

Copy link
@Yoann-Abbes

Yoann-Abbes Jan 9, 2020

Author Contributor

In fact, im checking if its a native one because if its not, then its a plugin one.

This comment has been minimized.

Copy link
@xbill82

xbill82 Jan 20, 2020

Member

Ok, sorry, didn't see the exclamation mark that negates the whole predicate. So this condition is OK!

Yoann-Abbes and others added 2 commits Jan 9, 2020
Co-Authored-By: Luca Marchesini <xbill82@gmail.com>
Co-Authored-By: Luca Marchesini <xbill82@gmail.com>
@Yoann-Abbes Yoann-Abbes requested a review from xbill82 Jan 9, 2020
@Aschen
Aschen approved these changes Jan 9, 2020
Yoann-Abbes added 5 commits Jan 10, 2020
…io/kuzzle into KZL-1356-set-wrong-rights-on-role
@Yoann-Abbes

This comment has been minimized.

Copy link
Contributor Author

Yoann-Abbes commented Jan 15, 2020

Reminder: All functional test are OK with the 7-dev branch of the SDK JS. (the force option is not released yet)

checkRolePluginsRights(role, { force=false } = {}) {
Object.keys(role.controllers).forEach(roleController => {
if ( !(roleController !== '*'
&& !this.kuzzle.funnel.isNativeController(roleController))

This comment has been minimized.

Copy link
@xbill82

xbill82 Jan 20, 2020

Member

Ok, sorry, didn't see the exclamation mark that negates the whole predicate. So this condition is OK!

Yoann-Abbes added 2 commits Jan 22, 2020
@sonarcloud

This comment has been minimized.

Copy link

sonarcloud bot commented Jan 22, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.1% 0.1% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.