opendev/gerrit
Code Issues Proposed changes

Merge branch 'stable-3.0'

Browse Source 
* stable-3.0:
  Fix eclipse project generation
  Always send an email when adding or deleting an SSH key
  AccountIT: Add test for adding GPG key to another account
  AccountIT: Add tests for modifying SSH keys of another account
  Remove GWT leftovers from .gitignore
  Update rules_closure to latest version
  Use bazelisk to switch between used bazel version
  Eclipse: Configure classpath to ignore warnings in generated classes
  Allow load labels to cross package boundaries by default

Change-Id: Iaf0aef486ef7dc189c11ab3cb0417600155c0b99
This commit is contained in:
David Pursehouse
2019-06-08 12:12:54 +09:00
parent 71e434c4b4 e2794d533e
commit a12e705889
8 changed files with 104 additions and 76 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
.bazelrc
View File

@@ -4,6 +4,7 @@ build --experimental_strict_action_env
build --action_env=PATH
build --disk_cache=~/.gerritcodereview/bazel-cache/cas
build --java_toolchain //tools:error_prone_warnings_toolchain
build --incompatible_disallow_load_labels_to_cross_package_boundaries=false
test --build_tests_only
test --test_output=errors

1
.bazelversion Normal file
View File

@@ -0,0 +1 @@
0.27.0rc3

2
.gitignore vendored
View File

@@ -6,7 +6,6 @@
*.swp
*~
.DS_Store
.gwt_work_dir
/.apt_generated
/.apt_generated_tests
/.bazel_path
@@ -27,7 +26,6 @@
/eclipse-out
/extras
/gerrit-package-plugins
/gwt-unitCache
/infer-out
/local.properties
/node_modules/

6
WORKSPACE
View File

