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

Fix #8812 - Pass model type so search_for is called on Host #2121

Closed

Conversation

roidelapluie
Copy link
Member

This PR is a rebase of GH-2013

At least on version 1.6.1, the absence of this second parameter leads to a
runtime crash when it's time to validate if the current user (non-admin) is
allowed to perform a power operation on given a host via the APIv2.

The root cause of the crash is basically that search_for is called on
Host::Base by app/services/authorizer.rb:50.

Not sure this is necessary in newer versions, up to the developers to figure
it out :)

@theforeman-bot
Copy link
Member

There were the following issues with the commit message:

  • bbd9479 must be in the format Fixes/refs #redmine_number - brief description.

Guidelines are available on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@roidelapluie roidelapluie changed the title [PL2013 rebased] Fix #8812 - Pass model type so search_for is called on Host [GH-2013 rebased] Fix #8812 - Pass model type so search_for is called on Host Feb 2, 2015
@lzap
Copy link
Member

lzap commented Feb 3, 2015

My initial idea was - holy moly, someone is programming Foreman with PL 2313 ;-)

@lzap
Copy link
Member

lzap commented Feb 3, 2015

FYI it is not required to create new PR when rebasing. Feel free to force push into your existing branch, github handles this fine.

@roidelapluie
Copy link
Member Author

@lzap I am not the author of the first PR, so I can not force push to someone else's repository. But I am really interested in getting that patch merged for 1.8/1.7..4

@lzap
Copy link
Member

lzap commented Feb 3, 2015

@lzap I am not the author of the first PR, so I can not force push to someone else's repository. But I am really interrested in getting that patch merged for 1.8/1.7..4

Oh sorry I missed that.

Thanks for the contributions. I expect folks who were doing initial
review will carry on with this one.

Later,
Lukas #lzap Zapletal

@domcleal
Copy link
Contributor

domcleal commented Feb 3, 2015

[test]

@roidelapluie
Copy link
Member Author

@domcleal Can you retrigger the tests? Thanks

@domcleal
Copy link
Contributor

[test]

@dLobatog
Copy link
Member

@roidelapluie could you take at look at fixing the failing test? Thanks!

@dLobatog
Copy link
Member

[test] because of the latest enabled cops

@roidelapluie
Copy link
Member Author

Would be great to see this in 1.8.

@domcleal
Copy link
Contributor

[test]

@roidelapluie roidelapluie force-pushed the pull2013rebased branch 2 times, most recently from b8b95e3 to fb3170d Compare March 17, 2015 12:42
@roidelapluie roidelapluie changed the title [GH-2013 rebased] Fix #8812 - Pass model type so search_for is called on Host Fix #8812 - Pass model type so search_for is called on Host Mar 17, 2015
@roidelapluie
Copy link
Member Author

@elobato I have written a test but I can not get it to work locally

@roidelapluie
Copy link
Member Author

it does not work because in app/models/user.rb line 320 cached_roles.detect {|role| role.allowed_to?(action)}.present

cached_roles is empty. i have no idea why. when I connect to the test server I see that my fixtures are correct.

@dLobatog
Copy link
Member

dLobatog commented Apr 7, 2015

I tried and I'm not sure why cached_roles is not being populated. However we're trying to get rid of using fixtures everywhere (especially in this case when it's a user only used once in the test base).

I spent some time changing your code to use Factories and tests pass just fine 😄 ,

Here is the patch !

I think this is probably ready to merge after applying it. Apply it, let's see what Mr. Jenkins thinks and if it's a 🍏 let's :shipit: 😄

Thanks again @roidelapluie !

@dLobatog dLobatog closed this Apr 7, 2015
@dLobatog dLobatog reopened this Apr 7, 2015
@theforeman-bot
Copy link
Member

There were the following issues with the commit message:

  • 948ab7a must be in the format Fixes/refs #redmine_number - brief description.

Guidelines are available on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@roidelapluie
Copy link
Member Author

Merged your patch :)

At least on version 1.6.1, the absence of this second parameter leads to a
runtime crash when it's time to validate if the current user (non-admin) is
allowed to perform a power operation on given a host via the APIv2.

The root cause of the crash is basically that search_for is called on
Host::Base by app/services/authorizer.rb:50.
@roidelapluie
Copy link
Member Author

@elobato :green:

@dLobatog
Copy link
Member

dLobatog commented Apr 7, 2015

Merged as e02a2ff, thanks to @roidelapluie and special thanks to @nbarrientos for the original patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants