Merge changes Ic4e04936,I6462d87c into stable-2.16

* changes:
  Fix editing name & email for service user
  Fix accountBelongsToRealm implementation

7 files changed, 185 insertions, 10 deletions

diff --git a/java/com/google/gerrit/server/auth/ldap/LdapRealm.java b/java/com/google/gerrit/server/auth/ldap/LdapRealm.java
index ed446f218a..b047488097 100644
--- a/java/com/google/gerrit/server/auth/ldap/LdapRealm.java
+++ b/java/com/google/gerrit/server/auth/ldap/LdapRealm.java
@@ -336,7 +336,7 @@ class LdapRealm extends AbstractRealm {
   @Override
   public boolean accountBelongsToRealm(Collection<ExternalId> externalIds) {
     for (ExternalId id : externalIds) {
-      if (id.toString().contains(SCHEME_GERRIT)) {
+      if (id.isScheme(SCHEME_GERRIT)) {
         return true;
       }
     }
diff --git a/java/com/google/gerrit/server/auth/oauth/OAuthRealm.java b/java/com/google/gerrit/server/auth/oauth/OAuthRealm.java
index 54d50f08f4..2cef62d723 100644
--- a/java/com/google/gerrit/server/auth/oauth/OAuthRealm.java
+++ b/java/com/google/gerrit/server/auth/oauth/OAuthRealm.java
@@ -122,7 +122,7 @@ public class OAuthRealm extends AbstractRealm {
   @Override
   public boolean accountBelongsToRealm(Collection<ExternalId> externalIds) {
     for (ExternalId id : externalIds) {
-      if (id.toString().contains(SCHEME_EXTERNAL)) {
+      if (id.isScheme(SCHEME_EXTERNAL)) {
         return true;
       }
     }
diff --git a/java/com/google/gerrit/server/restapi/account/DeleteEmail.java b/java/com/google/gerrit/server/restapi/account/DeleteEmail.java
index 6bacde2220..36788c754b 100644
--- a/java/com/google/gerrit/server/restapi/account/DeleteEmail.java
+++ b/java/com/google/gerrit/server/restapi/account/DeleteEmail.java
@@ -24,6 +24,7 @@ import com.google.gerrit.extensions.restapi.ResourceConflictException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountException;
@@ -80,12 +81,14 @@ public class DeleteEmail implements RestModifyView<AccountResource.Email, Input>
   public Response<?> apply(IdentifiedUser user, String email)
       throws ResourceNotFoundException, ResourceConflictException, MethodNotAllowedException,
           OrmException, IOException, ConfigInvalidException {
-    if (!realm.allowsEdit(AccountFieldName.REGISTER_NEW_EMAIL)) {
+    Account.Id accountId = user.getAccountId();
+    if (realm.accountBelongsToRealm(externalIds.byAccount(accountId))
+        && !realm.allowsEdit(AccountFieldName.REGISTER_NEW_EMAIL)) {
       throw new MethodNotAllowedException("realm does not allow deleting emails");
     }
 
     Set<ExternalId> extIds =
-        externalIds.byAccount(user.getAccountId()).stream()
+        externalIds.byAccount(accountId).stream()
             .filter(e -> email.equals(e.email()))
             .collect(toSet());
     if (extIds.isEmpty()) {
diff --git a/java/com/google/gerrit/server/restapi/account/PutName.java b/java/com/google/gerrit/server/restapi/account/PutName.java
index 1e00aaccaf..4918aa38dc 100644
--- a/java/com/google/gerrit/server/restapi/account/PutName.java
+++ b/java/com/google/gerrit/server/restapi/account/PutName.java
@@ -22,6 +22,7 @@ import com.google.gerrit.extensions.restapi.MethodNotAllowedException;
 import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
 import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.extensions.restapi.RestModifyView;
+import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.ServerInitiated;
@@ -29,6 +30,7 @@ import com.google.gerrit.server.account.AccountResource;
 import com.google.gerrit.server.account.AccountState;
 import com.google.gerrit.server.account.AccountsUpdate;
 import com.google.gerrit.server.account.Realm;
+import com.google.gerrit.server.account.externalids.ExternalIds;
 import com.google.gerrit.server.permissions.GlobalPermission;
 import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -44,6 +46,7 @@ public class PutName implements RestModifyView<AccountResource, NameInput> {
   private final Provider<CurrentUser> self;
   private final Realm realm;
   private final PermissionBackend permissionBackend;
+  private final ExternalIds externalIds;
   private final Provider<AccountsUpdate> accountsUpdateProvider;
 
   @Inject
@@ -51,10 +54,12 @@ public class PutName implements RestModifyView<AccountResource, NameInput> {
       Provider<CurrentUser> self,
       Realm realm,
       PermissionBackend permissionBackend,
+      ExternalIds externalIds,
       @ServerInitiated Provider<AccountsUpdate> accountsUpdateProvider) {
     this.self = self;
     this.realm = realm;
     this.permissionBackend = permissionBackend;
+    this.externalIds = externalIds;
     this.accountsUpdateProvider = accountsUpdateProvider;
   }
 
@@ -71,11 +76,14 @@ public class PutName implements RestModifyView<AccountResource, NameInput> {
   public Response<String> apply(IdentifiedUser user, NameInput input)
       throws MethodNotAllowedException, ResourceNotFoundException, IOException,
           ConfigInvalidException, OrmException {
+
     if (input == null) {
       input = new NameInput();
     }
 
-    if (!realm.allowsEdit(AccountFieldName.FULL_NAME)) {
+    Account.Id accountId = user.getAccountId();
+    if (realm.accountBelongsToRealm(externalIds.byAccount(accountId))
+        && !realm.allowsEdit(AccountFieldName.FULL_NAME)) {
       throw new MethodNotAllowedException("realm does not allow editing name");
     }
 
@@ -83,7 +91,7 @@ public class PutName implements RestModifyView<AccountResource, NameInput> {
     AccountState accountState =
         accountsUpdateProvider
             .get()
-            .update("Set Full Name via API", user.getAccountId(), u -> u.setFullName(newName))
+            .update("Set Full Name via API", accountId, u -> u.setFullName(newName))
             .orElseThrow(() -> new ResourceNotFoundException("account not found"));
     return Strings.isNullOrEmpty(accountState.getAccount().getFullName())
         ? Response.none()
diff --git a/java/com/google/gerrit/server/restapi/account/PutUsername.java b/java/com/google/gerrit/server/restapi/account/PutUsername.java
index 7fff626d16..bc95153cbe 100644
--- a/java/com/google/gerrit/server/restapi/account/PutUsername.java
+++ b/java/com/google/gerrit/server/restapi/account/PutUsername.java
@@ -79,10 +79,6 @@ public class PutUsername implements RestModifyView<AccountResource, UsernameInpu
       permissionBackend.currentUser().check(GlobalPermission.ADMINISTRATE_SERVER);
     }
 
-    if (!realm.allowsEdit(AccountFieldName.USER_NAME)) {
-      throw new MethodNotAllowedException("realm does not allow editing username");
-    }
-
     if (input == null) {
       input = new UsernameInput();
     }
@@ -92,6 +88,11 @@ public class PutUsername implements RestModifyView<AccountResource, UsernameInpu
       throw new MethodNotAllowedException("Username cannot be changed.");
     }
 
+    if (realm.accountBelongsToRealm(externalIds.byAccount(accountId))
+        && !realm.allowsEdit(AccountFieldName.USER_NAME)) {
+      throw new MethodNotAllowedException("realm does not allow editing username");
+    }
+
     if (Strings.isNullOrEmpty(input.username)) {
       return input.username;
     }
diff --git a/javatests/com/google/gerrit/server/auth/ldap/LdapRealmTest.java b/javatests/com/google/gerrit/server/auth/ldap/LdapRealmTest.java
new file mode 100644
index 0000000000..13de3e7ad0
--- /dev/null
+++ b/javatests/com/google/gerrit/server/auth/ldap/LdapRealmTest.java
@@ -0,0 +1,94 @@
+// Copyright (C) 2020 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.auth.ldap;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_GERRIT;
+import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_MAILTO;
+import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_USERNAME;
+import static com.google.gerrit.server.auth.ldap.LdapModule.GROUP_CACHE;
+import static com.google.gerrit.server.auth.ldap.LdapModule.GROUP_EXIST_CACHE;
+import static com.google.gerrit.server.auth.ldap.LdapModule.PARENT_GROUPS_CACHE;
+import static com.google.gerrit.server.auth.ldap.LdapModule.USERNAME_CACHE;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.server.account.externalids.ExternalId;
+import com.google.gerrit.server.cache.CacheModule;
+import com.google.gerrit.testing.InMemoryModule;
+import com.google.inject.Guice;
+import com.google.inject.Inject;
+import com.google.inject.Injector;
+import com.google.inject.TypeLiteral;
+import java.util.Arrays;
+import java.util.Optional;
+import java.util.Set;
+import org.junit.Before;
+import org.junit.Test;
+
+public final class LdapRealmTest {
+  @Inject private LdapRealm ldapRealm = null;
+
+  @Before
+  public void setUpInjector() throws Exception {
+    Injector injector =
+        Guice.createInjector(
+            new InMemoryModule(),
+            new CacheModule() {
+              @Override
+              protected void configure() {
+                cache(GROUP_CACHE, String.class, new TypeLiteral<Set<AccountGroup.UUID>>() {})
+                    .loader(LdapRealm.MemberLoader.class);
+                cache(USERNAME_CACHE, String.class, new TypeLiteral<Optional<Account.Id>>() {})
+                    .loader(LdapRealm.UserLoader.class);
+                cache(GROUP_EXIST_CACHE, String.class, new TypeLiteral<Boolean>() {})
+                    .loader(LdapRealm.ExistenceLoader.class);
+                cache(
+                    PARENT_GROUPS_CACHE, String.class, new TypeLiteral<ImmutableSet<String>>() {});
+              }
+            });
+    injector.injectMembers(this);
+  }
+
+  private ExternalId id(String scheme, String id) {
+    return ExternalId.create(scheme, id, new Account.Id(1000));
+  }
+
+  private boolean accountBelongsToRealm(ExternalId... ids) {
+    return ldapRealm.accountBelongsToRealm(Arrays.asList(ids));
+  }
+
+  private boolean accountBelongsToRealm(String scheme, String id) {
+    return accountBelongsToRealm(id(scheme, id));
+  }
+
+  @Test
+  public void accountBelongsToRealm() throws Exception {
+    assertThat(accountBelongsToRealm(SCHEME_GERRIT, "test")).isTrue();
+    assertThat(accountBelongsToRealm(id(SCHEME_USERNAME, "test"), id(SCHEME_GERRIT, "test")))
+        .isTrue();
+    assertThat(accountBelongsToRealm(id(SCHEME_GERRIT, "test"), id(SCHEME_USERNAME, "test")))
+        .isTrue();
+
+    assertThat(accountBelongsToRealm(SCHEME_USERNAME, "test")).isFalse();
+    assertThat(accountBelongsToRealm(SCHEME_MAILTO, "foo@bar.com")).isFalse();
+
+    assertThat(accountBelongsToRealm(SCHEME_USERNAME, "gerrit")).isFalse();
+    assertThat(accountBelongsToRealm(SCHEME_USERNAME, "xxgerritxx")).isFalse();
+    assertThat(accountBelongsToRealm(SCHEME_MAILTO, "gerrit.foo@bar.com")).isFalse();
+    assertThat(accountBelongsToRealm(SCHEME_MAILTO, "bar.gerrit@bar.com")).isFalse();
+  }
+}
diff --git a/javatests/com/google/gerrit/server/auth/oauth/OAuthRealmTest.java b/javatests/com/google/gerrit/server/auth/oauth/OAuthRealmTest.java
new file mode 100644
index 0000000000..7a0661a59b
--- /dev/null
+++ b/javatests/com/google/gerrit/server/auth/oauth/OAuthRealmTest.java
@@ -0,0 +1,69 @@
+// Copyright (C) 2020 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.auth.oauth;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_EXTERNAL;
+import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_MAILTO;
+import static com.google.gerrit.server.account.externalids.ExternalId.SCHEME_USERNAME;
+
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.server.account.externalids.ExternalId;
+import com.google.gerrit.testing.InMemoryModule;
+import com.google.inject.Guice;
+import com.google.inject.Inject;
+import com.google.inject.Injector;
+import java.util.Arrays;
+import org.junit.Before;
+import org.junit.Test;
+
+public final class OAuthRealmTest {
+  @Inject private OAuthRealm oauthRealm = null;
+
+  @Before
+  public void setUpInjector() throws Exception {
+    Injector injector = Guice.createInjector(new InMemoryModule());
+    injector.injectMembers(this);
+  }
+
+  private ExternalId id(String scheme, String id) {
+    return ExternalId.create(scheme, id, new Account.Id(1000));
+  }
+
+  private boolean accountBelongsToRealm(ExternalId... ids) {
+    return oauthRealm.accountBelongsToRealm(Arrays.asList(ids));
+  }
+
+  private boolean accountBelongsToRealm(String scheme, String id) {
+    return accountBelongsToRealm(id(scheme, id));
+  }
+
+  @Test
+  public void accountBelongsToRealm() throws Exception {
+    assertThat(accountBelongsToRealm(SCHEME_EXTERNAL, "test")).isTrue();
+    assertThat(accountBelongsToRealm(id(SCHEME_USERNAME, "test"), id(SCHEME_EXTERNAL, "test")))
+        .isTrue();
+    assertThat(accountBelongsToRealm(id(SCHEME_EXTERNAL, "test"), id(SCHEME_USERNAME, "test")))
+        .isTrue();
+
+    assertThat(accountBelongsToRealm(SCHEME_USERNAME, "test")).isFalse();
+    assertThat(accountBelongsToRealm(SCHEME_MAILTO, "foo@bar.com")).isFalse();
+
+    assertThat(accountBelongsToRealm(SCHEME_USERNAME, "external")).isFalse();
+    assertThat(accountBelongsToRealm(SCHEME_USERNAME, "xxexternalxx")).isFalse();
+    assertThat(accountBelongsToRealm(SCHEME_MAILTO, "external.foo@bar.com")).isFalse();
+    assertThat(accountBelongsToRealm(SCHEME_MAILTO, "bar.external@bar.com")).isFalse();
+  }
+}