NIFI-1884 Defining API for Users, Groups, and Policies

[bbende]

This pull request introduces the concept of a MutableAuthorizer which is an interface that extends the recently introduced Authorizer. A MutableAuthorizer has the ability to manage users, groups, and policies. In addition this PR introduces the classes for user, group, and access policy.

[mcgilman]

Reviewing...

[mcgilman]

+1 LGTM

[jtstorck]

For getters that return entities/request actions, since the underlying set is unmodifiable, should we mention that in the javadoc? Another alternative would be to return a defensive copy of the internal set.

[bbende]

Good call, I prefer documenting in the javadoc

[alopresto]

I'm not sure I understand the relationship between User and Group -- which class owns the relationship, and is it bidirectional? What would be the steps for the following tasks:

• Create a group "Admins", create a user "Bryan Bende", add "Bryan Bende" to "Admins"
• Create a group "Admins", create a user "Andrew LoPresto", add "Andrew LoPresto" to "Admins", change name of "Andrew LoPresto" to "Andy LoPresto"
• Create a group "Admins", create a user "Bryan Bende", create a user "Andy LoPresto", add "Bryan Bende" to "Admins", add "Andy LoPresto" to "Admins", remove user "Bryan Bende" from "Admins"

It seems as if the User and Group objects both have references to each other, but they are String identifiers, and the Sets are immutable.

[bbende]

@alopresto The intent was for the relationship to be bi-directional, meaning you could add a user to a group by adding/updating a User with a new group id in the set of groups, or by adding/updating a Group with a new user id in the set of users.

The overall idea was to retrieve an object, make a copy of it with the desired changes, and then call update. So for the example scenarios...

• You could do this two different ways, but one would be to create a new Group instance with name "Admins" and no users and call addGroup(), then create a new User instance with name "Bryan Bende" and a set of group ids containing the id of the "Admins" group and call addUser()
• Same as above to create the group and user and place the user in the group... then you would retrieve the User instance for "Andrew LoPresto", make a copy of that User instance with the name set to "Andy LoPrestro" and call updateUser(...)
• Same as scenario 1 to create the group and users and place the users in the group, although to add both users to the group in one action you could retrieve the Group, make a copy of the Group and add both user ids to the set of users, and call updateGroup(). Removing "Bryan Bende" from "Admins" could be done by copying the Group without "Bryan Bende"s id in the set of users and calling updateGroup, or by copying the User for "Bryan Bende" without the id of the "Admin" group in the set of groups and calling updateUser().

After thinking about all of these, it seems like it would nice to have builders that made it convenient for updating a User or Group... So something like...

User user = authorizer.getUser(id);
User updatedUser = new User.UserBuilder().fromUser(user).name("New Name").build();
authorizer.updateUser(updatedUser);

Also easily removing entries, so something like this for removing a user from a group:

User user = authorizer.getUser(id);
User updatedUser = new User.UserBuilder().fromUser(user).removeGroup(groupId).build();
authorizer.updateUser(updatedUser);

Thoughts?

[alopresto]

@bbende thanks for the explanation. I suppose I am more familiar with, and conditioned to assume, the models used in many database-backed frameworks where the user and group models are canonical -- while there may be many User objects instantiated that describe "Andy LoPresto", they all refer to the same record in persistent storage, and modifying one (and saving it) updates the record that all point to. In addition, it would not be necessary to clone a retrieved record in order to modify it.

Example (using Rails/Grails-esque syntax, but easily replaceable with authorizer.getUser(id) ):

User andy = User.getByUsername("alopresto")
logger.info(andy.getFirstName()) // "Andrew"
andy.setFirstName("Andy")
andy.update()

In addition, while updating the relationship can be done from either end (i.e. adding a user to n groups is easier by modifying the one user instead of retrieving and modifying n groups, while removing all users from a specific group would use the opposite operation), it seems like the model proposed above duplicates a lot of data during each operation. Is there a reason for this? It seems likely there is a tradeoff I am missing.

