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

Fixes #29376 - Use Authorizable on Pools #8619

Merged
merged 1 commit into from Mar 20, 2020

Conversation

jturel
Copy link
Member

@jturel jturel commented Mar 19, 2020

This PR fixes a problem where users with the view_subscriptions permission could not view subscriptions via API (/api/v2/subscriptions). I haven't been able to come up with my own reproducer but I patched an affected system with this change and things work. I think the key is in joins_authorized.

I think this regressed in a recent change to the permissions in SubscriptionsController#show and I also cleaned up some of the messaging there to match what I think is now more appropriate.

Here's how the queries to load subscriptions has changed (improved)

Before:

irb(main):001:0> User.current.admin?
=> true
irb(main):002:0> Katello::Pool.readable.to_sql
=> "SELECT \"katello_pools\".* FROM \"katello_pools\" WHERE \"katello_pools\".\"subscription_id\" IN (SELECT \"katello_subscriptions\".\"id\" FROM \"katello_subscriptions\")"

irb(main):003:0> User.current = User.find(9) #nonadmin
=> "SELECT \"katello_pools\".* FROM \"katello_pools\" WHERE \"katello_pools\".\"subscription_id\" IN (SELECT \"katello_subscriptions\".\"id\" FROM \"katello_subscriptions\" WHERE \"katello_subscriptions\".\"organization_id\" = 14)"

After:

irb(main):010:0> User.current.admin?
=> true
irb(main):011:0> Katello::Pool.readable.to_sql
=> "SELECT \"katello_pools\".* FROM \"katello_pools\" INNER JOIN \"katello_subscriptions\" ON \"katello_subscriptions\".\"id\" = \"katello_pools\".\"subscription_id\" WHERE (1=1)"


irb(main):012:0> User.current = User.find(9) #nonadmin
irb(main):013:0> Katello::Pool.readable.to_sql
=> "SELECT \"katello_pools\".* FROM \"katello_pools\" INNER JOIN \"katello_subscriptions\" ON \"katello_subscriptions\".\"id\" = \"katello_pools\".\"subscription_id\" WHERE \"katello_pools\".\"organization_id\" = 14"

@theforeman-bot
Copy link

Issues: #29376

@sjha4
Copy link
Member

sjha4 commented Mar 20, 2020

[test katello]

@sjha4
Copy link
Member

sjha4 commented Mar 20, 2020

This is what I see during testing. https://gist.github.com/sjha4/3fb56a99804b2570f2378b855a88bb4a

The only change I noticed is that before the change for a subscription in org1, both of the below would work for admin user:
GET /katello/api/v2/organizations/:org1_id/subscriptions/:sub_id
GET /katello/api/v2/organizations/:org2_id/subscriptions/:sub_id

now,
GET /katello/api/v2/organizations/:org2_id/subscriptions/:sub_id does not work and it looks logically correct to me.

Although the effect here is that on the UI for admin user, while earlier he could see a subscription irrespective of selected org, now he cannot, in a sense reverting #8520 partly on the UI.

@jturel: This looks correct and incorrect to me at the same time .. 😄 The behavior otherwise is same as before. I will let you take the call on the behavior above.

Copy link
Member

@sjha4 sjha4 left a comment

Choose a reason for hiding this comment

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

Based on offline discussions, I have come to a strong conclusion that this should be the intended behavior. 😄

Viewing a record in a different organization than the selected organization should not be allowed. I will try to make it consistent for other katello pages in my open PR.

@jturel jturel merged commit de6fd72 into Katello:master Mar 20, 2020
@jturel jturel deleted the subscription_permissions branch March 20, 2020 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants