Fixes #9947 - restrict user taxonomies if none is set

[ares]

No description provided.

[ohadlevy]

this is the 3rd time this logic appears, should it be extracted to a method?

[ares]

I had same feeling, just didn't know where to put it. It's in Taxonomix, Organization and Location. Maybe it could be Taxonomy class method? (I just don't want to introduce new concern).

[ares]

would this make it any better?

def used_organization_ids
  enforce_default
  return [] unless which_organization && SETTINGS[:organizations_enabled]
  get_taxonomy_ids(which_organization, which_ancestry_method) { |o, method| o.send(method) + o.ancestor_ids }
end

def get_taxonomy_ids(taxonomy, method, &block)
  Array(taxonomy).map { |t| block.call(t, method) }.flatten.uniq
end

[ohadlevy]

👍 from my side, @domcleal mind having another look?

[ares]

[test]

[lzap]

Can you move this to Host::Base?

The code looks good to me. I will test against Discovery.

[ares]

I'd change this as a separate PR, we should also move belongs_to and maybe more methods.

[ares]

anything I could do to make this reviewed/merged?

[dLobatog]

@ares Thanks, good patch, it's almost ready I'd say, but I found what looks like a important problem:

This is blocking users from being able to see taxable stuff with no taxonomy assigned. Following on the Redmine issue, try with 3 hosts:

• 1: assigned to a taxonomy that the user can see
• 2: assigned to a taxonomy the user cannot see
• 3: without any taxonomy

The expected result (I think) should be that the use can see 1 and 3, but not 2. However with this patch the user will only see 1, and 3 is hidden for every user but admins.

Other comments:

• [] is preferred over Array by rubocop, it increases the # of rubocop warnings (& violations when we turn it on), nitpick
• I don't understand very well self.expand from the comment, maybe add more info/examples to the comment or a more self explanatory name could help.
• Given the amount of assert_includes, refute_includes, and creation of objects, I think a method to do all these xxxxx_includes calls in one line would be much clearer. Something like assert_includes_ids taxable_ids, [1,2,4,6], refute_includes_ids taxable_ids, [3,5], I'm not sure of the syntax.

[ares]

@elobato so as disccussed, the first issue you mention is subject of following discussions and IMHO separate issue

other comments

• do you want me to do [org].flatten rather then Array(org)? I don't think I'd want to have such cop enabled in future
• I updated the comment, hopefully it's more clear now
• I don't want to introduce any new assertion helpers that readers would have to understand, also how we'd test it :-) I added some each cycles to make it a bit more readable, hope it's better now

[domcleal]

This is blocking users from being able to see taxable stuff with no taxonomy assigned. Following on the Redmine issue, try with 3 hosts:

1: assigned to a taxonomy that the user can see
2: assigned to a taxonomy the user cannot see
3: without any taxonomy

The expected result (I think) should be that the use can see 1 and 3, but not 2. However with this patch the user will only see 1, and 3 is hidden for every user but admins.

Not a big deal really, that's a misconfiguration if there are hosts present without multiorg attrs.

[dLobatog]

@domcleal It was probably a bad example, it applies for any object with taxonomies, not only hosts. I just happen to have misconfigured hosts without taxonomies myself.

[ares]

the way it works now if you select some org as a user, you also don't see global resources so I'd say if we want to have global resources to be visible (in both any org context and in specific org context) it's a separate issue

[dLobatog]

@ares I think we all agree we don't want to have global resources to be visible on specific contexts, however on Any context I think they should be visible.

[ares]

@elobato, so I might be the only one that disagree, I'd say that if they are global and we display them in any context, then it means they are also in org A and we should display then in there. I'd say it's either in both or none contexts. Clearly there's no obvious expectation and we can't say if displaying in any context (behaviour until now) was feature or bug, so if there will be some decision, it should be discussed, documented and implemented as separate issue.

[ohadlevy]

I'm with @ares on this one, clearly admin in any context see everything, non admin users in any context should see only their orgs resources and thats it.

having undefined objects pop up, to me, sounds like an alarming security concern then anything else.
also, on the $subject, I don't think its a good idea to introduce a flag to toggle the behavior, since we are already changing how it potentially worked before with this patch, going all the way (e.g. as @ares suggested) would be an acceptable change (imho) in light of this cve change.

[dLobatog]

Merged as abe910f, thanks @ares & everyone else involved during the review.

Warning for the release notes: this PR will make global (unscoped by organization/location) objects invisible to all users except for admins. If you want to make global objects you should assign them to all taxonomies.

[lzap]

Ok I tested this today - it works as expected ;-)