search

apache / accumulo-access

Apache Accumulo Access Control Library
https://accumulo.apache.org
Apache License 2.0
4 stars 11 forks source link

Fixes issue #66: adds a restriction info object that the UI can pull if it is present #67

Closed phrocker closed 6 months ago

phrocker commented 6 months ago

I didn't add additional null checks, which could be added as tests into AuthorizationsTests.

It felt less clean to ingest the singular test into AccessExpressionTest or AccessEvaluatorTest, but happy to get feedback. I'm new to this particular project so this is kind of a feeler.

keith-turner commented 6 months ago

I think there's more value in explicitly throwing an error if a user tries to supply a duplicate authorization, rather than silently remove duplicates.

What the behavior should be in this case is very subjective. My preference would be to dedupe, however I am not opposed to throwing an exception. One interesting thing is that the behavior is inconsistent between the two public static of() methods. Currently before this change one method will dedupe while the other throws an exception. It would be nice to make the two methods have consistent behavior, document the behavior for dupes, and test it. Whatever change is made, getting it consistent will be nice.

dlmarion commented 6 months ago

I think we can just change the API so that the user must provide us with a deduplicated set of authorizations. Then we don't have to worry about it or throw an exception. My suggestion on the issue - https://github.com/apache/accumulo-access/issues/66#issuecomment-1979782116

dlmarion commented 6 months ago

Created another possible solution in #68 that will either dedupe or throw an exception based on a new input parameter.

phrocker commented 6 months ago

These changes look good and re the test location, I think creating the new test class makes sense.

I don't understand the PR description.

Sorry about the description. Copy and pasta error. I'll close this out since @dlmarion submitting a good middle ground.