Skip to content
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

Merged

Conversation

AllenBW
Copy link
Member

@AllenBW AllenBW commented Jan 25, 2018

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 ping api/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)

rbac

@AllenBW AllenBW requested a review from himdel as a code owner January 25, 2018 04:07
@AllenBW AllenBW added this to the Sprint 78 Ending Jan 29, 2018 milestone Jan 25, 2018
@himdel
Copy link
Contributor

himdel commented Jan 25, 2018

@AllenBW The return truthy part makes sense, but .. shouldn't we be checking both sui_services_view and service_view?

I mean, doesn't not checking sui_services_view break the expectation of "if I disable this feature, they won't see services in SUI even if the generic feature is enabled"?

Or is the plan to remove sui_services_view completely?

@AllenBW
Copy link
Member Author

AllenBW commented Jan 25, 2018

@martinpovolny A good question, as mentioned in the initial message sui_services_view has no bearings on the 200 or 403 status of an api/services, so if you gate on sui_services_view you get a 403, the reason why we're here (evm-Desktop has sui_services_view but not service_view)

I don't know how it should be designed, maybe sui_services_view should also gate the correct rendering of api/service calls? Maybe not 🤔

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 sui_services_view and is still able to access it from the lnav, its just, without service_view it doesn't do ya any real good.

@himdel
Copy link
Contributor

himdel commented Jan 25, 2018

Aah, so what you're saying is service_view is for accessing service data in general, while sui_services_view is specifically about the "Services" secion in SUI, which this isn't a part of?

Yeah.. ok, that makes sense :).

@himdel
Copy link
Contributor

himdel commented Jan 25, 2018

...Actually.. no, why should the user see their services in the dashboard if they can't access them?

@AllenBW
Copy link
Member Author

AllenBW commented Jan 25, 2018

@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

@himdel
Copy link
Contributor

himdel commented Jan 25, 2018

So.. you are saying that users without sui_services_view are still supposed to see the Services section in the dashboard? :)

@AllenBW
Copy link
Member Author

AllenBW commented Jan 25, 2018

Huh? ☝️ Don't follow.

@himdel
Copy link
Contributor

himdel commented Jan 25, 2018

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
@AllenBW AllenBW force-pushed the BZ/MASTER/#1533222-dashboard-state-resolve branch from 433c862 to 1fd17c0 Compare January 25, 2018 14:09
@miq-bot
Copy link
Member

miq-bot commented Jan 25, 2018

Checked commit AllenBW@1fd17c0 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🍪

@himdel
Copy link
Contributor

himdel commented Jan 25, 2018

@AllenBW Either this isn't the right fix, or the caching rbac problem is not fixed yet, rendering this ..insufficient :).

  1. Log in as a user with superadmin & desktop roles.
  2. If you log in when the role is desktop, it manages to log in fine.
  3. But if you log in with admin as the main role (or really any other) and then switch to desktop, this still logs you out.

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 RBAC.has didn't notice the role switch.

@himdel
Copy link
Contributor

himdel commented Jan 25, 2018

magic

(the first reload is me, the second isn't)

@AllenBW
Copy link
Member Author

AllenBW commented Jan 25, 2018

@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
Addressing the logout on group change will precipitate once the resolve of the state is eliminated, and the resources grabbed from inside the state controller.

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.

AllenBW added a commit to AllenBW/manageiq-ui-service that referenced this pull request Jan 25, 2018
In support of the longer term fix brough on by ManageIQ#1372
AllenBW added a commit to AllenBW/manageiq-ui-service that referenced this pull request Jan 26, 2018
In support of the longer term fix brough on by ManageIQ#1372
@himdel
Copy link
Contributor

himdel commented Jan 26, 2018

Aah, got it, sorry for the delay.. this does fix the login problem, merging :)
And looking forward to the group switching fix :)

Copy link
Contributor

@himdel himdel left a comment

Choose a reason for hiding this comment

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

👍

@himdel himdel merged commit d60f840 into ManageIQ:master Jan 26, 2018
AllenBW added a commit to AllenBW/manageiq-ui-service that referenced this pull request Jan 26, 2018
In support of the longer term fix brough on by ManageIQ#1372
@AllenBW AllenBW deleted the BZ/MASTER/#1533222-dashboard-state-resolve branch January 26, 2018 16:05
simaishi pushed a commit that referenced this pull request Jan 26, 2018
…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
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit aaaaa5d46db0fa2269712afa0171a5e3a89eb911
Author: Martin Hradil <[email protected]>
Date:   Fri Jan 26 16:31:30 2018 +0100

    Merge pull request #1372 from AllenBW/BZ/MASTER/#1533222-dashboard-state-resolve
    
    Refactors dashboard state resolve to return true on failed query
    (cherry picked from commit d60f84003472182d5645a99f4920277b8a890d8f)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1533222

AllenBW added a commit to AllenBW/manageiq-ui-service that referenced this pull request Jan 26, 2018
AllenBW added a commit to AllenBW/manageiq-ui-service that referenced this pull request Feb 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants