WIP:GDB-6908 Added method compactAuthorities in the security controller and tests verifying it. #787
Conversation
…nd tests verifying it.
sava-savov-ontotext
requested review from
desislava-hristova-ontotext and
svilenvelikov
|
SonarCloud Quality Gate failed.
|
| @@ -104,6 +104,37 @@ const parseAuthorities = function (authorities) { | |||
| }; | |||
| }; | |||
|
|
|||
| const compactAuthorities = function (authorities) { | |||
| const writeAllRepos = authorities.find(authority => authority === `${WRITE_REPO_PREFIX}*`); | |||
There was a problem hiding this comment.
writeAllRepos is not a good name, better name would be something like writeAllRole or writeAllAuthority.
| const writeAllRepos = authorities.find(authority => authority === `${WRITE_REPO_PREFIX}*`); | ||
| if (writeAllRepos) { | ||
| return _.remove(authorities, function (_authority) { | ||
| if (_authority.indexOf(READ_REPO_PREFIX) === -1 && _authority.indexOf(WRITE_REPO_PREFIX) === -1) { |
There was a problem hiding this comment.
Can't we have constants for these WRITE_REPO_PREFIX_* and READ_REPO_PREFIX_*? As far as I can see, these are just static strings and can be defined once and reused.
E.g. WRITE_ALL_REPO_WILDCARD or something.
| @@ -104,6 +104,37 @@ const parseAuthorities = function (authorities) { | |||
| }; | |||
| }; | |||
|
|
|||
| const compactAuthorities = function (authorities) { | |||
There was a problem hiding this comment.
It's hard to understand what this function does without reading the code and why. Can you drop some doc comment here. Other than that, I wonder, why this is not placed inside some service?
| const compactAuthorities = function (authorities) { | ||
| const writeAllRepos = authorities.find(authority => authority === `${WRITE_REPO_PREFIX}*`); | ||
| if (writeAllRepos) { | ||
| return _.remove(authorities, function (_authority) { |
There was a problem hiding this comment.
You know that the function _.remove from lodash actually mutates the array argument, aren't you? Is it intentional that you just use the return value of the function and not aware of the actual authorities array? Isn't it better to use Array.reduce or Array.filter for example instead?
There was a problem hiding this comment.
The idea is to modify the array, leaving only "WRITE_REPO_", "READ_REPO_" and all other authorities, that aren't related with read or write ones.
| $scope.createUserHttp(); | ||
| $httpBackend.expectPOST("rest/security/users/testov", {"password":"testova", | ||
| "appSettings":{"DEFAULT_SAMEAS":true,"DEFAULT_INFERENCE":true,"EXECUTE_COUNT":true, "IGNORE_SHARED_QUERIES":false,"DEFAULT_VIS_GRAPH_SCHEMA":true}, | ||
| "grantedAuthorities":["ROLE_USER","WRITE_REPO_myrepo","READ_REPO_myrepo","READ_REPO_*"]}) |
There was a problem hiding this comment.
Maybe I don't get the requirement correctly, but shouldn't you expect
"grantedAuthorities":["ROLE_USER","WRITE_REPO_myrepo","READ_REPO_*"]
There was a problem hiding this comment.
If there is "WRITE_REPO_myrepo", "READ_REPO_myrepo" authority is expected as well.
sava-savov-ontotext
changed the title
No description provided.