I also have questions about resolving uniqueness constraints, ID mutability, locks, merge conflicts, etc. on updates, but it seems that these details are delegated to the authorizer implementation?

[mcgilman]

@alopresto Thanks for the very thorough review! This API is designed strictly for handling the persistence of access policies which would also include Users and Groups. These objects are designed as simple data objects or POJOs. While the MutableAuthorizer can load these records, it must be told when to save. It wouldn't know a given User/Group/Policy has been modified. This is part of the motivation to have the objects be immutable. The intent is very clear to an implementor of MutableAuthorizer that the objects won't be modified outside of their knowing. I believe that @bbende recommendation of introducing a Builder for creating new versions of a given User/Group/Policy is a great way to handle the cloning.

The end goal here was to design the API such that the implementation only needed to be concerned with persistence.

uniqueness constraint - How we address this would ultimately be based on whether we support reloading the Users/Groups/Policies while the application is running. We decided against this as the motivation for the MutableAuthorizer API was to support an Authorizer that was managed by NiFi. With this approach we would be enforcing uniqueness at startup and when new records were added outside of the MutableAuthorizer.

ID mutability - The identifier of a record is not mutable. The identity of a user and name of a group are (using the clone/builder approach described). This is to more easily support name chanes or typos without having to re-create all the policies for that entity as well.

locks & merge conflicts - Thread locking is handled by the web tier using the same mechanism as the other Resources. For 1.0.0 we are introducing fine grain locking through a RevisionManager. Obtaining a write lock for a given record would require a client to include a Revision. The RevisionManager manager would validate the Revision and lock the Revision to prevent other clients from modifying the same record.

[mcgilman]

Pretty sure this is guaranteed non-null based on the Builder. This comment applies to all the Set's in User, Group, and AccessPolicy.

[mcgilman]

With the builders included, looks good to me.

@jtstorck @alopresto Thoughts?

[alopresto]

@mcgilman thanks for the response above. It feels a little to me like the authorizer has gotten conflated with user and group management tasks. If the authorizer' responsibility is strictly to manage and enforce policy, then we probably want a UserService to manage user retrieval, modification, etc. Maybe this is injected into the authorizer to allow various identity provider implementations (LDAP, Kerberos, etc.) to work with various authorizers (Ranger, NiFi internal, etc.) I'm not going to hold up this step with the understanding there are still more design decisions to be made.

[mcgilman]

@alopresto Great comment about the UserService. The reason we cannot do that directly is the extension point that is getting discovered is an Authorizer. However, this did make me think of possibly changing the MutableAuthorizer into an abstract class which implements (and marks final) the authorize() method. Then the MutableAuthorizer would simply handle User/Group/Policy persistence. Maybe the name changes too... something like AbstractPolicyBasedAuthorizer. The NiFi internal implementation would look like

class FileAuthorizer extends AbstractPolicyBasedAuthorizer

We'll hash out some of the details and update the PR accordingly. May be a good place to handle duplicate detection and whatnot.

[jtstorck]

For add/remote methods, should the methods that add/remove a single entity be removed in favor of just using the add/removeEntities with a collection size of one?

[bbende]

We probably could do that... personally I like being able to say addEntity("a").addEntity("b") rather than creating a set of 2 elements, but I'm sure theres some utility that returns a set from var args and could make it just as easy.

Since the methods are already there I would opt to leave them unless you felt really strongly about it.

[jtstorck]

I don't feel that strongly about it.

[jtstorck]

+1 looks good to me at this point, though @mcgilman's earlier comment on making MutableAuthorizer a base class does sound good, since the idea of the MutableAuthorizer was not so much about the authorization, but about the management of users/groups/policies from the NiFi UI.

[bbende]

@mcgilman @jtstorck @alopresto pushed two new commits, the first contains minor changes to address some of Matt's comments, the second is an attempt at what Matt suggested about how the MutableAuthorizer could be an abstract class that provides an implementation of authorize(), let me know what you think

[mcgilman]

@bbende The updated PR looks good to me. A number of great iterations here. Definitely think this is a solid foundation to continue the policy based authorizers managed by NiFi.