@@ -49,9 +49,9 @@ http_archive(
http_archive(
name = "io_bazel_rules_closure",
sha256 = "75c58680af5f7b938ce9fe2abe8ecd9d24c698d160c0b71a945bd100fa77632b",
strip_prefix = "rules_closure-10cb1a78bd6cc8927eb39c2644c0369934f4aed6",
urls = ["https://github.com/bazelbuild/rules_closure/archive/10cb1a78bd6cc8927eb39c2644c0369934f4aed6.tar.gz"],
sha256 = "d075b084e6f4109d1b1ab877495ac72c1a6c4dbc593980967e0b7359f4254d7e",
strip_prefix = "rules_closure-78f1192664acf66ca1de24116cbcc98e1698f26b",
urls = ["https://github.com/bazelbuild/rules_closure/archive/78f1192664acf66ca1de24116cbcc98e1698f26b.tar.gz"],
)
# File is specific to Polymer and copied from the Closure Github -- should be

37
java/com/google/gerrit/server/mail/send/AddKeySender.java
View File

@@ -17,13 +17,9 @@ package com.google.gerrit.server.mail.send;
import com.google.common.base.Joiner;
import com.google.gerrit.exceptions.EmailException;
import com.google.gerrit.extensions.api.changes.RecipientType;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.mail.Address;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountSshKey;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.util.List;
@@ -35,22 +31,14 @@ public class AddKeySender extends OutgoingEmail {
AddKeySender create(IdentifiedUser user, List<String> gpgKey);
}
private final PermissionBackend permissionBackend;
private final IdentifiedUser callingUser;
private final IdentifiedUser user;
private final AccountSshKey sshKey;
private final List<String> gpgKeys;
@AssistedInject
public AddKeySender(
EmailArguments ea,
PermissionBackend permissionBackend,
IdentifiedUser callingUser,
@Assisted IdentifiedUser user,
@Assisted AccountSshKey sshKey) {
EmailArguments ea, @Assisted IdentifiedUser user, @Assisted AccountSshKey sshKey) {
super(ea, "addkey");
this.permissionBackend = permissionBackend;
this.callingUser = callingUser;
this.user = user;
this.sshKey = sshKey;
this.gpgKeys = null;
@@ -58,14 +46,8 @@ public class AddKeySender extends OutgoingEmail {
@AssistedInject
public AddKeySender(
EmailArguments ea,
PermissionBackend permissionBackend,
IdentifiedUser callingUser,
@Assisted IdentifiedUser user,
@Assisted List<String> gpgKeys) {
EmailArguments ea, @Assisted IdentifiedUser user, @Assisted List<String> gpgKeys) {
super(ea, "addkey");
this.permissionBackend = permissionBackend;
this.callingUser = callingUser;
this.user = user;
this.sshKey = null;
this.gpgKeys = gpgKeys;
@@ -85,20 +67,7 @@ public class AddKeySender extends OutgoingEmail {
return false;
}
if (user.equals(callingUser)) {
// Send email if the user self-added a key; this notification is necessary to alert
// the user if their account was compromised and a key was unexpectedly added.
return true;
}
try {
// Don't email if an administrator added a key on behalf of the user.
permissionBackend.user(callingUser).check(GlobalPermission.ADMINISTRATE_SERVER);
return false;
} catch (AuthException | PermissionBackendException e) {
// Send email if a non-administrator modified the keys, e.g. by MODIFY_ACCOUNT.
return true;
}
return true;
}
@Override

37
java/com/google/gerrit/server/mail/send/DeleteKeySender.java
View File

@@ -17,13 +17,9 @@ package com.google.gerrit.server.mail.send;
import com.google.common.base.Joiner;
import com.google.gerrit.exceptions.EmailException;
import com.google.gerrit.extensions.api.changes.RecipientType;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.mail.Address;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountSshKey;
import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.util.Collections;
@@ -36,22 +32,14 @@ public class DeleteKeySender extends OutgoingEmail {
DeleteKeySender create(IdentifiedUser user, List<String> gpgKeyFingerprints);
}
private final PermissionBackend permissionBackend;
private final IdentifiedUser callingUser;
private final IdentifiedUser user;
private final AccountSshKey sshKey;
private final List<String> gpgKeyFingerprints;
@AssistedInject
public DeleteKeySender(
EmailArguments ea,
PermissionBackend permissionBackend,
IdentifiedUser callingUser,
@Assisted IdentifiedUser user,
@Assisted AccountSshKey sshKey) {
EmailArguments ea, @Assisted IdentifiedUser user, @Assisted AccountSshKey sshKey) {
super(ea, "deletekey");
this.permissionBackend = permissionBackend;
this.callingUser = callingUser;
this.user = user;
this.gpgKeyFingerprints = Collections.emptyList();
this.sshKey = sshKey;
@@ -59,14 +47,8 @@ public class DeleteKeySender extends OutgoingEmail {
@AssistedInject
public DeleteKeySender(
EmailArguments ea,
PermissionBackend permissionBackend,
IdentifiedUser callingUser,
@Assisted IdentifiedUser user,
@Assisted List<String> gpgKeyFingerprints) {
EmailArguments ea, @Assisted IdentifiedUser user, @Assisted List<String> gpgKeyFingerprints) {
super(ea, "deletekey");
this.permissionBackend = permissionBackend;
this.callingUser = callingUser;
this.user = user;
this.gpgKeyFingerprints = gpgKeyFingerprints;
this.sshKey = null;
@@ -81,20 +63,7 @@ public class DeleteKeySender extends OutgoingEmail {
@Override
protected boolean shouldSendMessage() {
if (user.equals(callingUser)) {
// Send email if the user self-removed a key; this notification is necessary to alert
// the user if their account was compromised and a key was unexpectedly deleted.
return true;
}
try {
// Don't email if an administrator removed a key on behalf of the user.
permissionBackend.user(callingUser).check(GlobalPermission.ADMINISTRATE_SERVER);
return false;
} catch (AuthException | PermissionBackendException e) {
// Send email if a non-administrator modified the keys, e.g. by MODIFY_ACCOUNT.
return true;
}
return true;
}
@Override

80
javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
View File

@@ -2043,6 +2043,16 @@ public class AccountIT extends AbstractDaemonTest {
assertThat(thrown).hasMessageThat().contains(id);
}
@Test
public void adminCannotAddGpgKeyToOtherAccount() throws Exception {
TestKey key = validKeyWithoutExpiration();
addExternalIdEmail(user, "test1@example.com");
sender.clear();
requestScopeOperations.setApiUser(admin.id());
assertThrows(ResourceNotFoundException.class, () -> addGpgKey(user, key.getPublicKeyArmored()));
}
@Test
public void reAddExistingGpgKey() throws Exception {
addExternalIdEmail(admin, "test5@example.com");
@@ -2084,7 +2094,8 @@ public class AccountIT extends AbstractDaemonTest {
requestScopeOperations.setApiUser(user.id());
ResourceConflictException thrown =
assertThrows(ResourceConflictException.class, () -> addGpgKey(key.getPublicKeyArmored()));
assertThrows(
ResourceConflictException.class, () -> addGpgKey(user, key.getPublicKeyArmored()));
assertThat(thrown).hasMessageThat().contains("GPG key already associated with another account");
}
@@ -2243,6 +2254,63 @@ public class AccountIT extends AbstractDaemonTest {
accountIndexedCounter.assertReindexOf(admin);
}
@Test
@UseSsh
public void adminCanAddOrRemoveSshKeyOnOtherAccount() throws Exception {
// The test account should initially have exactly one ssh key
List<SshKeyInfo> info = gApi.accounts().self().listSshKeys();
assertThat(info).hasSize(1);
assertSequenceNumbers(info);
SshKeyInfo key = info.get(0);
KeyPair keyPair = sshKeys.getKeyPair(admin);
String initial = TestSshKeys.publicKey(keyPair, admin.email());
assertThat(key.sshPublicKey).isEqualTo(initial);
accountIndexedCounter.assertNoReindex();
// Add a new key
sender.clear();
String newKey = TestSshKeys.publicKey(TestSshKeys.genSshKey(), user.email());
gApi.accounts().id(user.username()).addSshKey(newKey);
info = gApi.accounts().id(user.username()).listSshKeys();
assertThat(info).hasSize(2);
assertSequenceNumbers(info);
accountIndexedCounter.assertReindexOf(user);
assertThat(sender.getMessages()).hasSize(1);
Message message = sender.getMessages().get(0);
assertThat(message.rcpt()).containsExactly(user.getEmailAddress());
assertThat(message.body()).contains("new SSH keys have been added");
// Delete key
sender.clear();
gApi.accounts().id(user.username()).deleteSshKey(1);
info = gApi.accounts().id(user.username()).listSshKeys();
assertThat(info).hasSize(1);
accountIndexedCounter.assertReindexOf(user);
assertThat(sender.getMessages()).hasSize(1);
message = sender.getMessages().get(0);
assertThat(message.rcpt()).containsExactly(user.getEmailAddress());
assertThat(message.body()).contains("SSH keys have been deleted");
}
@Test
@UseSsh
public void userCannotAddSshKeyToOtherAccount() throws Exception {
String newKey = TestSshKeys.publicKey(TestSshKeys.genSshKey(), admin.email());
requestScopeOperations.setApiUser(user.id());
assertThrows(AuthException.class, () -> gApi.accounts().id(admin.username()).addSshKey(newKey));
}
@Test
@UseSsh
public void userCannotDeleteSshKeyOfOtherAccount() throws Exception {
requestScopeOperations.setApiUser(user.id());
assertThrows(
ResourceNotFoundException.class,
() -> gApi.accounts().id(admin.username()).deleteSshKey(0));
}
// reindex is tested by {@link AbstractQueryAccountsTest#reindex}
@Test
public void reindexPermissions() throws Exception {
@@ -3129,9 +3197,15 @@ public class AccountIT extends AbstractDaemonTest {
}
private Map<String, GpgKeyInfo> addGpgKey(String armored) throws Exception {
return addGpgKey(admin, armored);
}
private Map<String, GpgKeyInfo> addGpgKey(TestAccount account, String armored) throws Exception {
Map<String, GpgKeyInfo> gpgKeys =
gApi.accounts().self().putGpgKeys(ImmutableList.of(armored), ImmutableList.of());
accountIndexedCounter.assertReindexOf(gApi.accounts().self().get());
gApi.accounts()
.id(account.username())
.putGpgKeys(ImmutableList.of(armored), ImmutableList.<String>of());
accountIndexedCounter.assertReindexOf(gApi.accounts().id(account.username()).get());
return gpgKeys;
}

16
tools/eclipse/project.py
View File

@@ -161,12 +161,25 @@ def gen_classpath(ext):
e.setAttribute('output', out)
if exported:
e.setAttribute('exported', 'true')
atts = None
if out and "test" in out:
atts = doc.createElement('attributes')
testAtt = doc.createElement('attribute')
testAtt.setAttribute('name', 'test')
testAtt.setAttribute('value', 'true')
atts.appendChild(testAtt)
if "apt_generated" in path:
if not atts:
atts = doc.createElement('attributes')
ignoreOptionalProblems = doc.createElement('attribute')
ignoreOptionalProblems.setAttribute('name', 'ignore_optional_problems')
ignoreOptionalProblems.setAttribute('value', 'true')
atts.appendChild(ignoreOptionalProblems)
optional = doc.createElement('attribute')
optional.setAttribute('name', 'optional')
optional.setAttribute('value', 'true')
atts.appendChild(optional)
if atts:
e.appendChild(atts)
doc.documentElement.appendChild(e)
@@ -189,6 +202,7 @@ def gen_classpath(ext):
# Exceptions: both source and lib
if p.endswith('libquery_parser.jar') or \
p.endswith('libgerrit-prolog-common.jar') or \
p.endswith('com_google_protobuf/libprotobuf_java.jar') or \
p.endswith('lucene-core-and-backward-codecs__merged.jar'):
lib.add(p)
# JGit dependency from external repository
@@ -264,6 +278,8 @@ def gen_classpath(ext):
classpathentry('con', JRE)
classpathentry('output', 'eclipse-out/classes')
classpathentry('src', '.apt_generated')
classpathentry('src', '.apt_generated_tests', out="eclipse-out/test")
p = path.join(ROOT, '.classpath')
with open(p, 'w') as fd: