diff --git a/community/security/src/main/java/org/neo4j/server/security/auth/FileUserRepository.java b/community/security/src/main/java/org/neo4j/server/security/auth/FileUserRepository.java index 53fb756f61143..2fdde14d13999 100644 --- a/community/security/src/main/java/org/neo4j/server/security/auth/FileUserRepository.java +++ b/community/security/src/main/java/org/neo4j/server/security/auth/FileUserRepository.java @@ -44,6 +44,8 @@ public class FileUserRepository extends LifecycleAdapter implements UserReposito { private final Path authFile; + // TODO: We could improve concurrency by using a ReadWriteLock + /** Quick lookup of users by name */ private final Map usersByName = new ConcurrentHashMap<>(); private final Log log; diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileRoleRepository.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileRoleRepository.java index 20d8269d05903..55f2e86b94b1f 100644 --- a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileRoleRepository.java +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileRoleRepository.java @@ -30,11 +30,9 @@ import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentSkipListSet; -import org.neo4j.kernel.api.security.exception.IllegalCredentialsException; import org.neo4j.kernel.lifecycle.LifecycleAdapter; import org.neo4j.logging.Log; import org.neo4j.logging.LogProvider; -//import org.neo4j.server.security.auth.UserRepository; import org.neo4j.server.security.auth.exception.ConcurrentModificationException; import static java.nio.file.StandardCopyOption.ATOMIC_MOVE; @@ -49,6 +47,8 @@ public class FileRoleRepository extends LifecycleAdapter implements RoleReposito { private final Path roleFile; + // TODO: We could improve concurrency by using a ReadWriteLock + /** Quick lookup of roles by name */ private final Map rolesByName = new ConcurrentHashMap<>(); private final Map> rolesByUsername = new ConcurrentHashMap<>(); @@ -88,11 +88,11 @@ public void start() throws Throwable } @Override - public void create( RoleRecord role ) throws IllegalCredentialsException, IOException + public void create( RoleRecord role ) throws IllegalArgumentException, IOException { if ( !isValidName( role.name() ) ) { - throw new IllegalCredentialsException( "'" + role.name() + "' is not a valid role name." ); + throw new IllegalArgumentException( "'" + role.name() + "' is not a valid role name." ); } synchronized (this) @@ -102,7 +102,7 @@ public void create( RoleRecord role ) throws IllegalCredentialsException, IOExce { if ( other.name().equals( role.name() ) ) { - throw new IllegalCredentialsException( "The specified role already exists" ); + throw new IllegalArgumentException( "The specified role already exists" ); } } @@ -122,7 +122,7 @@ public void update( RoleRecord existingRole, RoleRecord updatedRole ) throws Con // Assert input is ok if ( !existingRole.name().equals( updatedRole.name() ) ) { - throw new IllegalArgumentException( "updatedRole has a different name" ); + throw new IllegalArgumentException( "updated role has a different name" ); } synchronized (this) diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileUserRealm.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileUserRealm.java index 040ca905b9720..3090de48bab7a 100644 --- a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileUserRealm.java +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/FileUserRealm.java @@ -139,10 +139,10 @@ int numberOfUsers() return userRepository.numberOfUsers(); } - User newUser( String username, String initialPassword, boolean requirePasswordChange ) throws - IOException, IllegalCredentialsException, ConcurrentModificationException + User newUser( String username, String initialPassword, boolean requirePasswordChange ) + throws IOException, IllegalCredentialsException { - assertValidName( username ); + assertValidUsername( username ); User user = new User.Builder() .withName( username ) @@ -154,35 +154,64 @@ User newUser( String username, String initialPassword, boolean requirePasswordCh return user; } - RoleRecord newRole( String roleName, String... users ) throws - IOException, IllegalCredentialsException, ConcurrentModificationException + RoleRecord newRole( String roleName, String... users ) throws IOException, IllegalCredentialsException { - assertValidName( roleName ); + assertValidRoleName( roleName ); + for (String username : users) + { + assertValidUsername( username ); + } SortedSet userSet = new TreeSet( Arrays.asList( users ) ); + RoleRecord role = new RoleRecord.Builder().withName( roleName ).withUsers( userSet ).build(); roleRepository.create( role ); return role; } - private void addUserToRole( User user, String roleName ) - throws IOException, IllegalCredentialsException, ConcurrentModificationException + void addUserToRole( String username, String roleName ) throws IOException { + assertValidUsername( username ); + assertValidRoleName( roleName ); + + // TODO: FIXME This is not atomic. E.g. the user could be deleted between here and updating the role + User user = userRepository.findByName( username ); + if ( user == null ) + { + throw new IllegalArgumentException( "User " + username + " does not exist." ); + } + + // TODO: FIXME This is not atomic. E.g. the role could be created between here and create + // In that case we would get an IllegalArgumentException, but what we want is a + // ConcurrentModificationException and then retry. We should use a RoleRepositoty.createOrUpdate instead. RoleRecord role = roleRepository.findByName( roleName ); if ( role == null ) { - RoleRecord newRole = new RoleRecord( roleName, user.name() ); + RoleRecord newRole = new RoleRecord( roleName, username ); roleRepository.create( newRole ); } else { - RoleRecord newRole = role.augment().withUser( user.name() ).build(); - roleRepository.update( role, newRole ); + RoleRecord newRole = role.augment().withUser( username ).build(); + try + { + roleRepository.update( role, newRole ); + } + catch ( ConcurrentModificationException e ) + { + // Try again + addUserToRole( username, roleName ); + } } } - private void assertValidName( String name ) + void removeUserFromRole( String username, String rolename ) throws IOException + { + // TODO + } + + private void assertValidUsername( String name ) { if ( !userRepository.isValidName( name ) ) { @@ -190,4 +219,13 @@ private void assertValidName( String name ) "User name contains illegal characters. Please use simple ascii characters and numbers." ); } } + + private void assertValidRoleName( String name ) + { + if ( !roleRepository.isValidName( name ) ) + { + throw new IllegalArgumentException( + "Role name contains illegal characters. Please use simple ascii characters and numbers." ); + } + } } diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleManager.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleManager.java new file mode 100644 index 0000000000000..0b3f8d3e5a371 --- /dev/null +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleManager.java @@ -0,0 +1,29 @@ +/* + * Copyright (c) 2002-2016 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Neo4j is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ +package org.neo4j.server.security.enterprise.auth; + +import java.io.IOException; + +public interface RoleManager +{ + void addUserToRole( String username, String roleName ) throws IOException; + + void removeUserFromRole( String username, String roleName ) throws IOException; +} diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleRepository.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleRepository.java index 9e1971b1fa2b4..7974ca4ad1cc1 100644 --- a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleRepository.java +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/RoleRepository.java @@ -22,7 +22,6 @@ import java.io.IOException; import java.util.Set; -import org.neo4j.kernel.api.security.exception.IllegalCredentialsException; import org.neo4j.kernel.lifecycle.Lifecycle; import org.neo4j.server.security.auth.exception.ConcurrentModificationException; @@ -38,9 +37,9 @@ public interface RoleRepository extends Lifecycle /** * Create a role, given that the roles token is unique. * @param role the new role object - * @throws IllegalCredentialsException if the role name is not valid + * @throws IllegalArgumentException if the role name is not valid */ - void create( RoleRecord role ) throws IllegalCredentialsException, IOException; + void create( RoleRecord role ) throws IllegalArgumentException, IOException; /** * Update a role, given that the role token is unique. diff --git a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/ShiroAuthManager.java b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/ShiroAuthManager.java index 7385ca521d85c..6cae290ddd0b9 100644 --- a/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/ShiroAuthManager.java +++ b/enterprise/security/src/main/java/org/neo4j/server/security/enterprise/auth/ShiroAuthManager.java @@ -40,9 +40,8 @@ import org.neo4j.server.security.auth.RateLimitedAuthenticationStrategy; import org.neo4j.server.security.auth.User; import org.neo4j.server.security.auth.UserRepository; -import org.neo4j.server.security.auth.exception.ConcurrentModificationException; -public class ShiroAuthManager extends BasicAuthManager +public class ShiroAuthManager extends BasicAuthManager implements RoleManager { private final SecurityManager securityManager; private final EhCacheManager cacheManager; @@ -108,32 +107,14 @@ public User newUser( String username, String initialPassword, boolean requirePas { assertAuthEnabled(); - try - { - return realm.newUser( username, initialPassword, requirePasswordChange ); - - } - catch ( ConcurrentModificationException e ) - { - // TODO: Try again - return null; - } + return realm.newUser( username, initialPassword, requirePasswordChange ); } public RoleRecord newRole( String roleName, String... users ) throws IOException, IllegalCredentialsException { assertAuthEnabled(); - try - { - return realm.newRole( roleName, users ); - - } - catch ( ConcurrentModificationException e ) - { - // TODO: Try again - return null; - } + return realm.newRole( roleName, users ); } @@ -184,4 +165,16 @@ public void setPassword( AuthSubject authSubject, String username, String passwo setUserPassword( username, password ); } + + @Override + public void addUserToRole( String username, String roleName ) throws IOException + { + realm.addUserToRole( username, roleName ); + } + + @Override + public void removeUserFromRole( String username, String roleName ) throws IOException + { + realm.removeUserFromRole( username, roleName ); + } } diff --git a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/InMemoryRoleRepository.java b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/InMemoryRoleRepository.java index f6c4f9862357d..22e7d1b161e71 100644 --- a/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/InMemoryRoleRepository.java +++ b/enterprise/security/src/test/java/org/neo4j/server/security/enterprise/auth/InMemoryRoleRepository.java @@ -48,7 +48,7 @@ public Set findByUsername( String username ) } @Override - public void create( RoleRecord role ) throws IllegalCredentialsException + public void create( RoleRecord role ) { synchronized (this) { @@ -57,7 +57,7 @@ public void create( RoleRecord role ) throws IllegalCredentialsException { if ( other.name().equals( role.name() ) ) { - throw new IllegalCredentialsException( "The specified role already exists" ); + throw new IllegalArgumentException( "The specified role already exists" ); } }