[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
GitHub user bbende opened a pull request: https://github.com/apache/nifi/pull/452 NIFI-1884 Defining API for Users, Groups, and Policies 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. You can merge this pull request into a Git repository by running: $ git pull https://github.com/bbende/nifi NIFI-1884 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/nifi/pull/452.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #452 commit 2c9510ba4a2c3b42811606528a98f4b5ed7d09ac Author: Bryan Bende Date: 2016-05-16T21:11:41Z NIFI-1884 Defining API for Users, Groups, and Policies --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user mcgilman commented on the pull request: https://github.com/apache/nifi/pull/452#issuecomment-220023481 Reviewing... --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user mcgilman commented on the pull request: https://github.com/apache/nifi/pull/452#issuecomment-220025580 +1 LGTM --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user jtstorck commented on a diff in the pull request: https://github.com/apache/nifi/pull/452#discussion_r63743405 --- Diff: nifi-api/src/main/java/org/apache/nifi/authorization/AccessPolicy.java --- @@ -0,0 +1,125 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.authorization; + +import java.util.Collections; +import java.util.Objects; +import java.util.Set; + +/** + * Defines a policy for a set of entities to perform a set of actions on a given resource. + */ +public class AccessPolicy { + +private final String identifier; + +private final Resource resource; + +private final Set entities; + +private final Set actions; + +/** + * Constructs a new policy with the given resource, entities, and actions. + * + * @param identifier the identifier of the policy + * @param resource the resource for the policy + * @param entities the entity ids for the policy (i.e. user or group ids) + * @param actions the actions for the policy + */ +public AccessPolicy(final String identifier, final Resource resource, final Set entities, final Set actions) { +if (identifier == null || identifier.trim().isEmpty()) { +throw new IllegalArgumentException("Identifier can not be null or empty"); +} + +if (resource == null) { +throw new IllegalArgumentException("Resource can not be null"); +} + +if (entities == null || entities.isEmpty()) { +throw new IllegalArgumentException("Entities can not be null or empty"); +} + +if (actions == null || actions.isEmpty()) { +throw new IllegalArgumentException("Actions can not be null or empty"); +} + +this.identifier = identifier; +this.resource = resource; +this.entities = Collections.unmodifiableSet(entities); +this.actions = Collections.unmodifiableSet(actions); +} + +/** + * @return the identifier for this policy + */ +public String getIdentifier() { +return identifier; +} + +/** + * @return the resource for this policy + */ +public Resource getResource() { +return resource; +} + +/** + * @return the set of entity ids for this policy + */ +public Set getEntities() { +return entities; --- End diff -- 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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user jtstorck commented on a diff in the pull request: https://github.com/apache/nifi/pull/452#discussion_r63744574 --- Diff: nifi-api/src/main/java/org/apache/nifi/authorization/AccessPolicy.java --- @@ -0,0 +1,125 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.authorization; + +import java.util.Collections; +import java.util.Objects; +import java.util.Set; + +/** + * Defines a policy for a set of entities to perform a set of actions on a given resource. + */ +public class AccessPolicy { + +private final String identifier; + +private final Resource resource; + +private final Set entities; + +private final Set actions; + +/** + * Constructs a new policy with the given resource, entities, and actions. + * + * @param identifier the identifier of the policy + * @param resource the resource for the policy + * @param entities the entity ids for the policy (i.e. user or group ids) + * @param actions the actions for the policy + */ +public AccessPolicy(final String identifier, final Resource resource, final Set entities, final Set actions) { +if (identifier == null || identifier.trim().isEmpty()) { +throw new IllegalArgumentException("Identifier can not be null or empty"); +} + +if (resource == null) { +throw new IllegalArgumentException("Resource can not be null"); +} + +if (entities == null || entities.isEmpty()) { +throw new IllegalArgumentException("Entities can not be null or empty"); +} + +if (actions == null || actions.isEmpty()) { +throw new IllegalArgumentException("Actions can not be null or empty"); +} + +this.identifier = identifier; +this.resource = resource; +this.entities = Collections.unmodifiableSet(entities); +this.actions = Collections.unmodifiableSet(actions); +} + +/** + * @return the identifier for this policy + */ +public String getIdentifier() { +return identifier; +} + +/** + * @return the resource for this policy + */ +public Resource getResource() { +return resource; +} + +/** + * @return the set of entity ids for this policy + */ +public Set getEntities() { +return entities; +} + +/** + * @return the set of actions for this policy + */ +public Set getActions() { +return actions; +} + +@Override +public boolean equals(Object obj) { +if (obj == null) { +return false; +} +if (getClass() != obj.getClass()) { +return false; +} + +final AccessPolicy other = (AccessPolicy) obj; + +return this.identifier.equals(other.getIdentifier()) +&& this.resource.getIdentifier().equals(other.getResource().getIdentifier()) +&& this.entities.equals(other.entities) +&& this.actions.equals(other.actions); +} + +@Override +public int hashCode() { +int hash = 7; +hash = 53 * hash + Objects.hash(this.identifier, this.resource, this.entities, this.actions); --- End diff -- When using Objects.hash(), you should be able to just return the invocation of that with the class members you want included in the hash: `return Objects.hash(this.identifier, this.resource, this.entities, this.actions);` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.or
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user jtstorck commented on a diff in the pull request: https://github.com/apache/nifi/pull/452#discussion_r63745403 --- Diff: nifi-api/src/main/java/org/apache/nifi/authorization/Group.java --- @@ -0,0 +1,115 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.authorization; + +import java.util.Collections; +import java.util.HashSet; +import java.util.Objects; +import java.util.Set; + +/** + * A group that users can belong to. + */ +public class Group { + +private final String identifier; + +private final String name; + +private final Set users; + +/** + * Constructs a new group with the given identifier and name. + * + * @param identifier a unique identifier for the group + * @param name the name of the group + */ +public Group(final String identifier, final String name) { +this(identifier, name, null); +} + +/** + * Constructs a new group with the given identifier, name, and users. + * + * @param identifier a unique identifier for the group + * @param name the name of the group + * @param users the list of user identifiers that belong to this group + */ +public Group(final String identifier, final String name, final Set users) { +if (identifier == null || identifier.trim().isEmpty()) { +throw new IllegalArgumentException("Identifier can not be null or empty"); +} + +if (name == null || name.trim().isEmpty()) { +throw new IllegalArgumentException("Name can not be null or empty"); +} + +this.identifier = identifier; +this.name = name; +this.users = (users == null ? Collections.unmodifiableSet(new HashSet<>()) : Collections.unmodifiableSet(users)); +} + +/** + * @return the identifier of the group + */ +public String getIdentifier() { +return identifier; +} + +/** + * @return the name of the group + */ +public String getName() { +return name; +} + +/** + * @return the list of user identifiers that belong to this group + */ +public Set getUsers() { +return users; +} + +@Override +public boolean equals(Object obj) { +if (obj == null) { +return false; +} +if (getClass() != obj.getClass()) { +return false; +} + +final Group other = (Group) obj; +if (!Objects.equals(this.identifier, other.identifier)) { --- End diff -- Can you just return the result of `Objects.equals(this.identifier, other.identifier)` here? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user alopresto commented on the pull request: https://github.com/apache/nifi/pull/452#issuecomment-220108280 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 `Set`s are immutable. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user bbende commented on the pull request: https://github.com/apache/nifi/pull/452#issuecomment-220121052 @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? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user bbende commented on a diff in the pull request: https://github.com/apache/nifi/pull/452#discussion_r63760205 --- Diff: nifi-api/src/main/java/org/apache/nifi/authorization/Group.java --- @@ -0,0 +1,115 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.authorization; + +import java.util.Collections; +import java.util.HashSet; +import java.util.Objects; +import java.util.Set; + +/** + * A group that users can belong to. + */ +public class Group { + +private final String identifier; + +private final String name; + +private final Set users; + +/** + * Constructs a new group with the given identifier and name. + * + * @param identifier a unique identifier for the group + * @param name the name of the group + */ +public Group(final String identifier, final String name) { +this(identifier, name, null); +} + +/** + * Constructs a new group with the given identifier, name, and users. + * + * @param identifier a unique identifier for the group + * @param name the name of the group + * @param users the list of user identifiers that belong to this group + */ +public Group(final String identifier, final String name, final Set users) { +if (identifier == null || identifier.trim().isEmpty()) { +throw new IllegalArgumentException("Identifier can not be null or empty"); +} + +if (name == null || name.trim().isEmpty()) { +throw new IllegalArgumentException("Name can not be null or empty"); +} + +this.identifier = identifier; +this.name = name; +this.users = (users == null ? Collections.unmodifiableSet(new HashSet<>()) : Collections.unmodifiableSet(users)); +} + +/** + * @return the identifier of the group + */ +public String getIdentifier() { +return identifier; +} + +/** + * @return the name of the group + */ +public String getName() { +return name; +} + +/** + * @return the list of user identifiers that belong to this group + */ +public Set getUsers() { +return users; +} + +@Override +public boolean equals(Object obj) { +if (obj == null) { +return false; +} +if (getClass() != obj.getClass()) { +return false; +} + +final Group other = (Group) obj; +if (!Objects.equals(this.identifier, other.identifier)) { --- End diff -- Good call, will do that and will verify other equals methods are consistent if necessary --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user bbende commented on a diff in the pull request: https://github.com/apache/nifi/pull/452#discussion_r63759950 --- Diff: nifi-api/src/main/java/org/apache/nifi/authorization/AccessPolicy.java --- @@ -0,0 +1,125 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.authorization; + +import java.util.Collections; +import java.util.Objects; +import java.util.Set; + +/** + * Defines a policy for a set of entities to perform a set of actions on a given resource. + */ +public class AccessPolicy { + +private final String identifier; + +private final Resource resource; + +private final Set entities; + +private final Set actions; + +/** + * Constructs a new policy with the given resource, entities, and actions. + * + * @param identifier the identifier of the policy + * @param resource the resource for the policy + * @param entities the entity ids for the policy (i.e. user or group ids) + * @param actions the actions for the policy + */ +public AccessPolicy(final String identifier, final Resource resource, final Set entities, final Set actions) { +if (identifier == null || identifier.trim().isEmpty()) { +throw new IllegalArgumentException("Identifier can not be null or empty"); +} + +if (resource == null) { +throw new IllegalArgumentException("Resource can not be null"); +} + +if (entities == null || entities.isEmpty()) { +throw new IllegalArgumentException("Entities can not be null or empty"); +} + +if (actions == null || actions.isEmpty()) { +throw new IllegalArgumentException("Actions can not be null or empty"); +} + +this.identifier = identifier; +this.resource = resource; +this.entities = Collections.unmodifiableSet(entities); +this.actions = Collections.unmodifiableSet(actions); +} + +/** + * @return the identifier for this policy + */ +public String getIdentifier() { +return identifier; +} + +/** + * @return the resource for this policy + */ +public Resource getResource() { +return resource; +} + +/** + * @return the set of entity ids for this policy + */ +public Set getEntities() { +return entities; --- End diff -- Good call, I prefer documenting in the javadoc --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user bbende commented on a diff in the pull request: https://github.com/apache/nifi/pull/452#discussion_r63760082 --- Diff: nifi-api/src/main/java/org/apache/nifi/authorization/AccessPolicy.java --- @@ -0,0 +1,125 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.authorization; + +import java.util.Collections; +import java.util.Objects; +import java.util.Set; + +/** + * Defines a policy for a set of entities to perform a set of actions on a given resource. + */ +public class AccessPolicy { + +private final String identifier; + +private final Resource resource; + +private final Set entities; + +private final Set actions; + +/** + * Constructs a new policy with the given resource, entities, and actions. + * + * @param identifier the identifier of the policy + * @param resource the resource for the policy + * @param entities the entity ids for the policy (i.e. user or group ids) + * @param actions the actions for the policy + */ +public AccessPolicy(final String identifier, final Resource resource, final Set entities, final Set actions) { +if (identifier == null || identifier.trim().isEmpty()) { +throw new IllegalArgumentException("Identifier can not be null or empty"); +} + +if (resource == null) { +throw new IllegalArgumentException("Resource can not be null"); +} + +if (entities == null || entities.isEmpty()) { +throw new IllegalArgumentException("Entities can not be null or empty"); +} + +if (actions == null || actions.isEmpty()) { +throw new IllegalArgumentException("Actions can not be null or empty"); +} + +this.identifier = identifier; +this.resource = resource; +this.entities = Collections.unmodifiableSet(entities); +this.actions = Collections.unmodifiableSet(actions); +} + +/** + * @return the identifier for this policy + */ +public String getIdentifier() { +return identifier; +} + +/** + * @return the resource for this policy + */ +public Resource getResource() { +return resource; +} + +/** + * @return the set of entity ids for this policy + */ +public Set getEntities() { +return entities; +} + +/** + * @return the set of actions for this policy + */ +public Set getActions() { +return actions; +} + +@Override +public boolean equals(Object obj) { +if (obj == null) { +return false; +} +if (getClass() != obj.getClass()) { +return false; +} + +final AccessPolicy other = (AccessPolicy) obj; + +return this.identifier.equals(other.getIdentifier()) +&& this.resource.getIdentifier().equals(other.getResource().getIdentifier()) +&& this.entities.equals(other.entities) +&& this.actions.equals(other.actions); +} + +@Override +public int hashCode() { +int hash = 7; +hash = 53 * hash + Objects.hash(this.identifier, this.resource, this.entities, this.actions); --- End diff -- Good call, will update all the hashCode implementations --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user alopresto commented on the pull request: https://github.com/apache/nifi/pull/452#issuecomment-220227038 @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? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user mcgilman commented on the pull request: https://github.com/apache/nifi/pull/452#issuecomment-220307255 @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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user mcgilman commented on a diff in the pull request: https://github.com/apache/nifi/pull/452#discussion_r63955889 --- Diff: nifi-api/src/main/java/org/apache/nifi/authorization/AccessPolicy.java --- @@ -0,0 +1,291 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.authorization; + +import java.util.Collections; +import java.util.HashSet; +import java.util.Objects; +import java.util.Set; + +/** + * Defines a policy for a set of entities to perform a set of actions on a given resource. + */ +public class AccessPolicy { + +private final String identifier; + +private final Resource resource; + +private final Set entities; + +private final Set actions; + +private AccessPolicy(final AccessPolicyBuilder builder) { +this.identifier = builder.identifier; +this.resource = builder.resource; + +Set entities = new HashSet<>(); +if (builder.entities != null) { --- End diff -- Pretty sure this is guaranteed non-null based on the Builder. This comment applies to all the Set's in User, Group, and AccessPolicy. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user mcgilman commented on a diff in the pull request: https://github.com/apache/nifi/pull/452#discussion_r63956868 --- Diff: nifi-api/src/main/java/org/apache/nifi/authorization/AccessPolicy.java --- @@ -0,0 +1,291 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.authorization; + +import java.util.Collections; +import java.util.HashSet; +import java.util.Objects; +import java.util.Set; + +/** + * Defines a policy for a set of entities to perform a set of actions on a given resource. + */ +public class AccessPolicy { + +private final String identifier; + +private final Resource resource; + +private final Set entities; + +private final Set actions; + +private AccessPolicy(final AccessPolicyBuilder builder) { +this.identifier = builder.identifier; +this.resource = builder.resource; + +Set entities = new HashSet<>(); +if (builder.entities != null) { +entities.addAll(builder.entities); +} +this.entities = Collections.unmodifiableSet(entities); + +Set actions = new HashSet<>(); +if (builder.actions != null) { +actions.addAll(builder.actions); +} +this.actions = Collections.unmodifiableSet(actions); + +if (this.identifier == null || this.identifier.trim().isEmpty()) { +throw new IllegalArgumentException("Identifier can not be null or empty"); +} + +if (this.resource == null) { +throw new IllegalArgumentException("Resource can not be null"); +} + +if (this.entities == null || this.entities.isEmpty()) { +throw new IllegalArgumentException("Entities can not be null or empty"); +} + +if (this.actions == null || this.actions.isEmpty()) { +throw new IllegalArgumentException("Actions can not be null or empty"); +} +} + +/** + * @return the identifier for this policy + */ +public String getIdentifier() { +return identifier; +} + +/** + * @return the resource for this policy + */ +public Resource getResource() { +return resource; +} + +/** + * @return an unmodifiable set of entity ids for this policy + */ +public Set getEntities() { +return entities; +} + +/** + * @return an unmodifiable set of actions for this policy + */ +public Set getActions() { +return actions; +} + +@Override +public boolean equals(Object obj) { +if (obj == null) { +return false; +} +if (getClass() != obj.getClass()) { +return false; +} + +final AccessPolicy other = (AccessPolicy) obj; +return Objects.equals(this.identifier, other.identifier); +} + +@Override +public int hashCode() { +return Objects.hashCode(this.identifier); +} + +@Override +public String toString() { +return String.format("identifier[%s], resource[%s], entityId[%s], action[%s]", --- End diff -- I think there is a trailing ", " in the String.format. This applies the toString() in User, Group, and AccessPolicy. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user mcgilman commented on the pull request: https://github.com/apache/nifi/pull/452#issuecomment-220457062 With the builders included, looks good to me. @jtstorck @alopresto Thoughts? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user alopresto commented on the pull request: https://github.com/apache/nifi/pull/452#issuecomment-220464739 @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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user mcgilman commented on the pull request: https://github.com/apache/nifi/pull/452#issuecomment-220469545 @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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user jtstorck commented on a diff in the pull request: https://github.com/apache/nifi/pull/452#discussion_r64040788 --- Diff: nifi-api/src/main/java/org/apache/nifi/authorization/AccessPolicy.java --- @@ -0,0 +1,291 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.authorization; + +import java.util.Collections; +import java.util.HashSet; +import java.util.Objects; +import java.util.Set; + +/** + * Defines a policy for a set of entities to perform a set of actions on a given resource. + */ +public class AccessPolicy { + +private final String identifier; + +private final Resource resource; + +private final Set entities; + +private final Set actions; + +private AccessPolicy(final AccessPolicyBuilder builder) { +this.identifier = builder.identifier; +this.resource = builder.resource; + +Set entities = new HashSet<>(); +if (builder.entities != null) { +entities.addAll(builder.entities); +} +this.entities = Collections.unmodifiableSet(entities); + +Set actions = new HashSet<>(); +if (builder.actions != null) { +actions.addAll(builder.actions); +} +this.actions = Collections.unmodifiableSet(actions); + +if (this.identifier == null || this.identifier.trim().isEmpty()) { +throw new IllegalArgumentException("Identifier can not be null or empty"); +} + +if (this.resource == null) { +throw new IllegalArgumentException("Resource can not be null"); +} + +if (this.entities == null || this.entities.isEmpty()) { +throw new IllegalArgumentException("Entities can not be null or empty"); +} + +if (this.actions == null || this.actions.isEmpty()) { +throw new IllegalArgumentException("Actions can not be null or empty"); +} +} + +/** + * @return the identifier for this policy + */ +public String getIdentifier() { +return identifier; +} + +/** + * @return the resource for this policy + */ +public Resource getResource() { +return resource; +} + +/** + * @return an unmodifiable set of entity ids for this policy + */ +public Set getEntities() { +return entities; +} + +/** + * @return an unmodifiable set of actions for this policy + */ +public Set getActions() { +return actions; +} + +@Override +public boolean equals(Object obj) { +if (obj == null) { +return false; +} +if (getClass() != obj.getClass()) { +return false; +} + +final AccessPolicy other = (AccessPolicy) obj; +return Objects.equals(this.identifier, other.identifier); +} + +@Override +public int hashCode() { +return Objects.hashCode(this.identifier); +} + +@Override +public String toString() { +return String.format("identifier[%s], resource[%s], entityId[%s], action[%s]", +getIdentifier(), getResource().getIdentifier(), getEntities(), getActions(), ", "); +} + +/** + * Builder for Access Policies. + */ +public static class AccessPolicyBuilder { + +private String identifier; +private Resource resource; +private Set entities = new HashSet<>(); +private Set actions = new HashSet<>(); +private final boolean fromPolicy; + +/** + * Default constructor for building a new AccessPolicy. + */ +public AccessPolicyBuilder() { +this.fromPolicy = false;
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user jtstorck commented on the pull request: https://github.com/apache/nifi/pull/452#issuecomment-220607952 +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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user bbende commented on a diff in the pull request: https://github.com/apache/nifi/pull/452#discussion_r64076734 --- Diff: nifi-api/src/main/java/org/apache/nifi/authorization/AccessPolicy.java --- @@ -0,0 +1,291 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.authorization; + +import java.util.Collections; +import java.util.HashSet; +import java.util.Objects; +import java.util.Set; + +/** + * Defines a policy for a set of entities to perform a set of actions on a given resource. + */ +public class AccessPolicy { + +private final String identifier; + +private final Resource resource; + +private final Set entities; + +private final Set actions; + +private AccessPolicy(final AccessPolicyBuilder builder) { +this.identifier = builder.identifier; +this.resource = builder.resource; + +Set entities = new HashSet<>(); +if (builder.entities != null) { +entities.addAll(builder.entities); +} +this.entities = Collections.unmodifiableSet(entities); + +Set actions = new HashSet<>(); +if (builder.actions != null) { +actions.addAll(builder.actions); +} +this.actions = Collections.unmodifiableSet(actions); + +if (this.identifier == null || this.identifier.trim().isEmpty()) { +throw new IllegalArgumentException("Identifier can not be null or empty"); +} + +if (this.resource == null) { +throw new IllegalArgumentException("Resource can not be null"); +} + +if (this.entities == null || this.entities.isEmpty()) { +throw new IllegalArgumentException("Entities can not be null or empty"); +} + +if (this.actions == null || this.actions.isEmpty()) { +throw new IllegalArgumentException("Actions can not be null or empty"); +} +} + +/** + * @return the identifier for this policy + */ +public String getIdentifier() { +return identifier; +} + +/** + * @return the resource for this policy + */ +public Resource getResource() { +return resource; +} + +/** + * @return an unmodifiable set of entity ids for this policy + */ +public Set getEntities() { +return entities; +} + +/** + * @return an unmodifiable set of actions for this policy + */ +public Set getActions() { +return actions; +} + +@Override +public boolean equals(Object obj) { +if (obj == null) { +return false; +} +if (getClass() != obj.getClass()) { +return false; +} + +final AccessPolicy other = (AccessPolicy) obj; +return Objects.equals(this.identifier, other.identifier); +} + +@Override +public int hashCode() { +return Objects.hashCode(this.identifier); +} + +@Override +public String toString() { +return String.format("identifier[%s], resource[%s], entityId[%s], action[%s]", +getIdentifier(), getResource().getIdentifier(), getEntities(), getActions(), ", "); +} + +/** + * Builder for Access Policies. + */ +public static class AccessPolicyBuilder { + +private String identifier; +private Resource resource; +private Set entities = new HashSet<>(); +private Set actions = new HashSet<>(); +private final boolean fromPolicy; + +/** + * Default constructor for building a new AccessPolicy. + */ +public AccessPolicyBuilder() { +this.fromPolicy = false; +
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user jtstorck commented on a diff in the pull request: https://github.com/apache/nifi/pull/452#discussion_r64086436 --- Diff: nifi-api/src/main/java/org/apache/nifi/authorization/AccessPolicy.java --- @@ -0,0 +1,291 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.nifi.authorization; + +import java.util.Collections; +import java.util.HashSet; +import java.util.Objects; +import java.util.Set; + +/** + * Defines a policy for a set of entities to perform a set of actions on a given resource. + */ +public class AccessPolicy { + +private final String identifier; + +private final Resource resource; + +private final Set entities; + +private final Set actions; + +private AccessPolicy(final AccessPolicyBuilder builder) { +this.identifier = builder.identifier; +this.resource = builder.resource; + +Set entities = new HashSet<>(); +if (builder.entities != null) { +entities.addAll(builder.entities); +} +this.entities = Collections.unmodifiableSet(entities); + +Set actions = new HashSet<>(); +if (builder.actions != null) { +actions.addAll(builder.actions); +} +this.actions = Collections.unmodifiableSet(actions); + +if (this.identifier == null || this.identifier.trim().isEmpty()) { +throw new IllegalArgumentException("Identifier can not be null or empty"); +} + +if (this.resource == null) { +throw new IllegalArgumentException("Resource can not be null"); +} + +if (this.entities == null || this.entities.isEmpty()) { +throw new IllegalArgumentException("Entities can not be null or empty"); +} + +if (this.actions == null || this.actions.isEmpty()) { +throw new IllegalArgumentException("Actions can not be null or empty"); +} +} + +/** + * @return the identifier for this policy + */ +public String getIdentifier() { +return identifier; +} + +/** + * @return the resource for this policy + */ +public Resource getResource() { +return resource; +} + +/** + * @return an unmodifiable set of entity ids for this policy + */ +public Set getEntities() { +return entities; +} + +/** + * @return an unmodifiable set of actions for this policy + */ +public Set getActions() { +return actions; +} + +@Override +public boolean equals(Object obj) { +if (obj == null) { +return false; +} +if (getClass() != obj.getClass()) { +return false; +} + +final AccessPolicy other = (AccessPolicy) obj; +return Objects.equals(this.identifier, other.identifier); +} + +@Override +public int hashCode() { +return Objects.hashCode(this.identifier); +} + +@Override +public String toString() { +return String.format("identifier[%s], resource[%s], entityId[%s], action[%s]", +getIdentifier(), getResource().getIdentifier(), getEntities(), getActions(), ", "); +} + +/** + * Builder for Access Policies. + */ +public static class AccessPolicyBuilder { + +private String identifier; +private Resource resource; +private Set entities = new HashSet<>(); +private Set actions = new HashSet<>(); +private final boolean fromPolicy; + +/** + * Default constructor for building a new AccessPolicy. + */ +public AccessPolicyBuilder() { +this.fromPolicy = false;
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user bbende commented on the pull request: https://github.com/apache/nifi/pull/452#issuecomment-221046683 @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 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user mcgilman commented on the pull request: https://github.com/apache/nifi/pull/452#issuecomment-221086451 @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. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] nifi pull request: NIFI-1884 Defining API for Users, Groups, and P...
Github user asfgit closed the pull request at: https://github.com/apache/nifi/pull/452 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---