Let login fail if user name cannot be set
When a user logs in for the first time in Gerrit a
new account is created for this user. Depending on
the authentication type a user name might be
included in the authentication request. This user
name is then set for the newly created account.
If setting this user name fails (e.g. because it
is already taken) the login still succeeded and the
error was just logged. This makes sense if the user
is allowed to manually set a unique user name
later. However in case the realm does not allow
the user to set a user name it results in an
account for which no user name is set and for
which also no user name can be set. Such an
account is not fully functional. This is why in
this case it is better to let the login fail and
prevent the creation of such an account.
This fix is especially important if e.g. LDAP is
used for authentication and the user verification
in the LDAP system is case insensitive. E.g. it
could be that a user 'Guest' exists in the LDAP
system and the user logs in with 'Guest' into
Gerrit which will create a new account in Gerrit
with 'Guest' as Gerrit ID and 'Guest' as external
user name. If now the user logs in into Gerrit
with 'guest' Gerrit was creating a new account
with 'guest' as Gerrit ID but the setting of
'Guest' as external user name for this account
failed because it was already used for the account
with the Gerrit ID 'Guest'. So the user ended up
with two accounts in Gerrit from which one was
not usable due to the missing user name. Obviously
this can cause quite some confusion (e.g.
privileges are assigned to one account, user logs
in with the other account and wonders why his
privileges are "lost").
Signed-off-by: Edwin Kempin <edwin.kempin@gmail.com>
Change-Id: Id03036119c7f9a24b50addfe410018a45762e1c3
diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/auth/userpass/UserPassSignInDialog.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/auth/userpass/UserPassSignInDialog.java
index 6dd0cd5..c4709a5 100644
--- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/auth/userpass/UserPassSignInDialog.java
+++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/auth/userpass/UserPassSignInDialog.java
@@ -227,7 +227,7 @@
@Override
public void onFailure(final Throwable caught) {
super.onFailure(caught);
- enable(false);
+ enable(true);
}
});
}
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/UserPassAuthServiceImpl.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/UserPassAuthServiceImpl.java
index a59d013..e4577c1 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/UserPassAuthServiceImpl.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/UserPassAuthServiceImpl.java
@@ -19,6 +19,7 @@
import com.google.gerrit.httpd.WebSession;
import com.google.gerrit.server.account.AccountException;
import com.google.gerrit.server.account.AccountManager;
+import com.google.gerrit.server.account.AccountUserNameException;
import com.google.gerrit.server.account.AuthRequest;
import com.google.gerrit.server.account.AuthResult;
import com.google.gwt.user.client.rpc.AsyncCallback;
@@ -55,6 +56,12 @@
final AuthResult res;
try {
res = accountManager.authenticate(req);
+ } catch (AccountUserNameException e) {
+ // entered user name and password were correct, but user name could not be
+ // set for the newly created account and this is why the login fails,
+ // error screen with error message should be shown to the user
+ callback.onFailure(e);
+ return;
} catch (AccountException e) {
result.success = false;
callback.onSuccess(result);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java
index 83a5eed..5cb8f36 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountManager.java
@@ -290,13 +290,18 @@
try {
changeUserNameFactory.create(db, user, who.getUserName()).call();
} catch (NameAlreadyUsedException e) {
- log.error("Cannot assign user name \"" + who.getUserName()
- + "\" to account " + newId + "; name already in use.");
+ final String message =
+ "Cannot assign user name \"" + who.getUserName() + "\" to account "
+ + newId + "; name already in use.";
+ handleSettingUserNameFailure(db, account, extId, message, e, false);
} catch (InvalidUserNameException e) {
- log.error("Cannot assign user name \"" + who.getUserName()
- + "\" to account " + newId + "; name does not conform.");
+ final String message =
+ "Cannot assign user name \"" + who.getUserName() + "\" to account "
+ + newId + "; name does not conform.";
+ handleSettingUserNameFailure(db, account, extId, message, e, false);
} catch (OrmException e) {
- log.error("Cannot assign user name", e);
+ final String message = "Cannot assign user name";
+ handleSettingUserNameFailure(db, account, extId, message, e, true);
}
}
@@ -305,6 +310,49 @@
return new AuthResult(newId, extId.getKey(), true);
}
+ /**
+ * This method handles an exception that occurred during the setting of the
+ * user name for a newly created account. If the realm does not allow the user
+ * to set a user name manually this method deletes the newly created account
+ * and throws an {@link AccountUserNameException}. In any case the error
+ * message is logged.
+ *
+ * @param db the database
+ * @param account the newly created account
+ * @param extId the newly created external id
+ * @param errorMessage the error message
+ * @param e the exception that occurred during the setting of the user name
+ * for the new account
+ * @param logException flag that decides whether the exception should be
+ * included into the log
+ * @throws AccountUserNameException thrown if the realm does not allow the
+ * user to manually set the user name
+ * @throws OrmException thrown if cleaning the database failed
+ */
+ private void handleSettingUserNameFailure(final ReviewDb db,
+ final Account account, final AccountExternalId extId,
+ final String errorMessage, final Exception e, final boolean logException)
+ throws AccountUserNameException, OrmException {
+ if (logException) {
+ log.error(errorMessage, e);
+ } else {
+ log.error(errorMessage);
+ }
+ if (!realm.allowsEdit(Account.FieldName.USER_NAME)) {
+ // setting the given user name has failed, but the realm does not
+ // allow the user to manually set a user name,
+ // this means we would end with an account without user name
+ // (without 'username:<USERNAME>' entry in
+ // account_external_ids table),
+ // such an account cannot be used for uploading changes,
+ // this is why the best we can do here is to fail early and cleanup
+ // the database
+ db.accounts().delete(Collections.singleton(account));
+ db.accountExternalIds().delete(Collections.singleton(extId));
+ throw new AccountUserNameException(errorMessage, e);
+ }
+ }
+
private static AccountExternalId createId(final Account.Id newId,
final AuthRequest who) {
final String ext = who.getExternalId();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountUserNameException.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountUserNameException.java
new file mode 100644
index 0000000..1cf8be8
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountUserNameException.java
@@ -0,0 +1,28 @@
+// Copyright (C) 2010 The Android Open Source Project
+//
+// 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.google.gerrit.server.account;
+
+/**
+ * Thrown by {@link AccountManager} if the user name for a newly created account
+ * could not be set and the realm does not allow the user to set a user name
+ * manually.
+ */
+public class AccountUserNameException extends AccountException {
+ private static final long serialVersionUID = 1L;
+
+ public AccountUserNameException(final String message, final Throwable why) {
+ super(message, why);
+ }
+}
