Mind review findings

docs/en/user/admin/settings.md

@@ -36,7 +36,7 @@ Example: If anonymous access is enabled and the "_anonymous" user has full acces
 The url of the RSS Release Feed for SCM-Manager. This provides up-to-date version information. To disable this feature just leave the url blank.
 
 #### User converter
-Internal users will automatically be converted to external on their first login using an external system. After convertion the users may only log in using the external system.
+Internal users will automatically be converted to external on their first login using an external system. After conversion the users may only log in using the external system.
 
 #### Fallback Mail Domain Name
 This domain name will be used to create email addresses for users without one when needed. It will not be used to send mails nor will be accessed otherwise.

docs/en/user/user/index.md

@@ -19,6 +19,6 @@ The "Create User" form can be used to create new users in SCM-Manager. New users
 ### User Details Page
 The user details page shows the information about the user.
 
-The active box shows whether the user is able to use SCM-Manager. The external box shows if it is an internal user or is managed by an external system.
+The active box shows whether the user is able to use SCM-Manager. The external box shows if it is an internal user or whether it is managed by an external system.
 
 ![User-Information](assets/user-information.png)

docs/en/user/user/settings.md

@@ -5,7 +5,7 @@ subtitle: Settings
 ### General
 In the general settings the display name, e-mail address, external flag and active status of an account can be edited. 
 
-If an user is converted from internal to external the passwords will be removed. Switching an external user to internal user a password must be set using the password modal dialogue.
+If a user is converted from internal to external the password is going to be removed. When switching an external user to an internal one, a password must be set using the password modal dialogue.
 
 ![User Password Modal](assets/user-password-modal.png)
 

scm-core/src/main/java/sonia/scm/security/SyncingRealmHelper.java

