From 47d801963f86ce7ccf2fa6a52f7fcbab1a5bdec2 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 9 Dec 2015 01:24:59 +0000 Subject: [PATCH 1/7] Revert "Update buck to ba9f239f69287a553ca93af76a27484d83693563" Buck was forked only so that I could build Gerrit on my Macbook with OSX El Capitan. However doing this made Gerrit not buildable for anyone who follows the build instructions, which do not include a step for adding the Google fork to the buck repository's remotes. It's not worth updating the documentation; this particular version of buck is only needed on the stable-2.11 branch and only if it's being built on OSX El Capitan. Instead, revert the fork, and live with the fact that we have to use Ubuntu, or a version of OSX that doesn't suffer from this issue, to build Gerrit. This reverts commit 4d97777677abccbd86e4023abd01e635984f70ee. Change-Id: I71e2e5d0e6ec758a5cd2ae7888fcd103d476a8bf --- .buckversion | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.buckversion b/.buckversion index ffb82a4abe..9c09744a03 100644 --- a/.buckversion +++ b/.buckversion @@ -1 +1 @@ -ba9f239f69287a553ca93af76a27484d83693563 +79d36de9f5284f6e833cca81867d6088a25685fb From 0c3049183923a59ed947480de4c9fbeeda54af75 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 9 Dec 2015 01:25:50 +0000 Subject: [PATCH 2/7] Update 2.11.5 release notes to mention forked buck Add a warning that the Google repository needs to be added to the remotes. Change-Id: Ied78df702845215ba77f9ddef10e9d2b956a7df3 --- ReleaseNotes/ReleaseNotes-2.11.5.txt | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/ReleaseNotes/ReleaseNotes-2.11.5.txt b/ReleaseNotes/ReleaseNotes-2.11.5.txt index 03427bb4c2..d7758cbb48 100644 --- a/ReleaseNotes/ReleaseNotes-2.11.5.txt +++ b/ReleaseNotes/ReleaseNotes-2.11.5.txt @@ -9,6 +9,22 @@ https://gerrit-releases.storage.googleapis.com/gerrit-2.11.5.war] There are no schema changes from link:ReleaseNotes-2.11.4.html[2.11.4]. +Important Notes +--------------- + +*WARNING:* This release uses a forked version of buck. + +Buck was forked to cherry-pick an upstream fix for building on Mac OSX +El Capitan. + +To build this release from source, the Google repository must be added to +the remotes in the buck checkout: + +---- + $ git remote add google https://gerrit.googlesource.com/buck +---- + + Bug Fixes --------- From e6ac4ad532f01d5efeaaa6033dfb1e167d2dc0c8 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Tue, 8 Dec 2015 23:48:58 +0100 Subject: [PATCH 3/7] OAuth-Linking: Don't create new account when claimed identity unknown Claimed Identity feature was enabled to support old Google OpenID accounts, that cannot be activated anymore. In some corner cases, when for example the URL is not from the production Gerrit site, like it's always the case on staging instance, the OpenID identity may deviate from the original one. In case of mismatch, the lookup for real user for the claimed identity would fail, and the linking mode is lost, and as the consequence a new account is created. Creating new account, when user asked for linking is always the worst option we have, as this cannot be easily undone. Detect this case, preserve the linking mode and keep trying to link instead of create new account. Note that in case this account already exist, the linking would fail with the sane message. Test Plan I: 1. Create gerrit site gerrit.example.org in year 2010 2. Configure the site to use OpenId (non SSO mode) 3. Observe that 85% user base using Google OpeID 4. After Google's OpenID shutdown in May 2015, nobody is able to login anymore using their account and link their identity primary to Launchpad 5. Swap production site to staging site with entire database and Git repository 6. Install gerrit-oauth-provider plugin and activate automagically Google OpenID old token discovery and linking option 7. Configure new OAuth application on Google developer console, but route it to gerrit-test.example.org. Note that this deviation breaks the old OpenID tokens! 8. Test with old user, that has OpenID Google account, that was additionally linked to Launchpad OpenID provider 9. Login with Launchpad OpenID iendtity for this user 10. Profile=>Setting=>Link another identity 11. Select Google OAuth provider offered by the gerrit-oauth-provider plugin 12. Intead of linking to the existing account (or linking error) new account is created 13. This diff fixed this. Error is issued, that the account already exist and linking is not possible Note that when all this would be done on real production site, this error wouldn't happen, because the URL wouldn't deviate and there wouldn't be token mismatch between OpenID token in the database and the token discovered by Google's OAuth OpenID scope. But still, we could easily prevent in this very specific corner case the creation of new account. Note that it's still possible with this setting to create duplicate account for this user by signing in directly with Google OAuth provider without linking mode. However, this would also work as expected on the real production site, because old OpenID token would match with the existing Gerrit account and linking would happen automagically. That is exactly why the option link-to-existing-openid-accounts = true was invented. Unfortunately there is no way to test that this work as expected already on staging Gerrit instance. Test Plan II: 1. User register first time after Google's OpenID 2.0 shut down with OpenID provider, say Launchpad 2. Login with Launchpad 3. Profile=>Setting=>Link another identity 4. Link with Google OAuth2 provider Expected: The OAuth2 identity is linked to the existing account. Actual: New account is created. This diff fixed it. The problem is that I apparently misunderstood the migration spec: [1] and assumed that the OpenID token is provided only when a user was already associated with this site. This is not true. OpenID token also returned for new users, that were never registered with this site before. To rectify it, and still to work for both, known and unknown users, we apply the check. If the OpenID token is known use it for linking. If it is not known, ignore it, but preserve linking mode. * [1] https://developers.google.com/identity/protocols/OpenID2Migration#map-identifiers Change-Id: Icf70cde5fd96cd72aa383218e1d143107a551b45 --- .../auth/openid/OAuthSessionOverOpenID.java | 28 +++++++++++++++---- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OAuthSessionOverOpenID.java b/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OAuthSessionOverOpenID.java index 6d129bfd4f..ba7ce877f2 100644 --- a/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OAuthSessionOverOpenID.java +++ b/gerrit-openid/src/main/java/com/google/gerrit/httpd/auth/openid/OAuthSessionOverOpenID.java @@ -125,16 +125,32 @@ class OAuthSessionOverOpenID { try { String claimedIdentifier = user.getClaimedIdentity(); Account.Id actualId = accountManager.lookup(user.getExternalId()); - // Use case 1: claimed identity was provided during handshake phase + Account.Id claimedId = null; + + // We try to retrieve claimed identity. + // For some reason, for example staging instance + // it may deviate from the really old OpenID identity. + // What we want to avoid in any event is to create new + // account instead of linking to the existing one. + // That why we query it here, not to lose linking mode. if (!Strings.isNullOrEmpty(claimedIdentifier)) { - Account.Id claimedId = accountManager.lookup(claimedIdentifier); - if (claimedId != null && actualId != null) { + claimedId = accountManager.lookup(claimedIdentifier); + if (claimedId == null) { + log.debug("Claimed identity is unknown"); + } + } + + // Use case 1: claimed identity was provided during handshake phase + // and user account exists for this identity + if (claimedId != null) { + log.debug("Claimed identity is set and is known"); + if (actualId != null) { if (claimedId.equals(actualId)) { // Both link to the same account, that's what we expected. } else { // This is (for now) a fatal error. There are two records - // for what might be the same user. - // + // for what might be the same user. The admin would have to + // link the accounts manually. log.error("OAuth accounts disagree over user identity:\n" + " Claimed ID: " + claimedId + " is " + claimedIdentifier + "\n" + " Delgate ID: " + actualId + " is " @@ -142,7 +158,7 @@ class OAuthSessionOverOpenID { rsp.sendError(HttpServletResponse.SC_FORBIDDEN); return; } - } else if (claimedId != null && actualId == null) { + } else { // Claimed account already exists: link to it. // try { From bb8fd144f609d901240d0c8ef83759a5b913aa10 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Tue, 15 Dec 2015 10:54:14 -0500 Subject: [PATCH 4/7] Normalize case of {Author,Committer}Predicate Index implementations aren't guaranteed to do case folding of full text searches internally, but we can reasonably expect them to find exact matches when the case is the same. Use the same normalization in {Author,Committer}Predicate when sending the value to the index that we do in ChangeField when populating the field. Change-Id: I269a79f4ed8392d7bddb2d3fdd10193e3e756457 --- .../gerrit/server/query/change/AuthorPredicate.java | 2 +- .../gerrit/server/query/change/CommitterPredicate.java | 2 +- .../server/query/change/AbstractQueryChangesTest.java | 8 ++++++++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/AuthorPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/AuthorPredicate.java index 193a06176d..6264f3aa03 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/AuthorPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/AuthorPredicate.java @@ -23,7 +23,7 @@ import com.google.gwtorm.server.OrmException; public class AuthorPredicate extends IndexPredicate { AuthorPredicate(String value) { - super(AUTHOR, FIELD_AUTHOR, value); + super(AUTHOR, FIELD_AUTHOR, value.toLowerCase()); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/CommitterPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/CommitterPredicate.java index e5d9529362..3cb7f8edd8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/CommitterPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/CommitterPredicate.java @@ -23,7 +23,7 @@ import com.google.gwtorm.server.OrmException; public class CommitterPredicate extends IndexPredicate { CommitterPredicate(String value) { - super(COMMITTER, FIELD_COMMITTER, value); + super(COMMITTER, FIELD_COMMITTER, value.toLowerCase()); } @Override diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index 3382264c85..d01fb0361f 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java @@ -404,6 +404,10 @@ public abstract class AbstractQueryChangesTest { // By name part assertQuery("author:Author", change1); + // Case insensitive + assertQuery("author:jAuThOr", change1); + assertQuery("author:ExAmPlE", change1); + // By non-existing email address / name / part assertQuery("author:jcommitter@example.com"); assertQuery("author:somewhere.com"); @@ -427,6 +431,10 @@ public abstract class AbstractQueryChangesTest { // By name part assertQuery("committer:Committer", change1); + // Case insensitive + assertQuery("committer:jCoMmItTeR", change1); + assertQuery("committer:ExAmPlE", change1); + // By non-existing email address / name / part assertQuery("committer:jauthor@example.com"); assertQuery("committer:somewhere.com"); From 9afdd6236c01ec43b7d6847e7023685d8481e96d Mon Sep 17 00:00:00 2001 From: Marco Miller Date: Tue, 15 Dec 2015 10:54:24 -0500 Subject: [PATCH 5/7] Remove bucklets/local_jar.bucklet soft-link to removed lib/local.defs Since the latter was removed (by commit 455ed9c), the former links to that now-missing file; hence its removal. Change-Id: I4eba7bf4788c52143ab5a72c8c337ee9aa3c74ff --- bucklets/local_jar.bucklet | 1 - 1 file changed, 1 deletion(-) delete mode 120000 bucklets/local_jar.bucklet diff --git a/bucklets/local_jar.bucklet b/bucklets/local_jar.bucklet deleted file mode 120000 index 890482472c..0000000000 --- a/bucklets/local_jar.bucklet +++ /dev/null @@ -1 +0,0 @@ -../lib/local.defs \ No newline at end of file From 964821f6f654ef55cd6c3ccccdc5a574f3f31fbe Mon Sep 17 00:00:00 2001 From: Mike Bjorge Date: Tue, 8 Dec 2015 15:57:44 -0800 Subject: [PATCH 6/7] Put Change-Id after Test: footers in commit messages. Update the commit-msg hook to insert the Change-Id footer after any Test: footers, as is done with Bug: and Issue: footers. Change-Id: I75e756ec95f1472d4bdf0284b2bf9276be5dd35d (cherry picked from commit 72cf723e7e9fce47ff7b2cf00b14c80a9c206905) --- .../com/google/gerrit/server/tools/root/hooks/commit-msg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gerrit-server/src/main/resources/com/google/gerrit/server/tools/root/hooks/commit-msg b/gerrit-server/src/main/resources/com/google/gerrit/server/tools/root/hooks/commit-msg index 69884597f0..b537f265de 100644 --- a/gerrit-server/src/main/resources/com/google/gerrit/server/tools/root/hooks/commit-msg +++ b/gerrit-server/src/main/resources/com/google/gerrit/server/tools/root/hooks/commit-msg @@ -19,7 +19,7 @@ unset GREP_OPTIONS -CHANGE_ID_AFTER="Bug|Issue" +CHANGE_ID_AFTER="Bug|Issue|Test" MSG="$1" # Check for, and add if missing, a unique Change-Id From 82d55631b8e424b7c4e567d21acd6bab87806398 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 14 Dec 2015 10:31:27 +0000 Subject: [PATCH 7/7] Document that ldap.groupBase and ldap.accountBase are repeatable Both of these options can be given multiple times in the config file, for example: [ldap] accountBase = OU=Europe,DC=example,DC=net accountBase = OU=America,DC=example,DC=net accountBase = OU=Asia,DC=example,DC=net Extend the documentation to mention this. Change-Id: Iba684adc1beea145a7fdd1d1d71ca6c81bbda8a7 --- Documentation/config-gerrit.txt | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 837469faa2..f6aec768ec 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -2323,6 +2323,9 @@ server to respond until the TCP connection times out. + Root of the tree containing all user accounts. This is typically of the form `ou=people,dc=example,dc=com`. ++ +This setting may be added multiple times to specify more than +one root. [[ldap.accountScope]]ldap.accountScope:: + @@ -2434,6 +2437,9 @@ Active Directory. + Root of the tree containing all group objects. This is typically of the form `ou=groups,dc=example,dc=com`. ++ +This setting may be added multiple times to specify more than +one root. [[ldap.groupScope]]ldap.groupScope:: +