Enforcement

[atz]

Changes appear larger than they actually are, but include:

• Correct comment about solr_access_filters_logic params
• Un-protect discovery_permissions getter/setter
• Use attribute and alias declarations where possible
• Lots more YARD doc for all
• Delete #except and #append tests, these were testing SearchBuilder, not Enforcement. Apparently the methods existed in Enforcement once, but were deleted. The tests remained and still passed, so that gives you an idea of how useful they were.
• Stop testing SearchBuilder, start testing a class including the module. That class represents our downstream consumers, not SearchBuilder.
• Additional tests for a couple basic operations and exclusion of empty clauses

[jcoyne]

doesn't thin now apply no permissions, which gives access to everything?

[atz]

Seeing as how Ability has default groups and even anonymous public users have a group, I might also accept throwing an error here. But if you do intentionally pass "no group filters", what do you expect? Multiple :fq's against nothing or no group filters?

If you don't have groups then you would be creating filter clauses of, e.g. ({!terms f=my_field_ssim}). That matches nothing in a lucene query, because lucene doesn't index nothing. But more broadly, the end result is an OR query. After a single populated query, including additional no-match clauses is worthless. Without groups, the compound fq is going to say, e.g.:
'discover_access_person_ssim:user_1@abc.com' OR 'read_access_person_ssim:user_1@abc.com'

So that cannot match more than adding a bunch of other OR clauses.

Concrete queries:
{!terms%20f=format_main_ssim} -- field exists, query nothing, get nothing
{!terms%20f=format_main_ssim}Book -- field exists, query value, gets results
[{!terms f=no_such_field}](https://sul-solr-a.stanford.edu:443/solr/exhibits_stage/select?fq={!terms f=no_such_field}&indent=on&wt=json) -- field non-existent, query nothing, get nothing

[atz]

@jcoyne Do you consider this incorrect? If so, then I would consider the parallel case to apply_group_permissions in apply_user_permissions also incorrect:

return [] unless user && user.user_key.present?

While I cleaned up the form here, that behavior was pre-existing. No user_key, no filters. That was the scenario that actually had tests backing it, so I regarded it as more intentional:

blacklight-access_controls/spec/unit/enforcement_spec.rb

Lines 134 to 147 in eeac826

	describe "apply_user_permissions" do
	describe "when the user is a guest user (user key nil)" do
	it "does not create filters" do
	expect(subject.send(:apply_user_permissions, ["discover","read"])).to eq []
	end
	end
	describe "when the user is a guest user (user key empty string)" do
	let(:user) { User.new(email: '') }
	it "does not create filters" do
	expect(subject.send(:apply_user_permissions, ["discover","read"])).to eq []
	end
	end
	end

Happy to adapt if there is something more formal to work from, but this is my best understanding of the intended behavior.

[jcoyne]

I don't know if it is or isn't. I'm just checking to see if you've considered everything. I'm not eager to make changes to something that we believe works okay and is fundamental to access enforcement.

[atz]

The belief in this case is not validated by a test case or piece of documentation. Also, pragmatically, since even anonymous users should have a group, in theory this case is excluded in well behaved systems. I don't know what else to say. If you don't know what is intended, there's only one other person on this file's history to ask. @val99erie, do you have any insight here?

[carolyncole]

@atz have you looked at the sorl query created before and after your change? Can you give an example? We may be able to eyeball a test. aka if the output is invalid or does not make any sense your code would be the better option.

[atz]

As I mentioned, all production systems that I know about have a default user group (e.g. "public") and therefore will never experience this edge case. The tests here and in the overall suite still connect to solr with real queries and complete fine. If you want to strengthen the test suite for valid scenarios that you think are more likely to produce invalid queries, I would be happy to code to satisfy them. But I don't need to speculate about query validity here.

[atz]

@jcoyne Corrected the typos and added replies to your comments.

[atz]

@jcoyne updated that comment

[carolyncole]

@atz If the new standard is to set the discovery_permissions via discovery_permissions= The comment might be better to say that "This method defaults to [discover, read] permissions unless they have been set by calling discovery_permissions=". I think hit the setter may not entirely make sense to everyone.

[atz]

This is below the level of PR review I regard as substantive. The comment is improved by the addition. It can be merged as is without detriment. If you think it could be better still, you are probably correct! But you are also welcome to submit further revisions, including against this branch. IMO, the value represented by this PR is pretty well separate from the difference between the improved comment and a potentially better one.