NIFI-2453 Making FileAuthorizer perform initial seeding when users an…

[bbende]

…d groups are already present

[YolandaMDavis]

@bbende I'll take this on

[YolandaMDavis]

@bbende I tested this with a standalone configuration and setup a my cert's DN in the Initial Admin Identity field. Started up the server normally and was able to access the application. I then added another user in NiFi (using a DN from a different cert) just to have more than one to try. I later went back to the server, shutdown NiFi and removed the authorizations.xml file to ensure that file would be regenerated and left the users file in place.

On startup I saw that the authorizations.xml file was created yet empty (with only open and closed authorizations tags). This led to my previously authorized user (set as Initial Admin Identity) no longer able to access the application. Is this expected behavior? My thoughts were that if a user was in the initial admin identify then NiFi would at least restore that reference. Stopping and deleting the user's file restores the authorizations as it was on initial startup (with authorizations created with the initial admin identity).

On debug of the FileAuthorizer it appears as if the emptyAuthorizations variable (ln 260) is actually returning false. On inspection of the authorizationsHolder variable you can see the policy set is empty so I'm not sure why the isEmpty() call resolves to false. However it is preventing the if emptyAuthorizations conditional block to execute, which I think is the intended behavior for this case.

final AuthorizationsHolder authorizationsHolder = new AuthorizationsHolder(authorizations, tenants);
final boolean emptyAuthorizations = authorizationsHolder.getAllPolicies().isEmpty(); //this returns false

I've included a snapshot of this behavior in debug mode. Ironically when using IntelliJ's evaluate expression feature it resolves to true however at runtime it resolves to false #nobueno.

As a side note, while debugging I saw an interesting area of code which feels like it may cause side effects. This is in the Policies.java:

    public List<Policy> getPolicy() {
        if (policy == null) {
            policy = new ArrayList<Policy>();
        }
        return this.policy;
    }

And this line of code is in AuthorizationsHolder.java (ln. 111 - 113)

      Set<AccessPolicy> allPolicies = new HashSet<>();
        if (policies == null || policies.getPolicy() == null) {
            return allPolicies;
        }

So in the case where I was testing the empty authorizations issue, when executing the AuthorizationsHolder.createAccessPolicies method the incoming policies object would not be null but the policy list member attribute it contained would be null until it hit the above conditional. When getPolicy is called it actually returns a list instead of null leading to the condition resolving as false. Since it's an empty list it doesn't evaluate the subsequent for-loop but it still appeared to me as strange behavior (which I figured had some reason behind it). Since Policies isn't extended anywhere to allow setting of that policy member, I suggest to remove the policies.getPolicy() == null from that condition since it doesn't appear that it would ever resolve to true. If the Policies getPolicy code could be refactored I think that would be more optimal.

[bbende]

@YolandaMDavis Thanks for testing this! The behavior you are seeing is interesting, I tested this a few times before submitting the PR and again right now and can't seem to reproduce that. Also had a unit test that simulated starting with only a users file and verified policies were created.

Could you use the debugger to try and step into authorizationsHolder.getAllPolicies().isEmpty() and see how isEmpty() is returning false?

It should be an unmodifiable HashSet which seems to call HashMap isEmpty() which does return size == 0;

[YolandaMDavis]

@bbende good news confirmed it was environmental. Saw that authorizations file was generated using Initial Admin Identity when deleted. Will run full test suite/contrib-check before prepping for merge.

+1