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 #18554 - skip collection_cache_lookup on authorized_for #4299

Merged
merged 1 commit into from Feb 21, 2017

Conversation

beav
Copy link
Contributor

@beav beav commented Feb 17, 2017

The call to authorized_for can take multiple seconds in some cases, leading
to performance issues.

This patch will bypass the cache lookup, resulting in similar times for admins
and non-admins when viewing pages with lots of hosts.

@mention-bot
Copy link

@beav, thanks for your PR! By analyzing the history of the files in this pull request, we identified @amirfefer, @ohadlevy and @dLobatog to be potential reviewers.

@theforeman-bot
Copy link
Member

There were the following issues with the commit message:

  • commit message for e1fa5db is not wrapped at 72nd column
  • commit message for e1fa5db is not wrapped at 72nd column

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

test "disable cache when calling can?" do
as_user @user do
Authorizer.any_instance.expects(:can?).with(@permission, @domain1, false)
result = authorized_for({:permission => @permission, :auth_object => @domain1})

Choose a reason for hiding this comment

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

Useless assignment to variable - result.

The call to `authorized_for` can take multiple seconds in some cases,
leading to performance issues.

This patch will bypass the cache lookup, resulting in similar times for
admins and non-admins when viewing pages with lots of hosts.
@dLobatog
Copy link
Member

Seems to have a significant error on REX jobs, merging..

Thanks @beav !

@ares
Copy link
Member

ares commented Feb 21, 2017

This patch does not make sense. The original problem was on one page that probably does not use authorizer object properly. After the cache is disabled, on every listing page (e.g. host index) we verify "view_hosts" permission on each table row for each subject separately. The cache normally loads all @hosts that are about to be displayed at once and do the verification in memory. I think this should be reverted and the fix should be in remote execution plugin where the authroized_for does not use properly setup authorizer. Thanks @iNecas for spotting this!

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