Merge changes from topic "return-optional-from-currentuser"

* changes:
  IdentifiedUser#getLoggableName: Use preferred email if available
  Never return null from ReceiveCommits#displayName
  com.google.gerrit.server.account.AuthRequest#getUserName: Return Optional
  com.google.gerrit.server.auth.AuthRequest: Let methods return Optional
  Use identifiedUser.getUserName() instead of identifiedUser.state().getUserName()
  CurrentUser#getUserName: Return Optional<String> instead of nullable String
This commit is contained in:
Edwin Kempin
2018-01-26 14:58:46 +00:00
committed by Gerrit Code Review
34 changed files with 120 additions and 126 deletions

View File

@@ -15,7 +15,6 @@
package com.google.gerrit.httpd;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -65,12 +64,7 @@ public class GetUserFilter implements Filter {
throws IOException, ServletException {
CurrentUser user = userProvider.get();
if (user != null && user.isIdentifiedUser()) {
IdentifiedUser who = user.asIdentifiedUser();
if (who.getUserName() != null) {
req.setAttribute(REQ_ATTR_KEY, who.getUserName());
} else {
req.setAttribute(REQ_ATTR_KEY, "a/" + who.getAccountId());
}
req.setAttribute(REQ_ATTR_KEY, user.asIdentifiedUser().getLoggableName());
}
chain.doFilter(req, resp);
}

View File

@@ -72,6 +72,7 @@ import java.util.Enumeration;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import javax.servlet.http.HttpServlet;
import javax.servlet.http.HttpServletRequest;
@@ -573,13 +574,9 @@ class GitwebServlet extends HttpServlet {
String remoteUser = null;
if (userProvider.get().isIdentifiedUser()) {
IdentifiedUser u = userProvider.get().asIdentifiedUser();
String user = u.getUserName();
env.set("GERRIT_USER_NAME", user);
if (user != null) {
remoteUser = user;
} else {
remoteUser = "account-" + u.getAccountId();
}
Optional<String> user = u.getUserName();
env.set("GERRIT_USER_NAME", user.orElse(null));
remoteUser = user.orElseGet(() -> "account-" + u.getAccountId());
}
env.set("REMOTE_USER", remoteUser);

View File

@@ -30,6 +30,7 @@ import com.google.inject.Provider;
import com.google.inject.Singleton;
import com.google.inject.servlet.ServletModule;
import java.io.IOException;
import java.util.Optional;
import java.util.concurrent.Future;
import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.regex.Matcher;
@@ -225,9 +226,9 @@ public class ProjectQoSFilter implements Filter {
CurrentUser who = user.get();
if (who.isIdentifiedUser()) {
String name = who.asIdentifiedUser().getUserName();
if (name != null) {
userName = " (" + name + ")";
Optional<String> name = who.asIdentifiedUser().getUserName();
if (name.isPresent()) {
userName = " (" + name.get() + ")";
}
}

View File

@@ -19,6 +19,7 @@ import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.account.GroupMembership;
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.inject.servlet.RequestScoped;
import java.util.Optional;
import java.util.function.Consumer;
/**
@@ -90,8 +91,8 @@ public abstract class CurrentUser {
public abstract GroupMembership getEffectiveGroups();
/** Unique name of the user on this server, if one has been assigned. */
public String getUserName() {
return null;
public Optional<String> getUserName() {
return Optional.empty();
}
/** Check if user is the IdentifiedUser */

View File

@@ -14,6 +14,8 @@
package com.google.gerrit.server;
import static com.google.common.base.MoreObjects.firstNonNull;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
@@ -44,6 +46,7 @@ import java.net.URL;
import java.util.Date;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.TimeZone;
import org.eclipse.jgit.lib.PersonIdent;
@@ -296,14 +299,15 @@ public class IdentifiedUser extends CurrentUser {
* empty.
*/
@Override
@Nullable
public String getUserName() {
return state().getUserName().orElse(null);
public Optional<String> getUserName() {
return state().getUserName();
}
/** @return unique name of the user for logging, never {@code null} */
public String getLoggableName() {
return getUserName() != null ? getUserName() : "a/" + getAccountId().get();
return getUserName()
.orElseGet(
() -> firstNonNull(getAccount().getPreferredEmail(), "a/" + getAccountId().get()));
}
public Account getAccount() {
@@ -368,12 +372,7 @@ public class IdentifiedUser extends CurrentUser {
name = anonymousCowardName;
}
String user = getUserName();
if (user == null) {
user = "";
}
user = user + "|account-" + ua.getId().toString();
String user = getUserName().orElse("") + "|account-" + ua.getId().toString();
return new PersonIdent(name, user + "@" + guessHost(), when, tz);
}
@@ -387,10 +386,7 @@ public class IdentifiedUser extends CurrentUser {
// don't leak an address the user may have given us, but doesn't
// necessarily want to publish through Git records.
//
String user = getUserName();
if (user == null) {
user = "account-" + ua.getId().toString();
}
String user = getUserName().orElseGet(() -> "account-" + ua.getId().toString());
String host;
if (canonicalUrl.get() != null) {

View File

@@ -16,6 +16,7 @@ package com.google.gerrit.server;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.util.Optional;
/** User identity for plugin code that needs an identity. */
public class PluginUser extends InternalUser {
@@ -31,8 +32,8 @@ public class PluginUser extends InternalUser {
}
@Override
public String getUserName() {
return "plugin " + pluginName;
public Optional<String> getUserName() {
return Optional.of("plugin " + pluginName);
}
@Override

View File

@@ -187,7 +187,12 @@ public class AccountManager {
}
setInactiveFlag.deactivate(id.accountId());
} catch (Exception e) {
log.error("Unable to deactivate account " + authRequest.getUserName(), e);
log.error(
"Unable to deactivate account "
+ authRequest
.getUserName()
.orElse(" for external ID key " + authRequest.getExternalIdKey().get()),
e);
}
}
@@ -246,13 +251,15 @@ public class AccountManager {
}
if (!realm.allowsEdit(AccountFieldName.USER_NAME)
&& !Strings.isNullOrEmpty(who.getUserName())
&& who.getUserName().isPresent()
&& !who.getUserName().equals(user.getUserName())) {
if (user.getUserName() != null) {
if (user.getUserName().isPresent()) {
log.warn(
"Not changing already set username {} to {}", user.getUserName(), who.getUserName());
"Not changing already set username {} to {}",
user.getUserName().get(),
who.getUserName().get());
} else {
log.warn("Not setting username to {}", who.getUserName());
log.warn("Not setting username to {}", who.getUserName().get());
}
}
@@ -275,7 +282,7 @@ public class AccountManager {
ExternalId extId =
ExternalId.createWithEmail(who.getExternalIdKey(), newId, who.getEmailAddress());
ExternalId userNameExtId =
!Strings.isNullOrEmpty(who.getUserName()) ? createUsername(newId, who.getUserName()) : null;
who.getUserName().isPresent() ? createUsername(newId, who.getUserName().get()) : null;
boolean isFirstAccount = awaitsFirstAccountCheck.getAndSet(false) && !accounts.hasAnyAccount();
@@ -310,7 +317,7 @@ public class AccountManager {
}
if (userNameExtId != null) {
sshKeyCache.evict(who.getUserName());
who.getUserName().ifPresent(sshKeyCache::evict);
}
IdentifiedUser user = userFactory.create(newId);

View File

@@ -203,7 +203,7 @@ public class AccountState {
return userName;
}
public boolean checkPassword(String password, String username) {
public boolean checkPassword(@Nullable String password, String username) {
if (password == null) {
return false;
}

View File

@@ -18,7 +18,10 @@ import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_EXT
import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_GERRIT;
import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_MAILTO;
import com.google.common.base.Strings;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.server.account.externalids.ExternalId;
import java.util.Optional;
/**
* Information for {@link AccountManager#authenticate(AuthRequest)}.
@@ -59,7 +62,7 @@ public class AuthRequest {
private String password;
private String displayName;
private String emailAddress;
private String userName;
private Optional<String> userName;
private boolean skipAuthentication;
private String authPlugin;
private String authProvider;
@@ -111,12 +114,12 @@ public class AuthRequest {
emailAddress = email != null && email.length() > 0 ? email : null;
}
public String getUserName() {
public Optional<String> getUserName() {
return userName;
}
public void setUserName(String user) {
userName = user;
public void setUserName(@Nullable String user) {
userName = Optional.ofNullable(Strings.emptyToNull(user));
}
public boolean isSkipAuthentication() {

View File

@@ -14,16 +14,18 @@
package com.google.gerrit.server.auth;
import com.google.common.base.Strings;
import com.google.gerrit.common.Nullable;
import java.util.Optional;
/** Defines an abstract request for user authentication to Gerrit. */
public abstract class AuthRequest {
private final String username;
private final String password;
private final Optional<String> username;
private final Optional<String> password;
protected AuthRequest(String username, String password) {
this.username = username;
this.password = password;
protected AuthRequest(@Nullable String username, @Nullable String password) {
this.username = Optional.ofNullable(Strings.emptyToNull(username));
this.password = Optional.ofNullable(Strings.emptyToNull(password));
}
/**
@@ -31,8 +33,7 @@ public abstract class AuthRequest {
*
* @return username for authentication or null for anonymous access.
*/
@Nullable
public final String getUsername() {
public final Optional<String> getUsername() {
return username;
}
@@ -41,8 +42,7 @@ public abstract class AuthRequest {
*
* @return user's credentials or null
*/
@Nullable
public final String getPassword() {
public final Optional<String> getPassword() {
return password;
}
}

View File

@@ -14,7 +14,6 @@
package com.google.gerrit.server.auth;
import com.google.common.base.Strings;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.account.AccountState;
import com.google.gerrit.server.config.AuthConfig;
@@ -43,15 +42,15 @@ public class InternalAuthBackend implements AuthBackend {
public AuthUser authenticate(AuthRequest req)
throws MissingCredentialsException, InvalidCredentialsException, UnknownUserException,
UserNotAllowedException, AuthException {
if (Strings.isNullOrEmpty(req.getUsername()) || Strings.isNullOrEmpty(req.getPassword())) {
if (!req.getUsername().isPresent() || !req.getPassword().isPresent()) {
throw new MissingCredentialsException();
}
String username;
if (authConfig.isUserNameToLowerCase()) {
username = req.getUsername().toLowerCase(Locale.US);
username = req.getUsername().map(u -> u.toLowerCase(Locale.US)).get();
} else {
username = req.getUsername();
username = req.getUsername().get();
}
AccountState who =
@@ -64,7 +63,7 @@ public class InternalAuthBackend implements AuthBackend {
+ ": account inactive or not provisioned in Gerrit");
}
if (!who.checkPassword(req.getPassword(), username)) {
if (!who.checkPassword(req.getPassword().get(), username)) {
throw new InvalidCredentialsException();
}
return new AuthUser(AuthUser.UUID.create(username), username);

View File

@@ -60,16 +60,18 @@ public class LdapAuthBackend implements AuthBackend {
public AuthUser authenticate(AuthRequest req)
throws MissingCredentialsException, InvalidCredentialsException, UnknownUserException,
UserNotAllowedException, AuthException {
if (req.getUsername() == null || req.getPassword() == null) {
if (!req.getUsername().isPresent() || !req.getPassword().isPresent()) {
throw new MissingCredentialsException();
}
final String username =
lowerCaseUsername ? req.getUsername().toLowerCase(Locale.US) : req.getUsername();
String username =
lowerCaseUsername
? req.getUsername().map(u -> u.toLowerCase(Locale.US)).get()
: req.getUsername().get();
try {
final DirContext ctx;
if (authConfig.getAuthType() == AuthType.LDAP_BIND) {
ctx = helper.authenticate(username, req.getPassword());
ctx = helper.authenticate(username, req.getPassword().get());
} else {
ctx = helper.open();
}
@@ -81,7 +83,7 @@ public class LdapAuthBackend implements AuthBackend {
// We found the user account, but we need to verify
// the password matches it before we can continue.
//
helper.close(helper.authenticate(m.getDN(), req.getPassword()));
helper.close(helper.authenticate(m.getDN(), req.getPassword().get()));
}
return new AuthUser(AuthUser.UUID.create(username), username);
} finally {

View File

@@ -92,7 +92,7 @@ public class OAuthRealm extends AbstractRealm {
OAuthUserInfo userInfo;
try {
userInfo = loginProvider.login(who.getUserName(), who.getPassword());
userInfo = loginProvider.login(who.getUserName().orElse(null), who.getPassword());
} catch (IOException e) {
throw new AccountException("Cannot authenticate", e);
}
@@ -100,8 +100,7 @@ public class OAuthRealm extends AbstractRealm {
throw new AccountException("Cannot authenticate");
}
if (!Strings.isNullOrEmpty(userInfo.getEmailAddress())
&& (Strings.isNullOrEmpty(who.getUserName())
|| !allowsEdit(AccountFieldName.REGISTER_NEW_EMAIL))) {
&& (!who.getUserName().isPresent() || !allowsEdit(AccountFieldName.REGISTER_NEW_EMAIL))) {
who.setEmailAddress(userInfo.getEmailAddress());
}
if (!Strings.isNullOrEmpty(userInfo.getDisplayName())

View File

@@ -583,7 +583,7 @@ class ReceiveCommits {
for (ReceiveError error : errors.keySet()) {
rp.sendMessage(buildError(error, errors.get(error)));
}
rp.sendMessage(String.format("User: %s", displayName(user)));
rp.sendMessage(String.format("User: %s", user.getLoggableName()));
rp.sendMessage(COMMAND_REJECTION_MESSAGE_FOOTER);
}
@@ -811,14 +811,6 @@ class ReceiveCommits {
return sb.append(":\n").append(error.get()).toString();
}
private static String displayName(IdentifiedUser user) {
String displayName = user.getUserName();
if (displayName == null) {
displayName = user.getAccount().getPreferredEmail();
}
return displayName;
}
private void parseCommands(Collection<ReceiveCommand> commands)
throws PermissionBackendException, NoSuchProjectException, IOException {
List<String> optionList = rp.getPushOptions();

View File

@@ -394,7 +394,7 @@ public class CommitValidators {
return String.format(
" gitdir=$(git rev-parse --git-dir); scp -p -P %d %s@%s:hooks/commit-msg ${gitdir}/hooks/",
sshPort, user.getUserName(), sshHost);
sshPort, user.getUserName().orElse("<USERNAME>"), sshHost);
}
}

View File

@@ -28,6 +28,7 @@ import dk.brics.automaton.Automaton;
import java.util.HashMap;
import java.util.Map;
import java.util.regex.Pattern;
import java.util.stream.Stream;
public abstract class RefPatternMatcher {
public static RefPatternMatcher getMatcher(String pattern) {
@@ -133,16 +134,11 @@ public abstract class RefPatternMatcher {
}
private ImmutableSet<String> getUsernames(CurrentUser user) {
Stream<String> usernames = Streams.stream(user.getUserName());
if (user.isIdentifiedUser()) {
return Streams.concat(
user.asIdentifiedUser().getEmailAddresses().stream(),
ImmutableSet.of(user.getUserName()).stream())
.collect(toImmutableSet());
usernames = Streams.concat(usernames, user.asIdentifiedUser().getEmailAddresses().stream());
}
if (user.getUserName() != null) {
return ImmutableSet.of(user.getUserName());
}
return ImmutableSet.of();
return usernames.collect(toImmutableSet());
}
public boolean matchPrefix(String ref) {

View File

@@ -108,7 +108,7 @@ public class AddSshKey implements RestModifyView<AccountResource, SshKeyInput> {
"Cannot send SSH key added message to " + user.getAccount().getPreferredEmail(), e);
}
sshKeyCache.evict(user.getUserName());
user.getUserName().ifPresent(sshKeyCache::evict);
return Response.<SshKeyInfo>created(GetSshKeys.newSshKeyInfo(sshKey));
} catch (InvalidSshKeyException e) {
throw new BadRequestException(e.getMessage());

View File

@@ -62,7 +62,7 @@ public class DeleteSshKey implements RestModifyView<AccountResource.SshKey, Inpu
}
authorizedKeys.deleteKey(rsrc.getUser().getAccountId(), rsrc.getSshKey().getKey().get());
sshKeyCache.evict(rsrc.getUser().getUserName());
rsrc.getUser().getUserName().ifPresent(sshKeyCache::evict);
return Response.none();
}

View File

@@ -61,7 +61,7 @@ class GetOAuthToken implements RestReadView<AccountResource> {
throw new ResourceNotFoundException();
}
OAuthTokenInfo accessTokenInfo = new OAuthTokenInfo();
accessTokenInfo.username = rsrc.getUser().state().getUserName().orElse(null);
accessTokenInfo.username = rsrc.getUser().getUserName().orElse(null);
accessTokenInfo.resourceHost = getHostName(canonicalWebUrlProvider.get());
accessTokenInfo.accessToken = accessToken.getToken();
accessTokenInfo.providerId = accessToken.getProviderId();

View File

@@ -28,6 +28,6 @@ public class GetUsername implements RestReadView<AccountResource> {
@Override
public String apply(AccountResource rsrc) throws AuthException, ResourceNotFoundException {
return rsrc.getUser().state().getUserName().orElseThrow(ResourceNotFoundException::new);
return rsrc.getUser().getUserName().orElseThrow(ResourceNotFoundException::new);
}
}

View File

@@ -100,11 +100,9 @@ public class PutHttpPassword implements RestModifyView<AccountResource, HttpPass
public Response<String> apply(IdentifiedUser user, String newPassword)
throws ResourceNotFoundException, ResourceConflictException, OrmException, IOException,
ConfigInvalidException {
if (user.getUserName() == null) {
throw new ResourceConflictException("username must be set");
}
ExternalId extId = externalIds.get(ExternalId.Key.create(SCHEME_USERNAME, user.getUserName()));
String userName =
user.getUserName().orElseThrow(() -> new ResourceConflictException("username must be set"));
ExternalId extId = externalIds.get(ExternalId.Key.create(SCHEME_USERNAME, userName));
if (extId == null) {
throw new ResourceNotFoundException();
}

View File

@@ -135,6 +135,7 @@ import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.OptionalInt;
import java.util.Set;
import org.eclipse.jgit.errors.ConfigInvalidException;
@@ -333,9 +334,11 @@ public class PostReview
// User posting this review isn't currently in the reviewer or CC list,
// isn't being explicitly added, and isn't voting on any label.
// Automatically CC them on this change so they receive replies.
PostReviewers.Addition selfAddition =
Optional<PostReviewers.Addition> selfAddition =
postReviewers.ccCurrentUser(revision.getUser(), revision);
bu.addOp(revision.getChange().getId(), selfAddition.op);
if (selfAddition.isPresent()) {
bu.addOp(revision.getChange().getId(), selfAddition.get().op);
}
}
// Add WorkInProgressOp if requested.

View File

@@ -80,6 +80,7 @@ import java.io.IOException;
import java.text.MessageFormat;
import java.util.Collection;
import java.util.HashSet;
import java.util.Optional;
import java.util.Set;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.Config;
@@ -208,15 +209,18 @@ public class PostReviewers
return addByEmail(reviewer, rsrc, state, notify, accountsToNotify);
}
Addition ccCurrentUser(CurrentUser user, RevisionResource revision) {
return new Addition(
user.getUserName(),
revision.getChangeResource(),
ImmutableSet.of(user.getAccountId()),
null,
CC,
NotifyHandling.NONE,
ImmutableListMultimap.of());
Optional<Addition> ccCurrentUser(CurrentUser user, RevisionResource revision) {
return user.getUserName()
.map(
u ->
new Addition(
u,
revision.getChangeResource(),
ImmutableSet.of(user.getAccountId()),
null,
CC,
NotifyHandling.NONE,
ImmutableListMultimap.of()));
}
@Nullable

View File

@@ -320,7 +320,7 @@ public abstract class BaseCommand implements Command {
if (user.isIdentifiedUser()) {
final IdentifiedUser u = user.asIdentifiedUser();
m.append(" (user ");
m.append(u.state().getUserName().orElse(null));
m.append(u.getUserName().orElse(null));
m.append(" account ");
m.append(u.getAccountId());
m.append(")");
@@ -381,8 +381,8 @@ public abstract class BaseCommand implements Command {
m.append(getTaskDescription());
if (user.isIdentifiedUser()) {
IdentifiedUser u = user.asIdentifiedUser();
if (u.state().getUserName().isPresent()) {
m.append(" (").append(u.state().getUserName().get()).append(")");
if (u.getUserName().isPresent()) {
m.append(" (").append(u.getUserName().get()).append(")");
}
}
return m.toString();

View File

@@ -152,7 +152,7 @@ class NoShell implements Factory<Command> {
Account account = user.getAccount();
String name = account.getFullName();
if (name == null || name.isEmpty()) {
name = user.getUserName() != null ? user.getUserName() : anonymousCowardName;
name = user.getUserName().orElse(anonymousCowardName);
}
msg.append(" Hi ");
msg.append(name);
@@ -171,8 +171,8 @@ class NoShell implements Factory<Command> {
}
msg.append(" git clone ssh://");
if (user.getUserName() != null) {
msg.append(user.getUserName());
if (user.getUserName().isPresent()) {
msg.append(user.getUserName().get());
msg.append("@");
}
msg.append(host);

View File

@@ -235,7 +235,7 @@ class SshLog implements LifecycleListener {
if (user != null && user.isIdentifiedUser()) {
IdentifiedUser u = user.asIdentifiedUser();
userName = u.state().getUserName().orElse(null);
userName = u.getUserName().orElse(null);
accountId = "a/" + u.getAccountId().toString();
} else if (user instanceof PeerDaemonUser) {

View File

@@ -116,7 +116,7 @@ final class Receive extends AbstractGitCommand {
StringBuilder msg = new StringBuilder();
msg.append("Receive error on project \"").append(projectState.getName()).append("\"");
msg.append(" (user ");
msg.append(currentUser.state().getUserName().orElse(null));
msg.append(currentUser.getUserName().orElse(null));
msg.append(" account ");
msg.append(currentUser.getAccountId());
msg.append("): ");

View File

@@ -204,7 +204,7 @@ final class ShowConnections extends SshCommand {
IdentifiedUser u = user.asIdentifiedUser();
if (!numeric) {
Optional<String> name = u.state().getUserName();
Optional<String> name = u.getUserName();
if (name.isPresent()) {
return name.get();
}

View File

@@ -109,8 +109,8 @@ final class StreamEvents extends BaseCommand {
public String toString() {
StringBuilder b = new StringBuilder();
b.append("Stream Events");
if (currentUser.state().getUserName().isPresent()) {
b.append(" (" + currentUser.state().getUserName().get() + ")");
if (currentUser.getUserName().isPresent()) {
b.append(" (" + currentUser.getUserName().get() + ")");
}
return b.toString();
}