@@ -80,6 +80,7 @@ public final class SyncingRealmHelper {
    * @param ctx          administration context
    * @param userManager  user manager
    * @param groupManager group manager
+   * @deprecated
    */
   @Deprecated
   public SyncingRealmHelper(AdministrationContext ctx, UserManager userManager, GroupManager groupManager) {
@@ -110,17 +111,9 @@ public final class SyncingRealmHelper {
   public void store(final Group group) {
     ctx.runAsAdmin(() -> {
       if (groupManager.get(group.getId()) != null) {
-        try {
+        modifyGroup(group);
-          groupManager.modify(group);
-        } catch (NotFoundException e) {
-          throw new IllegalStateException("got NotFoundException though group " + group.getName() + " could be loaded", e);
-        }
       } else {
-        try {
+        createNewGroup(group);
-          groupManager.create(group);
-        } catch (AlreadyExistsException e) {
-          throw new IllegalStateException("got AlreadyExistsException though group " + group.getName() + " could not be loaded", e);
-        }
       }
     });
   }
@@ -133,30 +126,54 @@ public final class SyncingRealmHelper {
   public void store(final User user) {
     ctx.runAsAdmin(() -> {
       if (userManager.contains(user.getName())) {
-        User clone = user.clone();
+        modifyUser(user);
-        if (!externalUserConverters.isEmpty()) {
-          log.debug("execute available user converters");
-          for (ExternalUserConverter converter : externalUserConverters) {
-            clone = converter.convert(clone);
-          }
-        }
-
-        try {
-          userManager.modify(clone);
-        } catch (NotFoundException e) {
-          throw new IllegalStateException("got NotFoundException though user " + clone.getName() + " could be loaded", e);
-        }
       } else {
-        try {
+        createNewUser(user);
-          User clone = user.clone();
-          // New user created by syncing realm helper is always external
-          clone.setExternal(true);
-          userManager.create(clone);
-        } catch (AlreadyExistsException e) {
-          throw new IllegalStateException("got AlreadyExistsException though user " + user.getName() + " could not be loaded", e);
-
-        }
       }
     });
   }
+
+  private void createNewUser(User user) {
+    try {
+      User clone = user.clone();
+      // New user created by syncing realm helper is always external
+      clone.setExternal(true);
+      userManager.create(clone);
+    } catch (AlreadyExistsException e) {
+      throw new IllegalStateException("got AlreadyExistsException though user " + user.getName() + " could not be loaded", e);
+
+    }
+  }
+
+  private void modifyUser(User user) {
+    User clone = user.clone();
+    if (!externalUserConverters.isEmpty()) {
+      log.debug("execute available user converters");
+      for (ExternalUserConverter converter : externalUserConverters) {
+        clone = converter.convert(clone);
+      }
+    }
+
+    try {
+      userManager.modify(clone);
+    } catch (NotFoundException e) {
+      throw new IllegalStateException("got NotFoundException though user " + clone.getName() + " could be loaded", e);
+    }
+  }
+
+  private void createNewGroup(Group group) {
+    try {
+      groupManager.create(group);
+    } catch (AlreadyExistsException e) {
+      throw new IllegalStateException("got AlreadyExistsException though group " + group.getName() + " could not be loaded", e);
+    }
+  }
+
+  private void modifyGroup(Group group) {
+    try {
+      groupManager.modify(group);
+    } catch (NotFoundException e) {
+      throw new IllegalStateException("got NotFoundException though group " + group.getName() + " could be loaded", e);
+    }
+  }
 }

scm-core/src/main/java/sonia/scm/user/User.java

@@ -69,9 +69,9 @@ public class User extends BasicPropertiesAware implements Principal, ModelObject
   private String password;
   @Deprecated
   /**
-   * @deprecated Use external instead.
    * The user type is replaced by {@link external} flag
-   * Since 2.8.0
+   * @since 2.8.0
+   * @deprecated Use {@link external} instead.
    */
   private String type;
 

scm-ui/ui-webapp/src/users/components/UserForm.tsx

@@ -128,10 +128,10 @@ class UserForm extends React.Component<Props, State> {
     const { user, passwordValid } = this.state;
     event.preventDefault();
     if (!this.isInvalid()) {
+      this.props.submitForm(this.state.user);
       if (user.password && passwordValid && user._links?.password) {
         setPassword((user._links.password as Link).href, user.password).catch(error => this.setState({ error }));
       }
-      this.props.submitForm(this.state.user);
     }
   };
 
@@ -208,7 +208,7 @@ class UserForm extends React.Component<Props, State> {
               <Checkbox
                 label={t("user.externalFlag")}
                 onChange={this.handleExternalFlagChange}
-                checked={!!user?.external && user.external}
+                checked={!!user?.external}
                 helpText={t("help.externalFlagHelpText")}
               />
             </div>

scm-ui/ui-webapp/src/users/components/navLinks/SetPasswordNavLink.tsx

@@ -42,7 +42,8 @@ class ChangePasswordNavLink extends React.Component<Props> {
   }
 
   hasPermissionToSetPassword = () => {
-    return this.props.user._links.password;
+    const { user } = this.props;
+    return !user.external && user._links.password;
   };
 }
 

scm-ui/ui-webapp/src/users/components/table/Details.tsx

@@ -64,7 +64,7 @@ class Details extends React.Component<Props> {
           <tr>
             <th>{t("user.externalFlag")}</th>
             <td>
-              <Checkbox checked={!!user?.external && user.external} />
+              <Checkbox checked={!!user.external} />
             </td>
           </tr>
           <tr>

scm-webapp/src/main/java/sonia/scm/user/DefaultUserManager.java

@@ -389,8 +389,8 @@ public class DefaultUserManager extends AbstractUserManager
     if (user == null) {
       throw new NotFoundException(User.class, userId);
     }
-    if (isAnonymousUser(user)) {
+    if (isAnonymousUser(user) || user.isExternal()) {
-      throw new ChangePasswordNotAllowedException(ContextEntry.ContextBuilder.entity("PasswordChange", "-").in(User.class, user.getName()), user.getType());
+      throw new ChangePasswordNotAllowedException(ContextEntry.ContextBuilder.entity("PasswordChange", "-").in(User.class, user.getName()), "external");
     }
     user.setPassword(newPassword);
     this.modify(user);

scm-webapp/src/test/java/sonia/scm/user/DefaultUserManagerTest.java

@@ -21,66 +21,52 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  * SOFTWARE.
  */
-    
-package sonia.scm.user;
 
-//~--- non-JDK imports --------------------------------------------------------
+package sonia.scm.user;
 
 import com.github.sdorra.shiro.ShiroRule;
 import com.github.sdorra.shiro.SubjectAware;
-import com.google.common.collect.Lists;
-
 import org.assertj.core.api.Assertions;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
-
 import org.mockito.ArgumentCaptor;
 import sonia.scm.NotFoundException;
 import sonia.scm.store.JAXBConfigurationStoreFactory;
 import sonia.scm.user.xml.XmlUserDAO;
 
-import static org.mockito.Mockito.*;
+import static org.mockito.Mockito.doNothing;
-
+import static org.mockito.Mockito.mock;
-//~--- JDK imports ------------------------------------------------------------
+import static org.mockito.Mockito.when;
-
-import java.util.Collections;
-import java.util.List;
-import org.junit.Rule;
 
 /**
- *
  * @author Sebastian Sdorra
  */
 @SubjectAware(
-    username = "trillian",
+  username = "trillian",
-    password = "secret",
+  password = "secret",
-    configuration = "classpath:sonia/scm/repository/shiro.ini"
+  configuration = "classpath:sonia/scm/repository/shiro.ini"
 )
-public class DefaultUserManagerTest extends UserManagerTestBase
+public class DefaultUserManagerTest extends UserManagerTestBase {
-{
 
   @Rule
   public ShiroRule shiro = new ShiroRule();
 
-
+  private UserDAO userDAO;
-  private UserDAO userDAO ;
-  private User trillian;
 
   /**
    * Method description
    *
-   *
    * @return
    */
   @Override
-  public UserManager createManager()
+  public UserManager createManager() {
-  {
     return new DefaultUserManager(createXmlUserDAO());
   }
 
   @Before
   public void initDao() {
-    trillian = UserTestData.createTrillian();
+    User trillian = UserTestData.createTrillian();
     trillian.setPassword("oldEncrypted");
 
     userDAO = mock(UserDAO.class);
@@ -115,6 +101,16 @@ public class DefaultUserManagerTest extends UserManagerTestBase
     userManager.overwritePassword("notExisting", "---");
   }
 
+  @Test(expected = ChangePasswordNotAllowedException.class)
+  public void shouldFailOverwritePasswordForExternalUser() {
+    User trillian = new User("trillian");
+    trillian.setExternal(true);
+    when(userDAO.get("trillian")).thenReturn(trillian);
+    UserManager userManager = new DefaultUserManager(userDAO);
+
+    userManager.overwritePassword("trillian", "---");
+  }
+
   @Test
   public void shouldSucceedOverwritePassword() {
     ArgumentCaptor<User> userCaptor = ArgumentCaptor.forClass(User.class);