Fixes #18738 - fixes the sub query

[ares]

I think this has been introduced by this change 142ccd5#diff-56ae1a984a0feed9e676a8e2f8240113R90

I'm not sure but I think it's similarly broken for all katello resources since this is generic method in api_controller. The problem I believe is in the fact the query needs to select only id attribute but it can contain other conditions, like name = Library. Doing the pluck introduces one more SQL query but make it safer since it does the (SELECT *) and returns id only, therefore it supports all conditions specified in filters.

I'm not familiar enough with katello testing infrastructure, so pointing me to some place where I could add a test for this and tips how to stub data would be appreciated.

[mention-bot]

@ares, thanks for your PR! By analyzing the history of the files in this pull request, we identified @johnpmitsch, @jlsherrill and @komidore64 to be potential reviewers.

[houndci-bot]

unexpected token tIVAR

[houndci-bot]

unexpected token tRPAREN

[ares]

Ok, I've got tests green. The tests does not test it on Activation key but still has some value. The reason why I don't test it on AK is that there's no factory defined for it and I don't feel up to experiment with what it needs. If someone provides the factory to easily create AK, I'm happy to add one more tests. But I hope that this does not block the PR. I'd appreciate if someone could review and test. I don't have write permissions on this repo so whoever ACKs please merge too :-)

EDIT: oh too soon, the test seems to fail again

[ares]

[test]

[houndci-bot]

unexpected token tRPAREN

[jlsherrill]

@ares this change makes sense, looks like a test is still failing.

[ares]

Yeah, honestly I don't know why and I'd appreciate if someone else provides the test or I'll remove it completely. I spent too much time on this already and without deeper knowledge of katello fixtures and factories I'm not sure how to move further within reasonable time.

[ares]

Tests should be green now, I removed my test that was failing.

[ares]

What's the status here?

[ehelms]

@jlsherrill please re-review

[jlsherrill]

Status was me finding time :)

[jlsherrill]

@ares doesn't look like the current test actually tests the problem. I had been meaning to help you write a test but hadn't found the time, but here you go:

diff --git a/test/controllers/api/v2/api_controller_test.rb b/test/controllers/api/v2/api_controller_test.rb
index 68c708b..63c8c5c 100644
--- a/test/controllers/api/v2/api_controller_test.rb
+++ b/test/controllers/api/v2/api_controller_test.rb
@@ -4,6 +4,8 @@ require "katello_test_helper"
 
 module Katello
   class Api::V2::ApiControllerTest < ActionController::TestCase
+    include Katello::AuthorizationSupportMethods
+
     def setup
       @controller = Katello::Api::V2::ApiController.new
       @query = Erratum.all
@@ -62,6 +64,21 @@ module Katello
       assert_nil response[:error], "error"
     end
 
+    def test_with_reduced_perms
+      params = {}
+      @controller.stubs(:params).returns(params)
+      @default_sort = %w(name desc)
+
+      User.current = users(:restricted)
+      key = katello_activation_keys(:simple_key)
+      setup_current_user_with_permissions(:name => "view_activation_keys",
+                                          :search => "environment = #{key.environment}")
+
+      @options = { :resource_class => Katello::ActivationKey }
+      keys = @controller.scoped_search(ActivationKey.readable, @default_sort[0], @default_sort[1], @options)
+      assert_includes keys[:results], key
+    end
+
     def test_scoped_search_zero_total
       @query = Erratum.where('1=0')
       params = {}

mind updating to add this test instead?

[ares]

Thanks a lot @jlsherrill, patch applied, let's see the test results now.

[jlsherrill]

ACK thanks @ares!