From 1199891854bfce499b40c3611b026c025cc30816 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 3 Jun 2011 09:12:13 +0200 Subject: [PATCH 1/5] Resolve Project Owners when checking access right on any ref A project is visible to a user when the user has Read Access on any ref of the project. This check whether an access right is granted on any ref is not taking the access rights granted to the 'Project Owners' group into account. As a result a user does not see a project if he is project owner and Read Access is only granted to the 'Project Owners' group. This change ensures that the 'Project Owners' group is properly resolved in this case. Bug: issue 997 Change-Id: I27cd8293e5c4a01c867a4e076073bf587294e0ba Signed-off-by: Edwin Kempin --- .../gerrit/server/project/ProjectControl.java | 29 ++++++++++++++++--- .../gerrit/server/project/RefControlTest.java | 3 +- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java index 25778a6fb8..2a55019761 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java @@ -21,6 +21,7 @@ import com.google.gerrit.reviewdb.Branch; import com.google.gerrit.reviewdb.Change; import com.google.gerrit.reviewdb.Project; import com.google.gerrit.reviewdb.RefRight; +import com.google.gerrit.reviewdb.SystemConfig; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.ReplicationUser; import com.google.gerrit.server.config.GitReceivePackGroups; @@ -29,6 +30,7 @@ import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.assistedinject.Assisted; +import java.util.Collections; import java.util.HashSet; import java.util.Set; @@ -101,6 +103,7 @@ public class ProjectControl { ProjectControl create(CurrentUser who, ProjectState ps); } + private final SystemConfig systemConfig; private final Set uploadGroups; private final Set receiveGroups; @@ -109,10 +112,12 @@ public class ProjectControl { private final ProjectState state; @Inject - ProjectControl(@GitUploadPackGroups Set uploadGroups, + ProjectControl(final SystemConfig systemConfig, + @GitUploadPackGroups Set uploadGroups, @GitReceivePackGroups Set receiveGroups, final RefControl.Factory refControlFactory, @Assisted CurrentUser who, @Assisted ProjectState ps) { + this.systemConfig = systemConfig; this.uploadGroups = uploadGroups; this.receiveGroups = receiveGroups; this.refControlFactory = refControlFactory; @@ -197,7 +202,7 @@ public class ProjectControl { // TODO (anatol.pomazau): Try to merge this method with similar RefRightsForPattern#canPerform private boolean canPerformOnAnyRef(ApprovalCategory.Id actionId, short requireValue) { - final Set groups = user.getEffectiveGroups(); + final Set groups = getEffectiveUserGroups(); for (final RefRight pr : state.getAllRights(actionId, true)) { if (groups.contains(pr.getAccountGroupId()) @@ -209,6 +214,22 @@ public class ProjectControl { return false; } + /** + * @return the effective groups of the current user for this project + */ + private Set getEffectiveUserGroups() { + final Set userGroups = user.getEffectiveGroups(); + if (isOwner()) { + final Set userGroupsOnProject = + new HashSet(userGroups.size() + 1); + userGroupsOnProject.addAll(userGroups); + userGroupsOnProject.add(systemConfig.ownerGroupId); + return Collections.unmodifiableSet(userGroupsOnProject); + } else { + return userGroups; + } + } + private boolean canPerformOnAllRefs(ApprovalCategory.Id actionId, short requireValue) { boolean canPerform = false; @@ -238,10 +259,10 @@ public class ProjectControl { } public boolean canRunUploadPack() { - return isAnyIncludedIn(uploadGroups, user.getEffectiveGroups()); + return isAnyIncludedIn(uploadGroups, getEffectiveUserGroups()); } public boolean canRunReceivePack() { - return isAnyIncludedIn(receiveGroups, user.getEffectiveGroups()); + return isAnyIncludedIn(receiveGroups, getEffectiveUserGroups()); } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java index cf5d2643ed..6b0f13a62e 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java @@ -277,7 +277,8 @@ public class RefControlTest extends TestCase { return new RefControl(systemConfig, projectControl, ref); } }; - return new ProjectControl(Collections. emptySet(), + return new ProjectControl(systemConfig, + Collections. emptySet(), Collections. emptySet(), refControlFactory, new MockUser(memberOf), newProjectState()); } From c82e9e98b6d41dbd2267d72b96627a03c94739ab Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 3 Jun 2011 13:11:46 +0200 Subject: [PATCH 2/5] Do not reset Patch History selection on navigation to next file diff When the user views a file diff and then changes in the Patch History the selection of side A (left side), then this selection is lost when the user navigates to the previous or next file diff. The selection of side A is always reset to the patch set that is selected as 'Old Version History' on the ChangeScreen (the base for comparing patch sets). This change ensures that the selection in the Patch History is kept when the user navigates to the previous or next file diff. Bug: issue 999 Change-Id: I19cafedd8a11683dca4a24fbebbcacf63425fe62 Signed-off-by: Edwin Kempin --- .../java/com/google/gerrit/client/patches/PatchScreen.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java index 0cdfcc8fcc..c815097e16 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java @@ -479,6 +479,9 @@ public abstract class PatchScreen extends Screen implements public void setSideA(PatchSet.Id patchSetId) { idSideA = patchSetId; diffSideA = patchSetId; + if (fileList != null) { + fileList.setPatchSetIdToCompareWith(patchSetId); + } } public void setSideB(PatchSet.Id patchSetId) { From e313242c05197c6f8a62e44415e578e340962822 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Tue, 7 Jun 2011 10:10:36 -0700 Subject: [PATCH 3/5] Fix API breakage on ChangeDetailService The Mylyn Reviews project pointed out Gerrit changed the API signature of a JSON method they use remotely. Fix the API back to the original signature, and define a new method with the extra arguments. Change-Id: I51cbbdd64bdb72a666a6b5266db3b93494b75182 Signed-off-by: Shawn O. Pearce --- .../gerrit/common/data/ChangeDetailService.java | 4 +++- .../changes/PatchSetComplexDisclosurePanel.java | 4 ++-- .../gerrit/client/patches/PatchScreen.java | 6 +++--- .../changedetail/ChangeDetailServiceImpl.java | 12 ++++++++---- .../rpc/changedetail/PatchSetDetailFactory.java | 16 +++++++++------- 5 files changed, 25 insertions(+), 17 deletions(-) diff --git a/gerrit-common/src/main/java/com/google/gerrit/common/data/ChangeDetailService.java b/gerrit-common/src/main/java/com/google/gerrit/common/data/ChangeDetailService.java index 621f52df3a..2fbda21af9 100644 --- a/gerrit-common/src/main/java/com/google/gerrit/common/data/ChangeDetailService.java +++ b/gerrit-common/src/main/java/com/google/gerrit/common/data/ChangeDetailService.java @@ -29,7 +29,9 @@ public interface ChangeDetailService extends RemoteJsonService { void includedInDetail(Change.Id id, AsyncCallback callback); - void patchSetDetail(PatchSet.Id keyA, PatchSet.Id keyB, + void patchSetDetail(PatchSet.Id key, AsyncCallback callback); + + void patchSetDetail2(PatchSet.Id baseId, PatchSet.Id key, AccountDiffPreference diffPrefs, AsyncCallback callback); @SignInRequired diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java index 364d710c5f..740e08ce97 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/PatchSetComplexDisclosurePanel.java @@ -539,7 +539,7 @@ class PatchSetComplexDisclosurePanel extends ComplexDisclosurePanel implements O diffPrefs = patchTable.getPreferences().get(); } - Util.DETAIL_SVC.patchSetDetail(patchSet.getId(), diffBaseId, diffPrefs, + Util.DETAIL_SVC.patchSetDetail2(diffBaseId, patchSet.getId(), diffPrefs, new GerritCallback() { @Override public void onSuccess(PatchSetDetail result) { @@ -575,7 +575,7 @@ class PatchSetComplexDisclosurePanel extends ComplexDisclosurePanel implements O diffPrefs = new ListenableAccountDiffPreference().get(); } - Util.DETAIL_SVC.patchSetDetail(patchSet.getId(), diffBaseId, diffPrefs, + Util.DETAIL_SVC.patchSetDetail2(diffBaseId, patchSet.getId(), diffPrefs, new GerritCallback() { public void onSuccess(final PatchSetDetail result) { ensureLoaded(result); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java index c815097e16..736d1f012e 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/patches/PatchScreen.java @@ -320,7 +320,7 @@ public abstract class PatchScreen extends Screen implements protected void onLoad() { super.onLoad(); if (patchSetDetail == null) { - Util.DETAIL_SVC.patchSetDetail(idSideB, null, null, + Util.DETAIL_SVC.patchSetDetail(idSideB, new GerritCallback() { @Override public void onSuccess(PatchSetDetail result) { @@ -403,7 +403,7 @@ public abstract class PatchScreen extends Screen implements commitMessageBlock.display(patchSetDetail.getInfo().getMessage()); } else { commitMessageBlock.setVisible(false); - Util.DETAIL_SVC.patchSetDetail(idSideB, null, null, + Util.DETAIL_SVC.patchSetDetail(idSideB, new GerritCallback() { @Override public void onSuccess(PatchSetDetail result) { @@ -500,7 +500,7 @@ public abstract class PatchScreen extends Screen implements final PatchSet.Id psid = patchKey.getParentKey(); fileList = new PatchTable(prefs); fileList.setSavePointerId("PatchTable " + psid); - Util.DETAIL_SVC.patchSetDetail(psid, null, null, + Util.DETAIL_SVC.patchSetDetail(psid, new GerritCallback() { public void onSuccess(final PatchSetDetail result) { fileList.display(result); diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailServiceImpl.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailServiceImpl.java index f131123bab..fa0579e3d8 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailServiceImpl.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailServiceImpl.java @@ -52,10 +52,14 @@ class ChangeDetailServiceImpl implements ChangeDetailService { includedInDetail.create(id).to(callback); } - public void patchSetDetail(final PatchSet.Id idA, final PatchSet.Id idB, - final AccountDiffPreference diffPrefs, - final AsyncCallback callback) { - patchSetDetail.create(idA, idB, diffPrefs).to(callback); + public void patchSetDetail(PatchSet.Id id, + AsyncCallback callback) { + patchSetDetail2(null, id, null, callback); + } + + public void patchSetDetail2(PatchSet.Id baseId, PatchSet.Id id, + AccountDiffPreference diffPrefs, AsyncCallback callback) { + patchSetDetail.create(baseId, id, diffPrefs).to(callback); } public void patchSetPublishDetail(final PatchSet.Id id, diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java index 6865b2b0e7..f478d667a6 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetDetailFactory.java @@ -56,8 +56,10 @@ class PatchSetDetailFactory extends Handler { LoggerFactory.getLogger(PatchSetDetailFactory.class); interface Factory { - PatchSetDetailFactory create(@Assisted("psIdNew") PatchSet.Id psIdA, - @Assisted("psIdOld") PatchSet.Id psIdB, AccountDiffPreference diffPrefs); + PatchSetDetailFactory create( + @Assisted("psIdBase") @Nullable PatchSet.Id psIdBase, + @Assisted("psIdNew") PatchSet.Id psIdNew, + @Nullable AccountDiffPreference diffPrefs); } private final PatchSetInfoFactory infoFactory; @@ -66,8 +68,8 @@ class PatchSetDetailFactory extends Handler { private final ChangeControl.Factory changeControlFactory; private Project.NameKey projectKey; + private final PatchSet.Id psIdBase; private final PatchSet.Id psIdNew; - private final PatchSet.Id psIdOld; private final AccountDiffPreference diffPrefs; private ObjectId oldId; private ObjectId newId; @@ -80,16 +82,16 @@ class PatchSetDetailFactory extends Handler { PatchSetDetailFactory(final PatchSetInfoFactory psif, final ReviewDb db, final PatchListCache patchListCache, final ChangeControl.Factory changeControlFactory, + @Assisted("psIdBase") @Nullable final PatchSet.Id psIdBase, @Assisted("psIdNew") final PatchSet.Id psIdNew, - @Assisted("psIdOld") @Nullable final PatchSet.Id psIdOld, @Assisted @Nullable final AccountDiffPreference diffPrefs) { this.infoFactory = psif; this.db = db; this.patchListCache = patchListCache; this.changeControlFactory = changeControlFactory; + this.psIdBase = psIdBase; this.psIdNew = psIdNew; - this.psIdOld = psIdOld; this.diffPrefs = diffPrefs; } @@ -106,9 +108,9 @@ class PatchSetDetailFactory extends Handler { final PatchList list; - if (psIdOld != null) { + if (psIdBase != null) { + oldId = toObjectId(psIdBase); newId = toObjectId(psIdNew); - oldId = psIdOld != null ? toObjectId(psIdOld) : null; projectKey = control.getProject().getNameKey(); From 0a939db5bfff20d7622e5d8aee0e0f9cf1c9aa9e Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Tue, 7 Jun 2011 10:12:44 -0700 Subject: [PATCH 4/5] Release notes for 2.1.7.1 Change-Id: If75ca735e5bea95f136cd7dc0ebf73de582af37f Signed-off-by: Shawn O. Pearce --- ReleaseNotes/ReleaseNotes-2.1.7.1.txt | 32 +++++++++++++++++++++++++++ ReleaseNotes/index.txt | 1 + 2 files changed, 33 insertions(+) create mode 100644 ReleaseNotes/ReleaseNotes-2.1.7.1.txt diff --git a/ReleaseNotes/ReleaseNotes-2.1.7.1.txt b/ReleaseNotes/ReleaseNotes-2.1.7.1.txt new file mode 100644 index 0000000000..2d7622e5bb --- /dev/null +++ b/ReleaseNotes/ReleaseNotes-2.1.7.1.txt @@ -0,0 +1,32 @@ +Release notes for Gerrit 2.1.7.1 +================================ + +Gerrit 2.1.7.1 is now available: + +link:http://code.google.com/p/gerrit/downloads/detail?name=gerrit-2.1.7.1.war[http://code.google.com/p/gerrit/downloads/detail?name=gerrit-2.1.7.1.war] + +Bug Fixes +--------- +* issue 997 Resolve Project Owners when checking access rights ++ +Members of the 'Project Owners' magical group did not always have +their project owner privileges when Gerrit Code Review was looking +for "access to any ref" at the project level. This was caused by +not expanding the 'Project Owner's group to the actual ownership +list. Fixed. + +* issue 999 Do not reset Patch History selection on navigation ++ +Navigating to the next/previous file lost the setting of the +"Old Version" made under the "Patch History" expandable control +on a specific file view. This was accidentally broken when the +"Old Version History" control was added to the change page. Fixed. + +* Fix API breakage on ChangeDetailService ++ +Version 2.1.7 broke the Gerrit Code Review plugin for Mylyn Reviews +due to an accidential signature change of one of the remote JSON +APIs. The ChangeDetailService.patchSetDetail() method is back to the +old signature and a new patchSetDetail2() method has been added to +handle the newer calling convention used in some contexts of the +web UI. diff --git a/ReleaseNotes/index.txt b/ReleaseNotes/index.txt index 4e8cb2df7b..6f020049df 100644 --- a/ReleaseNotes/index.txt +++ b/ReleaseNotes/index.txt @@ -4,6 +4,7 @@ Gerrit Code Review - Release Notes [[2_1]] Version 2.1.x ------------- +* link:ReleaseNotes-2.1.7.1.html[2.1.7.1], * link:ReleaseNotes-2.1.7.html[2.1.7], * link:ReleaseNotes-2.1.6.html[2.1.6], link:ReleaseNotes-2.1.6.1.html[2.1.6.1] From 019d2c4dfb7351c43858b582ac32bb5e9b176c88 Mon Sep 17 00:00:00 2001 From: "Shawn O. Pearce" Date: Tue, 7 Jun 2011 11:47:22 -0700 Subject: [PATCH 5/5] Fix ChangeDetailFactory's invocation of PatchSetDetailFactory I flipped the order of the arguments, but did not correctly update all callers. Change-Id: Iae14ec35374342f53bc353082db35cb8750a750a Signed-off-by: Shawn O. Pearce --- .../{ReleaseNotes-2.1.7.1.txt => ReleaseNotes-2.1.7.2.txt} | 0 ReleaseNotes/index.txt | 2 +- .../gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java | 2 +- 3 files changed, 2 insertions(+), 2 deletions(-) rename ReleaseNotes/{ReleaseNotes-2.1.7.1.txt => ReleaseNotes-2.1.7.2.txt} (100%) diff --git a/ReleaseNotes/ReleaseNotes-2.1.7.1.txt b/ReleaseNotes/ReleaseNotes-2.1.7.2.txt similarity index 100% rename from ReleaseNotes/ReleaseNotes-2.1.7.1.txt rename to ReleaseNotes/ReleaseNotes-2.1.7.2.txt diff --git a/ReleaseNotes/index.txt b/ReleaseNotes/index.txt index 6f020049df..d340a09018 100644 --- a/ReleaseNotes/index.txt +++ b/ReleaseNotes/index.txt @@ -4,7 +4,7 @@ Gerrit Code Review - Release Notes [[2_1]] Version 2.1.x ------------- -* link:ReleaseNotes-2.1.7.1.html[2.1.7.1], +* link:ReleaseNotes-2.1.7.2.html[2.1.7.2], * link:ReleaseNotes-2.1.7.html[2.1.7], * link:ReleaseNotes-2.1.6.html[2.1.6], link:ReleaseNotes-2.1.6.1.html[2.1.6.1] diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java index 560b1552af..40166413d7 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java @@ -194,7 +194,7 @@ public class ChangeDetailFactory extends Handler { NoSuchEntityException, PatchSetInfoNotAvailableException, NoSuchChangeException { final PatchSet.Id psId = detail.getChange().currentPatchSetId(); - final PatchSetDetailFactory loader = patchSetDetail.create(psId, null, null); + final PatchSetDetailFactory loader = patchSetDetail.create(null, psId, null); loader.patchSet = detail.getCurrentPatchSet(); loader.control = control; detail.setCurrentPatchSetDetail(loader.call());