From c1fcc3569d4a671184b440ad1ab75392d2d049b6 Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Fri, 23 Sep 2016 17:49:08 +0200 Subject: [PATCH] Disallow deleting final user in neo4j-admin to match server The server does not allow you to delete yourself. In neo4j-admin the running user is not identified, but the logged in user is assumed to be the admin. The closest role to match server behaviour is to disallow deleting the last user. --- .../admin/security/UsersCommand.java | 4 ++ .../admin/security/CommandTestBase.java | 17 ++++---- .../admin/security/DeleteCommandTest.java | 20 ++++++++++ .../admin/security/UsersCommandIT.java | 40 ++++++++++++++++++- 4 files changed, 70 insertions(+), 11 deletions(-) diff --git a/community/security/src/main/java/org/neo4j/commandline/admin/security/UsersCommand.java b/community/security/src/main/java/org/neo4j/commandline/admin/security/UsersCommand.java index db08e8e592445..bcc9b40311057 100644 --- a/community/security/src/main/java/org/neo4j/commandline/admin/security/UsersCommand.java +++ b/community/security/src/main/java/org/neo4j/commandline/admin/security/UsersCommand.java @@ -183,6 +183,10 @@ private void deleteUser( String username ) throws Throwable { BasicAuthManager authManager = getAuthManager(); authManager.getUser( username ); // Will throw error on missing user + if ( authManager.getAllUsernames().size() == 1 ) + { + throw new IllegalArgumentException( "Deleting the only remaining user '" + username + "' is not allowed" ); + } if ( authManager.deleteUser( username ) ) { outsideWorld.stdOutLine( "Deleted user '" + username + "'" ); diff --git a/community/security/src/test/java/org/neo4j/commandline/admin/security/CommandTestBase.java b/community/security/src/test/java/org/neo4j/commandline/admin/security/CommandTestBase.java index e3c99b43e1bdf..46cc421ca9808 100644 --- a/community/security/src/test/java/org/neo4j/commandline/admin/security/CommandTestBase.java +++ b/community/security/src/test/java/org/neo4j/commandline/admin/security/CommandTestBase.java @@ -22,7 +22,6 @@ import org.junit.Before; import java.io.File; -import java.io.IOException; import java.nio.file.FileSystems; import org.neo4j.commandline.admin.AdminTool; @@ -30,7 +29,6 @@ import org.neo4j.commandline.admin.OutsideWorld; import org.neo4j.io.fs.DelegateFileSystemAbstraction; import org.neo4j.io.fs.FileSystemAbstraction; -import org.neo4j.kernel.api.exceptions.InvalidArgumentsException; import org.neo4j.logging.NullLogProvider; import org.neo4j.server.security.auth.Credential; import org.neo4j.server.security.auth.FileUserRepository; @@ -40,6 +38,7 @@ import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.contains; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.reset; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -53,6 +52,7 @@ abstract class CommandTestBase File confDir; File homeDir; OutsideWorld out; + AdminTool tool; @Before public void setup() @@ -60,7 +60,9 @@ public void setup() graphDir = testDir.graphDbDir(); confDir = ensureDir( "conf" ); homeDir = ensureDir( "home" ); + out = mock( OutsideWorld.class ); resetOutsideWorldMock(); + tool = new AdminTool( CommandLocator.fromServiceLocator(), out, true ); } protected File ensureDir( String name ) @@ -75,7 +77,7 @@ protected File ensureDir( String name ) protected void resetOutsideWorldMock() { - out = mock( OutsideWorld.class ); + reset(out); when( out.fileSystem() ).thenReturn( fileSystem ); } @@ -84,9 +86,10 @@ private File authFile() return new File( new File( new File( testDir.graphDbDir(), "data" ), "dbms" ), "auth" ); } - User createTestUser( String username, String password ) throws IOException, InvalidArgumentsException + User createTestUser( String username, String password ) throws Throwable { FileUserRepository users = new FileUserRepository( fileSystem, authFile(), NullLogProvider.getInstance() ); + users.start(); User user = new User.Builder( username, Credential.forPassword( password ) ).withRequiredPasswordChange( true ) .build(); @@ -109,7 +112,7 @@ String[] args(String... args) protected abstract String command(); - private String[] makeArgs( String subCommand, String... args ) + protected String[] makeArgs( String subCommand, String... args ) { String[] allArgs = new String[args.length + 2]; System.arraycopy( args, 0, allArgs, 2, args.length ); @@ -120,9 +123,7 @@ private String[] makeArgs( String subCommand, String... args ) void assertFailedSubCommand( String command, String[] args, String... errors ) { - // When running set password on a failing case (missing user, or other error) resetOutsideWorldMock(); - AdminTool tool = new AdminTool( CommandLocator.fromServiceLocator(), out, true ); tool.execute( graphDir.toPath(), confDir.toPath(), makeArgs( command, args ) ); // Then we get the expected error @@ -136,9 +137,7 @@ void assertFailedSubCommand( String command, String[] args, String... errors ) void assertSuccessfulSubCommand( String command, String[] args, String... messages ) { - // When running set password on a successful case (user exists) resetOutsideWorldMock(); - AdminTool tool = new AdminTool( CommandLocator.fromServiceLocator(), out, true ); tool.execute( graphDir.toPath(), confDir.toPath(), makeArgs( command, args )); // Then we get the expected output messages diff --git a/community/security/src/test/java/org/neo4j/commandline/admin/security/DeleteCommandTest.java b/community/security/src/test/java/org/neo4j/commandline/admin/security/DeleteCommandTest.java index d54ac151e879c..27a6edc36d124 100644 --- a/community/security/src/test/java/org/neo4j/commandline/admin/security/DeleteCommandTest.java +++ b/community/security/src/test/java/org/neo4j/commandline/admin/security/DeleteCommandTest.java @@ -56,10 +56,30 @@ public void shouldFailToDeleteMissingUser() throws Throwable } } + @Test + public void shouldNotDeleteDefaultUser() throws Throwable + { + // Given - default user + createTestUser( "neo4j", "abc" ); + + // When + try + { + UsersCommand usersCommand = new UsersCommand( graphDir.toPath(), confDir.toPath(), out ); + usersCommand.execute( new String[]{"delete", "neo4j"} ); + } + catch ( CommandFailed e ) + { + // Expect failure message + assertThat( e.getMessage(), containsString( "Deleting the only remaining user 'neo4j' is not allowed" ) ); + } + } + @Test public void shouldDeleteExistingUser() throws Throwable { // Given - existing user + createTestUser( "neo4j", "abc" ); createTestUser( "another", "abc" ); // When - creating new user diff --git a/community/security/src/test/java/org/neo4j/commandline/admin/security/UsersCommandIT.java b/community/security/src/test/java/org/neo4j/commandline/admin/security/UsersCommandIT.java index 860aefc57a99e..c152177f36ca6 100644 --- a/community/security/src/test/java/org/neo4j/commandline/admin/security/UsersCommandIT.java +++ b/community/security/src/test/java/org/neo4j/commandline/admin/security/UsersCommandIT.java @@ -19,6 +19,7 @@ */ package org.neo4j.commandline.admin.security; +import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.RuleChain; @@ -36,6 +37,18 @@ public class UsersCommandIT extends UsersCommandTestBase @Rule public RuleChain ruleChain = RuleChain.outerRule( testDir ); + @Before + public void setup() + { + super.setup(); + // the following line ensures that the test setup code (like creating test users) works on the same initial + // environment that the actual tested commands will encounter. In particular some auth state is created + // on demand in both the UserCommand and in the real server. We want that state created before the tests + // are run. + tool.execute( graphDir.toPath(), confDir.toPath(), makeArgs( "list") ); + resetOutsideWorldMock(); + } + @Test public void shouldGetUsageErrorsWithNoSubCommand() throws Throwable { @@ -259,20 +272,43 @@ public void shouldNotDeleteMissingUser() throws Throwable } @Test - public void shouldDeleteDefaultUser() throws Throwable + public void shouldNotDeleteDefaultUserIfNoOtherUserExists() throws Throwable { // Given default state (only default user) + // When running 'delete' with correct parameters, expect error + assertFailedSubCommand( "delete", args("neo4j"), "Deleting the only remaining user 'neo4j' is not allowed" ); + } + + @Test + public void shouldDeleteDefaultUserIfAnotherUserExists() throws Throwable + { + // Given a user that requires password change + createTestUser( "another", "neo4j" ); + // When running 'delete' with correct parameters, expect success assertSuccessfulSubCommand( "delete", args("neo4j"), "Deleted user 'neo4j'" ); } @Test - public void shouldDeleteExistingUser() throws Throwable + public void shouldNotDeleteExistingUserIfNoOtherUserExists() throws Throwable { // Given a user that requires password change createTestUser( "another", "neo4j" ); + // When running 'delete' with correct parameters, expect success + assertSuccessfulSubCommand( "delete", args("neo4j"), "Deleted user 'neo4j'" ); + + // When running 'delete' with correct parameters, expect error + assertFailedSubCommand( "delete", args("another"), "Deleting the only remaining user 'another' is not allowed" ); + } + + @Test + public void shouldDeleteExistingUser() throws Throwable + { + // Given the default user and another new user + createTestUser( "another", "neo4j" ); + // When running 'create' with correct parameters, expect correct output assertSuccessfulSubCommand( "delete", args("another"), "Deleted user 'another'" ); }