From 9aa11943f821cb6c10a6d1c41c3d2381676f5047 Mon Sep 17 00:00:00 2001 From: James Moger Date: Tue, 4 Mar 2014 17:29:02 -0500 Subject: [PATCH] Implement user "disabled" flag as an alternative to deleting the account --- releases.moxie | 1 + .../java/com/gitblit/ConfigUserService.java | 6 ++ .../com/gitblit/client/EditUserDialog.java | 5 ++ .../manager/AuthenticationManager.java | 37 ++++++++--- .../java/com/gitblit/models/UserModel.java | 1 + .../gitblit/wicket/GitBlitWebApp.properties | 4 +- .../gitblit/wicket/pages/EditUserPage.html | 7 +- .../gitblit/wicket/pages/EditUserPage.java | 2 + .../com/gitblit/wicket/pages/SessionPage.java | 10 +++ .../com/gitblit/wicket/panels/UsersPanel.java | 7 +- src/main/resources/gitblit.css | 7 +- .../tests/AuthenticationManagerTest.java | 66 +++++++++++++++++++ .../java/com/gitblit/tests/GitBlitSuite.java | 2 +- 13 files changed, 138 insertions(+), 17 deletions(-) create mode 100644 src/test/java/com/gitblit/tests/AuthenticationManagerTest.java diff --git a/releases.moxie b/releases.moxie index 0f4a75ec..28a62350 100644 --- a/releases.moxie +++ b/releases.moxie @@ -78,6 +78,7 @@ r20: { - Added FishEye hook script (pr-137) - Added Redmine Fetch hook script (issue-359) - Added Subgit hook contributed by TMate Software + - Added function to retain a user account but prohibit authentication. This is an alternative to deleting a user account. dependencyChanges: - updated to Jetty 8.1.13 - updated to JGit 3.2.0 diff --git a/src/main/java/com/gitblit/ConfigUserService.java b/src/main/java/com/gitblit/ConfigUserService.java index a2a3277e..9b4dd7f1 100644 --- a/src/main/java/com/gitblit/ConfigUserService.java +++ b/src/main/java/com/gitblit/ConfigUserService.java @@ -98,6 +98,8 @@ public class ConfigUserService implements IUserService { private static final String ACCOUNTTYPE = "accountType"; + private static final String DISABLED = "disabled"; + private final File realmFile; private final Logger logger = LoggerFactory.getLogger(ConfigUserService.class); @@ -701,6 +703,9 @@ public class ConfigUserService implements IUserService { if (!StringUtils.isEmpty(model.countryCode)) { config.setString(USER, model.username, COUNTRYCODE, model.countryCode); } + if (model.disabled) { + config.setBoolean(USER, model.username, DISABLED, true); + } if (model.getPreferences() != null) { if (!StringUtils.isEmpty(model.getPreferences().locale)) { config.setString(USER, model.username, LOCALE, model.getPreferences().locale); @@ -868,6 +873,7 @@ public class ConfigUserService implements IUserService { if (Constants.EXTERNAL_ACCOUNT.equals(user.password) && user.accountType.isLocal()) { user.accountType = AccountType.EXTERNAL; } + user.disabled = config.getBoolean(USER, username, DISABLED, false); user.organizationalUnit = config.getString(USER, username, ORGANIZATIONALUNIT); user.organization = config.getString(USER, username, ORGANIZATION); user.locality = config.getString(USER, username, LOCALITY); diff --git a/src/main/java/com/gitblit/client/EditUserDialog.java b/src/main/java/com/gitblit/client/EditUserDialog.java index ab3ea67e..676916b2 100644 --- a/src/main/java/com/gitblit/client/EditUserDialog.java +++ b/src/main/java/com/gitblit/client/EditUserDialog.java @@ -92,6 +92,8 @@ public class EditUserDialog extends JDialog { private JCheckBox notFederatedCheckbox; + private JCheckBox disabledCheckbox; + private JTextField organizationalUnitField; private JTextField organizationField; @@ -153,6 +155,7 @@ public class EditUserDialog extends JDialog { notFederatedCheckbox = new JCheckBox( Translation.get("gb.excludeFromFederationDescription"), anUser.excludeFromFederation); + disabledCheckbox = new JCheckBox(Translation.get("gb.disableUserDescription"), anUser.disabled); organizationalUnitField = new JTextField(anUser.organizationalUnit == null ? "" : anUser.organizationalUnit, 25); organizationField = new JTextField(anUser.organization == null ? "" : anUser.organization, 25); @@ -176,6 +179,7 @@ public class EditUserDialog extends JDialog { fieldsPanel.add(newFieldPanel(Translation.get("gb.canCreate"), canCreateCheckbox)); fieldsPanel.add(newFieldPanel(Translation.get("gb.excludeFromFederation"), notFederatedCheckbox)); + fieldsPanel.add(newFieldPanel(Translation.get("gb.disableUser"), disabledCheckbox)); JPanel attributesPanel = new JPanel(new GridLayout(0, 1, 5, 2)); attributesPanel.add(newFieldPanel(Translation.get("gb.organizationalUnit") + " (OU)", organizationalUnitField)); @@ -355,6 +359,7 @@ public class EditUserDialog extends JDialog { user.canFork = canForkCheckbox.isSelected(); user.canCreate = canCreateCheckbox.isSelected(); user.excludeFromFederation = notFederatedCheckbox.isSelected(); + user.disabled = disabledCheckbox.isSelected(); user.organizationalUnit = organizationalUnitField.getText().trim(); user.organization = organizationField.getText().trim(); diff --git a/src/main/java/com/gitblit/manager/AuthenticationManager.java b/src/main/java/com/gitblit/manager/AuthenticationManager.java index 48975142..ad4a9851 100644 --- a/src/main/java/com/gitblit/manager/AuthenticationManager.java +++ b/src/main/java/com/gitblit/manager/AuthenticationManager.java @@ -198,7 +198,7 @@ public class AuthenticationManager implements IAuthenticationManager { flagWicketSession(AuthenticationType.CONTAINER); logger.debug(MessageFormat.format("{0} authenticated by servlet container principal from {1}", user.username, httpRequest.getRemoteAddr())); - return user; + return validateAuthentication(user, AuthenticationType.CONTAINER); } else if (settings.getBoolean(Keys.realm.container.autoCreateAccounts, false) && !internalAccount) { // auto-create user from an authenticated container principal @@ -210,7 +210,7 @@ public class AuthenticationManager implements IAuthenticationManager { flagWicketSession(AuthenticationType.CONTAINER); logger.debug(MessageFormat.format("{0} authenticated and created by servlet container principal from {1}", user.username, httpRequest.getRemoteAddr())); - return user; + return validateAuthentication(user, AuthenticationType.CONTAINER); } else if (!internalAccount) { logger.warn(MessageFormat.format("Failed to find UserModel for {0}, attempted servlet container authentication from {1}", principal.getName(), httpRequest.getRemoteAddr())); @@ -231,7 +231,7 @@ public class AuthenticationManager implements IAuthenticationManager { flagWicketSession(AuthenticationType.CERTIFICATE); logger.debug(MessageFormat.format("{0} authenticated by client certificate {1} from {2}", user.username, metadata.serialNumber, httpRequest.getRemoteAddr())); - return user; + return validateAuthentication(user, AuthenticationType.CERTIFICATE); } else { logger.warn(MessageFormat.format("Failed to find UserModel for {0}, attempted client certificate ({1}) authentication from {2}", model.username, metadata.serialNumber, httpRequest.getRemoteAddr())); @@ -253,7 +253,7 @@ public class AuthenticationManager implements IAuthenticationManager { flagWicketSession(AuthenticationType.COOKIE); logger.debug(MessageFormat.format("{0} authenticated by cookie from {1}", user.username, httpRequest.getRemoteAddr())); - return user; + return validateAuthentication(user, AuthenticationType.COOKIE); } } @@ -275,7 +275,7 @@ public class AuthenticationManager implements IAuthenticationManager { flagWicketSession(AuthenticationType.CREDENTIALS); logger.debug(MessageFormat.format("{0} authenticated by BASIC request header from {1}", user.username, httpRequest.getRemoteAddr())); - return user; + return validateAuthentication(user, AuthenticationType.CREDENTIALS); } else { logger.warn(MessageFormat.format("Failed login attempt for {0}, invalid credentials from {1}", username, httpRequest.getRemoteAddr())); @@ -285,6 +285,27 @@ public class AuthenticationManager implements IAuthenticationManager { return null; } + /** + * This method allows the authentication manager to reject authentication + * attempts. It is called after the username/secret have been verified to + * ensure that the authentication technique has been logged. + * + * @param user + * @return + */ + protected UserModel validateAuthentication(UserModel user, AuthenticationType type) { + if (user == null) { + return null; + } + if (user.disabled) { + // user has been disabled + logger.warn("Rejected {} authentication attempt by disabled account \"{}\"", + type, user.username); + return null; + } + return user; + } + protected void flagWicketSession(AuthenticationType authenticationType) { RequestCycle requestCycle = RequestCycle.get(); if (requestCycle != null) { @@ -338,7 +359,7 @@ public class AuthenticationManager implements IAuthenticationManager { // plain-text password returnedUser = user; } - return returnedUser; + return validateAuthentication(returnedUser, AuthenticationType.CREDENTIALS); } // try registered external authentication providers @@ -349,12 +370,12 @@ public class AuthenticationManager implements IAuthenticationManager { if (user != null) { // user authenticated user.accountType = provider.getAccountType(); - return user; + return validateAuthentication(user, AuthenticationType.CREDENTIALS); } } } } - return user; + return validateAuthentication(user, AuthenticationType.CREDENTIALS); } /** diff --git a/src/main/java/com/gitblit/models/UserModel.java b/src/main/java/com/gitblit/models/UserModel.java index 63208f35..fbf311a5 100644 --- a/src/main/java/com/gitblit/models/UserModel.java +++ b/src/main/java/com/gitblit/models/UserModel.java @@ -67,6 +67,7 @@ public class UserModel implements Principal, Serializable, Comparable public boolean canFork; public boolean canCreate; public boolean excludeFromFederation; + public boolean disabled; // retained for backwards-compatibility with RPC clients @Deprecated public final Set repositories = new HashSet(); diff --git a/src/main/java/com/gitblit/wicket/GitBlitWebApp.properties b/src/main/java/com/gitblit/wicket/GitBlitWebApp.properties index 86dd585f..49a57e9c 100644 --- a/src/main/java/com/gitblit/wicket/GitBlitWebApp.properties +++ b/src/main/java/com/gitblit/wicket/GitBlitWebApp.properties @@ -648,4 +648,6 @@ gb.looksGood = looks good gb.approve = approve gb.hasNotReviewed = has not reviewed gb.about = about -gb.ticketN = ticket #{0} \ No newline at end of file +gb.ticketN = ticket #{0} +gb.disableUser = disable user +gb.disableUserDescription = prevent this account from authenticating \ No newline at end of file diff --git a/src/main/java/com/gitblit/wicket/pages/EditUserPage.html b/src/main/java/com/gitblit/wicket/pages/EditUserPage.html index e78e44ee..3bccdd59 100644 --- a/src/main/java/com/gitblit/wicket/pages/EditUserPage.html +++ b/src/main/java/com/gitblit/wicket/pages/EditUserPage.html @@ -31,10 +31,11 @@ - - - + + + + diff --git a/src/main/java/com/gitblit/wicket/pages/EditUserPage.java b/src/main/java/com/gitblit/wicket/pages/EditUserPage.java index 15c35fa6..b9a84805 100644 --- a/src/main/java/com/gitblit/wicket/pages/EditUserPage.java +++ b/src/main/java/com/gitblit/wicket/pages/EditUserPage.java @@ -243,6 +243,8 @@ public class EditUserPage extends RootSubPage { form.add(new CheckBox("canFork").setEnabled(app().settings().getBoolean(Keys.web.allowForking, true))); form.add(new CheckBox("canCreate")); form.add(new CheckBox("excludeFromFederation")); + form.add(new CheckBox("disabled")); + form.add(new RegistrantPermissionsPanel("repositories", RegistrantType.REPOSITORY, repos, permissions, getAccessPermissions())); form.add(teams.setEnabled(editTeams)); diff --git a/src/main/java/com/gitblit/wicket/pages/SessionPage.java b/src/main/java/com/gitblit/wicket/pages/SessionPage.java index 8065c5aa..909342ae 100644 --- a/src/main/java/com/gitblit/wicket/pages/SessionPage.java +++ b/src/main/java/com/gitblit/wicket/pages/SessionPage.java @@ -56,6 +56,16 @@ public abstract class SessionPage extends WebPage { // any changes to permissions or roles (issue-186) UserModel user = app().users().getUserModel(session.getUser().username); + if (user.disabled) { + // user was disabled during session + HttpServletResponse response = ((WebResponse) getRequestCycle().getResponse()) + .getHttpServletResponse(); + app().authentication().logout(response, user); + session.setUser(null); + session.invalidateNow(); + return; + } + // validate cookie during session (issue-361) if (user != null && app().settings().getBoolean(Keys.web.allowCookieAuthentication, true)) { HttpServletRequest request = ((WebRequest) getRequestCycle().getRequest()) diff --git a/src/main/java/com/gitblit/wicket/panels/UsersPanel.java b/src/main/java/com/gitblit/wicket/panels/UsersPanel.java index ed990c89..5d626552 100644 --- a/src/main/java/com/gitblit/wicket/panels/UsersPanel.java +++ b/src/main/java/com/gitblit/wicket/panels/UsersPanel.java @@ -57,7 +57,8 @@ public class UsersPanel extends BasePanel { @Override public void populateItem(final Item item) { final UserModel entry = item.getModelObject(); - LinkPanel editLink = new LinkPanel("username", "list", entry.username, + String css = "list" + (entry.disabled ? "-strikethrough":""); + LinkPanel editLink = new LinkPanel("username", css, entry.username, EditUserPage.class, WicketUtils.newUsernameParameter(entry.username)); WicketUtils.setHtmlTooltip(editLink, getString("gb.edit") + " " + entry.getDisplayName()); item.add(editLink); @@ -65,7 +66,7 @@ public class UsersPanel extends BasePanel { if (StringUtils.isEmpty(entry.displayName)) { item.add(new Label("displayName").setVisible(false)); } else { - editLink = new LinkPanel("displayName", "list", entry.getDisplayName(), + editLink = new LinkPanel("displayName", css, entry.getDisplayName(), EditUserPage.class, WicketUtils.newUsernameParameter(entry.username)); WicketUtils.setHtmlTooltip(editLink, getString("gb.edit") + " " + entry.getDisplayName()); item.add(editLink); @@ -74,7 +75,7 @@ public class UsersPanel extends BasePanel { if (StringUtils.isEmpty(entry.emailAddress)) { item.add(new Label("emailAddress").setVisible(false)); } else { - editLink = new LinkPanel("emailAddress", "list", entry.emailAddress, + editLink = new LinkPanel("emailAddress", css, entry.emailAddress, EditUserPage.class, WicketUtils.newUsernameParameter(entry.username)); WicketUtils.setHtmlTooltip(editLink, getString("gb.edit") + " " + entry.getDisplayName()); item.add(editLink); diff --git a/src/main/resources/gitblit.css b/src/main/resources/gitblit.css index 78947709..748a3198 100644 --- a/src/main/resources/gitblit.css +++ b/src/main/resources/gitblit.css @@ -55,7 +55,7 @@ a.bugtraq { white-space: nowrap; vertical-align: baseline; } - + [class^="icon-"], [class*=" icon-"] i { /* override for a links that look like bootstrap buttons */ vertical-align: text-bottom; @@ -993,6 +993,11 @@ a.list { color: inherit; } +a.list-strikethrough { + text-decoration: line-through; + color: inherit; +} + a.list.subject { font-weight: bold; } diff --git a/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java b/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java new file mode 100644 index 00000000..84a2b749 --- /dev/null +++ b/src/test/java/com/gitblit/tests/AuthenticationManagerTest.java @@ -0,0 +1,66 @@ +/* + * Copyright 2013 gitblit.com. + * + * Licensed 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 com.gitblit.tests; + +import java.util.HashMap; + +import org.junit.Test; + +import com.gitblit.manager.AuthenticationManager; +import com.gitblit.manager.IAuthenticationManager; +import com.gitblit.manager.IUserManager; +import com.gitblit.manager.RuntimeManager; +import com.gitblit.manager.UserManager; +import com.gitblit.models.UserModel; +import com.gitblit.tests.mock.MemorySettings; + +/** + * Class for testing local authentication. + * + * @author James Moger + * + */ +public class AuthenticationManagerTest extends GitblitUnitTest { + + IUserManager users; + + MemorySettings getSettings() { + return new MemorySettings(new HashMap()); + } + + IAuthenticationManager newAuthenticationManager() { + RuntimeManager runtime = new RuntimeManager(getSettings(), GitBlitSuite.BASEFOLDER).start(); + users = new UserManager(runtime).start(); + AuthenticationManager auth = new AuthenticationManager(runtime, users).start(); + return auth; + } + + @Test + public void testAuthenticate() throws Exception { + IAuthenticationManager auth = newAuthenticationManager(); + + UserModel user = new UserModel("sunnyjim"); + user.password = "password"; + users.updateUserModel(user); + + assertNotNull(auth.authenticate(user.username, user.password.toCharArray())); + user.disabled = true; + + users.updateUserModel(user); + assertNull(auth.authenticate(user.username, user.password.toCharArray())); + users.deleteUserModel(user); + } +} diff --git a/src/test/java/com/gitblit/tests/GitBlitSuite.java b/src/test/java/com/gitblit/tests/GitBlitSuite.java index cba575d7..c015c847 100644 --- a/src/test/java/com/gitblit/tests/GitBlitSuite.java +++ b/src/test/java/com/gitblit/tests/GitBlitSuite.java @@ -64,7 +64,7 @@ import com.gitblit.utils.JGitUtils; GroovyScriptTest.class, LuceneExecutorTest.class, RepositoryModelTest.class, FanoutServiceTest.class, Issue0259Test.class, Issue0271Test.class, HtpasswdAuthenticationTest.class, ModelUtilsTest.class, JnaUtilsTest.class, LdapSyncServiceTest.class, FileTicketServiceTest.class, - BranchTicketServiceTest.class, RedisTicketServiceTest.class }) + BranchTicketServiceTest.class, RedisTicketServiceTest.class, AuthenticationManagerTest.class }) public class GitBlitSuite { public static final File BASEFOLDER = new File("data"); -- 2.39.5