SO-4113: authorization improvements #880

cmark · 2021-08-17T15:05:33Z

This PR introduces new authorization options to allow/restrict access to server resources.
Access tokens describe the necessary scopes to give access to certain resources, either by:

directly denoting the resource ID in the scope
denoting the containing bundle ID in the scope (direct bundleId parent for now)
or by allowing access to all resources (*)

...multiple resources per permission (all or anyof matching)

...multiple resources per permission (all or anyof matching) Fix remaining compile errors and test failures.

Support security filter when searching for resource documents (CodeSystems, Bundles, etc.) which restricts the result set to the allowed resources. Users require either direct access by ID, or transitive access via permission to a container bundle ID otherwise they will receive HTTP 404 messages that the resource is not found (or empty lists if it was a search request).

apeteri

GetResourceRequest should also implement AccessControl, the same way as UpdateRequest does; I think I saw some sub-classes in the other repository where the implements keyword was removed entirely.

apeteri · 2021-08-17T15:46:06Z

...nal.snowowl.core/src/com/b2international/snowowl/core/request/BaseResourceSearchRequest.java

+ */
+ protected final void addSecurityFilter(ExpressionBuilder queryBuilder, User user) {
+ if (!user.isAdministrator() && !user.hasPermission(Permission.requireAll(Permission.OPERATION_BROWSE, Permission.ALL))) {
+ Set<String> permittedResourceIds = user.getPermissions().stream().flatMap(p -> p.getResources().stream()).collect(Collectors.toSet());


Does having any sort of permission on a resource imply BROWSE rights? With the currently used operations (edit, import, export, version, promote task, classify), it feels so.

Yes, if you have at least one permission with other than the browse scope that includes the browse scope as well. For example, you would not be able to edit a resource without reading it first if this wasn't true.

apeteri · 2021-08-17T15:49:48Z

...international.snowowl.core/src/com/b2international/snowowl/core/identity/BasePermission.java

+
+ @Override
+ public final String getPermission() {
+ if (permission == null) {


For thread safety reasons, this should be put into a memoizing Supplier instead.

apeteri · 2021-08-17T15:53:31Z

core/com.b2international.snowowl.core/src/com/b2international/snowowl/core/identity/User.java

@@ -50,7 +54,10 @@ public String getUsername() {

 @JsonIgnore
 public List<Permission> getPermissions() {
- return getRoles().stream().flatMap(role -> role.getPermissions().stream()).distinct().collect(Collectors.toList());
+ if (permissions == null) {


cmark · 2021-08-17T17:12:46Z

GetResourceRequest should also implement AccessControl, the same way as UpdateRequest does; I think I saw some sub-classes in the other repository where the implements keyword was removed entirely.

Unfortunately, we cannot apply the same pattern there because certain subclasses (like CodeSystemGetRequest) should NOT check for access control. Scenarios where the token only states that the user has access to a bundle (and implicitly all its contained resources) would fail early because the permissions only describe the bundle and getting the nested resource requires evaluation of the described permissions, so this becomes a post-check as opposed to a pre-check.
What I could do instead is to introduce a new subclass that implements the same logic as the UpgradeRequest, but with the browse scope, but certain get requests require other scopes like ClassificationGetRequest, which require a non-default scope, which would further increase the complexity of that hierarchy, so I have decided to ditch the idea.

cmark · 2021-08-17T17:29:40Z

@apeteri fixed (or answered) all requested changes (comments).

codecov-commenter · 2021-08-18T07:58:51Z

Codecov Report

Merging #880 (8a8f89d) into 8.x (5900031) will increase coverage by 0.14%.
The diff coverage is 50.00%.

@@             Coverage Diff              @@
##                8.x     #880      +/-   ##
============================================
+ Coverage     62.46%   62.61%   +0.14%     
- Complexity    11465    11520      +55     
============================================
  Files          1709     1712       +3     
  Lines         55001    55072      +71     
  Branches       5267     5274       +7     
============================================
+ Hits          34359    34485     +126     
+ Misses        18467    18401      -66     
- Partials       2175     2186      +11

Impacted Files	Coverage Δ
...onal/snowowl/core/authorization/AccessControl.java	`0.00% <0.00%> (-100.00%)`	⬇️
...owowl/core/branch/AbstractBranchChangeRequest.java	`88.75% <ø> (ø)`
...ional/snowowl/core/branch/BranchCreateRequest.java	`92.85% <ø> (ø)`
...ional/snowowl/core/branch/BranchDeleteRequest.java	`100.00% <ø> (ø)`
...national/snowowl/core/branch/BranchGetRequest.java	`100.00% <ø> (ø)`
...ional/snowowl/core/branch/BranchReopenRequest.java	`0.00% <ø> (ø)`
...ional/snowowl/core/branch/BranchSearchRequest.java	`83.72% <ø> (ø)`
...ional/snowowl/core/branch/BranchUpdateRequest.java	`77.77% <ø> (ø)`
...tional/snowowl/core/branch/DeleteMergeRequest.java	`0.00% <ø> (ø)`
...rnational/snowowl/core/branch/GetMergeRequest.java	`75.00% <ø> (ø)`
... and 92 more

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 5900031...8a8f89d. Read the comment docs.

apeteri · 2021-08-18T08:04:59Z

EclSerializerTest failed in a previous CI build (NullPointerException was thrown in a Promise instead of concepts being returned), but it does not seem to be related to authorization changes. The concept search request invoked has a User instance registered with permissions in its context.

cmark added 5 commits August 16, 2021 18:14

feat(auth): refactor permission model and authorization to support...

0af462b

...multiple resources per permission (all or anyof matching)

chore: fix references to changed/removed classes/constants

462b42b

feat(auth): refactor permission model and authorization to support...

a6a6a93

...multiple resources per permission (all or anyof matching) Fix remaining compile errors and test failures.

fix(auth): missing User object when evaluating access control

1049bb3

cmark added enhancement feature labels Aug 17, 2021

cmark requested a review from apeteri August 17, 2021 15:05

cmark self-assigned this Aug 17, 2021

feat(auth): support allOf/anyOf evaluation of permissions

84f2d5d

apeteri requested changes Aug 17, 2021

View reviewed changes

fix(auth): use memoizing suppliers to compute permissions

8a8f89d

apeteri approved these changes Aug 18, 2021

View reviewed changes

apeteri merged commit f548e3c into 8.x Aug 18, 2021

apeteri deleted the feature/SO-4113-authorization-improvements branch August 18, 2021 08:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SO-4113: authorization improvements #880

SO-4113: authorization improvements #880

cmark commented Aug 17, 2021

apeteri left a comment

apeteri Aug 17, 2021

cmark Aug 17, 2021

apeteri Aug 17, 2021

apeteri Aug 17, 2021

cmark commented Aug 17, 2021 •

edited

Loading

cmark commented Aug 17, 2021

codecov-commenter commented Aug 18, 2021

apeteri commented Aug 18, 2021

SO-4113: authorization improvements #880

SO-4113: authorization improvements #880

Conversation

cmark commented Aug 17, 2021

apeteri left a comment

Choose a reason for hiding this comment

apeteri Aug 17, 2021

Choose a reason for hiding this comment

cmark Aug 17, 2021

Choose a reason for hiding this comment

apeteri Aug 17, 2021

Choose a reason for hiding this comment

apeteri Aug 17, 2021

Choose a reason for hiding this comment

cmark commented Aug 17, 2021 • edited Loading

cmark commented Aug 17, 2021

codecov-commenter commented Aug 18, 2021

Codecov Report

apeteri commented Aug 18, 2021

cmark commented Aug 17, 2021 •

edited

Loading