Skip to content

Backport: Removing hidden/reserved roles added via rolesmapping#595

Closed
debjanibnrj wants to merge 1 commit intoopensearch-project:opendistro-1.8from
debjanibnrj:opendistro-1.8
Closed

Backport: Removing hidden/reserved roles added via rolesmapping#595
debjanibnrj wants to merge 1 commit intoopensearch-project:opendistro-1.8from
debjanibnrj:opendistro-1.8

Conversation

@debjanibnrj
Copy link
Contributor

Earlier the InternalUsers API was filtering out all hidden/reserved roles. We will just restrict it to hidden/reserved roles associated via rolesmapping. This will ensure behaviour of rolesmapping and internalusers API is the same.

(cherry picked from commit 6c1f002)

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Earlier the InternalUsers API was filtering out all hidden/reserved roles. We will just restrict it to hidden/reserved roles associated via rolesmapping. This will ensure behaviour of rolesmapping and internalusers API is the same.

(cherry picked from commit 6c1f002)
@debjanibnrj debjanibnrj requested a review from a team as a code owner July 28, 2020 20:32
@debjanibnrj debjanibnrj changed the title Removing hidden/reserved roles added via rolesmapping Backport: Removing hidden/reserved roles added via rolesmapping Jul 28, 2020
@codecov
Copy link

codecov bot commented Jul 28, 2020

Codecov Report

Merging #595 into opendistro-1.8 will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                 Coverage Diff                  @@
##             opendistro-1.8     #595      +/-   ##
====================================================
+ Coverage             62.74%   62.78%   +0.03%     
- Complexity             2972     2974       +2     
====================================================
  Files                   228      228              
  Lines                 16370    16375       +5     
  Branches               2971     2972       +1     
====================================================
+ Hits                  10272    10281       +9     
+ Misses                 4550     4548       -2     
+ Partials               1548     1546       -2     
Impacted Files Coverage Δ Complexity Δ
...security/dlic/rest/api/InternalUsersApiAction.java 87.80% <100.00%> (+0.62%) 26.00 <0.00> (ø)
...ch/security/securityconf/DynamicConfigFactory.java 60.54% <100.00%> (+0.27%) 13.00 <0.00> (ø)
...earch/security/auditlog/impl/AbstractAuditLog.java 70.72% <0.00%> (+0.51%) 93.00% <0.00%> (+1.00%)
...ticsearch/security/auditlog/impl/AuditMessage.java 82.91% <0.00%> (+1.26%) 68.00% <0.00%> (+1.00%)

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 f0b002c...73a37c5. Read the comment docs.

Copy link
Member

@hardik-k-shah hardik-k-shah left a comment

Choose a reason for hiding this comment

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

We should not allow non-superadmin to update user or rolemapping if
(1) role is hidden
(2) if role mapping is hidden
(3) if role mapping is reserved

#2 and #3 are covered.

Any thoughts on #1 --> If role is hidden, that means superadmin doesn't want anyone to see that role and hence we should not allow anyone to update role-mapping or role-mapping field (for a hidden role)

@debjanibnrj
Copy link
Contributor Author

We should not allow non-superadmin to update user or rolemapping if
(1) role is hidden
(2) if role mapping is hidden
(3) if role mapping is reserved

#2 and #3 are covered.

Any thoughts on #1 --> If role is hidden, that means superadmin doesn't want anyone to see that role and hence we should not allow anyone to update role-mapping or role-mapping field (for a hidden role)

Added #1 as part of the following issue https://github.com/opendistro-for-elasticsearch/security/issues/590. I will need to update both the rolesmapping and internalusers API to accomodate this use case.

@debjanibnrj
Copy link
Contributor Author

I have addressed comments for this review as part of https://github.com/opendistro-for-elasticsearch/security/pull/614 (@hardik-k-shah). Will close this PR and address comments in #614 before backporting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants