Fix Container* belongsto filter in Rbac::Filterer #18654
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `if` statement for `Rbac::Filterer#get_belongsto_matches` is getting a bit unwieldy and hard to digest. This simply breaks the logic out into it's own method, since a follow up commit will add to it, just adding to the complexity.
`ContainerGroup`, among other classes, were not being filter based on it's Entitlements. This not only includes `ContainerGroup` in the `Rbac::Filterer`'s `BELONGSTO_FILTER_CLASSES` constant, but also makes sure it is one of the classes that is filtered via associations, and not as itself and it's descendants. Applies the same treatment to all `Container*` classes.
NickLaMuro
force-pushed
the
fix-container-group-entitlement-filtering-in-rbac
branch
from
7f085c5 to
8fbebdb
Compare
Regarding concerns brought up for having class names in constants causing potential autoloading issues in certain scenarios (the appliance build for examples), this has been moved from a constant to a method to avoid that case.
|
Checked commits NickLaMuro/manageiq@f2a00f9~...45834ce with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
kbrock
reviewed
Apr 15, 2019
| end | ||
| end | ||
|
|
||
| def associated_belongsto_models |
There was a problem hiding this comment.
future reference: putting into a constant preloads all classes. not good for load time and development re-loading.
he was not a fan of safe_constantize - so we agreed that this was the best solution.
|
also note, the first commit (refactor) simplifies the second commit (the addition of the extra clases) |
|
@kbrock if this can be backported, can you add the label: hammer/yes |
|
@miq-bot add_label hammer/yes |
simaishi
pushed a commit
that referenced
this pull request
Apr 22, 2019
…ment-filtering-in-rbac Fix Container* belongsto filter in Rbac::Filterer (cherry picked from commit 171c281) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1702076
|
Hammer backport details: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ContainerGroup, among other classes, were not being filter based on it's Entitlements for a given user's role, as reported by this BZ:https://bugzilla.redhat.com/show_bug.cgi?id=1631694
This not only includes
ContainerGroupin theRbac::Filterer'sBELONGSTO_FILTER_CLASSESconstant, but also makes sure it is one of the classes that is filtered via associations, and not as itself and it's descendants (which is what happens inelseportion ofRbac::Filterer#get_belongsto_matches). Applies the same treatment to allContainer*classes, since they also suffered from the same problem, and specs were created for each class.There was a small bit of refactoring done in this PR, which was just to make the
ifclause more legible, but it is doing it in a way that doesn't completely retain the clause in it's original form (but is arguably more readable), so some eyes on that portion of things would be necessary. The refactoring effort can be put off if desired and done in a separate PR.(Possible) Outstanding issue
While this does fix the issue described in the ticket, the
MiqProductFeatures that are setup (and described as being present in the ticket) are not applied at all inRbac(if at all). I don't know if this is something that is filtered elsewhere, but currently, the following filters:Does not do anything to my knowledge to limit viewing permissions in
Rbac.That said, it might limit it in the view/UI, but that is not being tested here (even though it is being setup in the QA steps below)
Links
Steps for Testing/QA
The provided gist above has two scripts to run, so follow the
README.mdinstructions there.