-
Notifications
You must be signed in to change notification settings - Fork 91
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
Refactors dashboard state resolve to return true on failed query #1372
Refactors dashboard state resolve to return true on failed query #1372
Conversation
@AllenBW The return truthy part makes sense, but .. shouldn't we be checking both I mean, doesn't not checking Or is the plan to remove |
@martinpovolny A good question, as mentioned in the initial message I don't know how it should be designed, maybe So to really answer your question, the calls modified by this pr only render dashboard counts. So this does not break the expectation of "if I disable this feature, they won't see services in SUI even if the generic feature is enabled". As you can see from the video, the group has |
Aah, so what you're saying is Yeah.. ok, that makes sense :). |
...Actually.. no, why should the user see their services in the dashboard if they can't access them? |
@himdel Agreed, it doesn't make much sense, but it is the way api rbac is presently configured Most of the time sui_services_view and service_view are together, this is a rare instance where they are not. Maybe a second part of this fix is to add service_view to evm-Desktop? Not sure, it wasn't there originally 🤔 Fun fact the way both the sui and sui_limited roles are built, includes product features that prevent a 403 on api request for different collections |
So.. you are saying that users without |
Huh? ☝️ Don't follow. |
Just add both rbac checks and I'll merge it :). |
If anything other than a truthy value is returned, ui-router's resolve flips out and throws a state change error. SUI has state change errors written to then flipout and kick users outta da app. So when the rbac doesn't match, the query wont be made, the 403 won't ever prevent the state from resolving. Long term fix, this needs to be rewritten sans resolve
433c862
to
1fd17c0
Compare
Checked commit AllenBW@1fd17c0 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0 |
@AllenBW Either this isn't the right fix, or the caching rbac problem is not fixed yet, rendering this ..insufficient :).
Also, it looks like the role switching still doesn't reload menus properly, switching from desktop to superadmin causes request dashboards to update, but not the menus, and not the service dashboards - so this really sounds like |
@himdel confirmed the behavior... group switching logs you out, but at least now you're able to log back in 😏 While you have documented desired behavior, it is not the BZ, which is, cannot log in. This work fixes the inability to login. Edit As for the menu not updating on group change, its a result of that was addressed here #1363, what you're seeing is an artifact of the resolve blowing everything up. |
In support of the longer term fix brough on by ManageIQ#1372
In support of the longer term fix brough on by ManageIQ#1372
Aah, got it, sorry for the delay.. this does fix the login problem, merging :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
In support of the longer term fix brough on by ManageIQ#1372
…ate-resolve Refactors dashboard state resolve to return true on failed query (cherry picked from commit d60f840) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1533222
Gaprindashvili backport details:
|
In support of the longer term fix brought on by ManageIQ#1372
In support of the longer term fix brought on by ManageIQ#1372
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1533222
If anything other than a truthy value is returned, ui-router's resolve flips out and throws a state change error. SUI has state change errors written to then flipout and kick users outta da app.
So when the rbac doesn't match, the query wont be made, the 403 won't ever prevent the state from resolving.
Also, this functionality can't be gated on the
sui_services_view
product features, as this isn't the spice. IT ISN'T! Meaning, sure, you can have that product feature on your role, but end of the day if you pingapi/services
its gonna 403 you back to 🕳 🔥 😟 (which is no bueno as mentioned above)This state has gotta be rewritten to exclude
resolve
stat.🌮 💃 🍿 (ie 🎥 😋 )(ie proof its working as desired)