From b4606a06fd770f79cd8d33f5a5765044282ea459 Mon Sep 17 00:00:00 2001 From: Gustaf Lundh Date: Wed, 12 Jun 2013 17:02:08 +0200 Subject: [PATCH 1/4] Fixed: Draft patch sets are visible in diff screens If a change included drafts that were not visible to the user, the drafts were still being shown and were selectable in the header of the diff screens. The user was therefore able to view diffs including patch sets that he should not be able to see. Add checks to prevent non-visible drafts from being shown. Bug: Issue 1915 Change-Id: I89841e59c9f8171824919f847f18b3bd65e46d68 Signed-off-by: David Pursehouse Signed-off-by: Gustaf Lundh --- .../google/gerrit/httpd/rpc/patch/PatchScriptFactory.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptFactory.java index e0ec4654c1..797229c992 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptFactory.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchScriptFactory.java @@ -138,6 +138,11 @@ class PatchScriptFactory extends Handler { aId = psa != null ? toObjectId(db, psa) : null; bId = toObjectId(db, psb); + if ((psa != null && !control.isPatchVisible(db.patchSets().get(psa), db)) || + (psb != null && !control.isPatchVisible(db.patchSets().get(psb), db))) { + throw new NoSuchChangeException(changeId); + } + final Repository git; try { git = repoManager.openRepository(projectKey); @@ -232,6 +237,9 @@ class PatchScriptFactory extends Handler { // proper rename detection between the patch sets. // for (final PatchSet ps : db.patchSets().byChange(changeId)) { + if (!control.isPatchVisible(ps, db)) { + continue; + } String name = patchKey.get(); if (psa != null) { switch (changeType) { From 253611e965288654c69f0a77f2aa1b2f541bf53f Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 13 Jun 2013 09:09:11 +0200 Subject: [PATCH 2/4] Update the 2.6 release notes Change-Id: I1ac6435655646dcd1e1273e691525e95ebdf4740 --- ReleaseNotes/ReleaseNotes-2.6.txt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ReleaseNotes/ReleaseNotes-2.6.txt b/ReleaseNotes/ReleaseNotes-2.6.txt index 12986f61c0..2f065b88c1 100644 --- a/ReleaseNotes/ReleaseNotes-2.6.txt +++ b/ReleaseNotes/ReleaseNotes-2.6.txt @@ -1192,7 +1192,7 @@ but was coming up black on initial page load due to a style setup ordering issue between the SearchPanel and the HintTextBox. * link:https://code.google.com/p/gerrit/issues/detail?id=1661[Issue 1661]: - Update links to Change-Id and Signed-off-by docu on project info + Update links to Change-Id and Signed-off-by documentation on project info screen * Use href="javascript;" for All {Side-by-Side,Unified} links @@ -1232,6 +1232,9 @@ Correctly handle paths with URL-escaped characters URL-unescape the path portion of a change history token to correctly handle paths with URL-escapable characters, i.e. '+', ' ', etc. +* link:https://code.google.com/p/gerrit/issues/detail?id=1915[Issue 1915]: +Don't show non-visible drafts in the diff screens. + REST API ~~~~~~~~ From 5394b9fb1b16ebf5f21ff465bf70c311caf3d582 Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Sat, 8 Jun 2013 20:10:25 +0200 Subject: [PATCH 3/4] Only handle last value change event for attached change screens Bug: issue 1801 Change-Id: I2993a833d7547a234bdb6c36694f85913215b323 (cherry picked from 47d612e4fb0dc97b14553e1c048841a05e4baeaa) --- .../gerrit/client/changes/ChangeScreen.java | 18 ++++++++++++++++-- .../gerrit/client/ui/ListenableValue.java | 9 +++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeScreen.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeScreen.java index bee8ac48da..7983512898 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeScreen.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/changes/ChangeScreen.java @@ -83,6 +83,7 @@ public class ChangeScreen extends Screen private KeyCommandSet keysAction; private HandlerRegistration regNavigation; private HandlerRegistration regAction; + private HandlerRegistration regDetailCache; private Grid patchesGrid; private ListBox patchesList; @@ -127,6 +128,10 @@ public class ChangeScreen extends Screen regAction.removeHandler(); regAction = null; } + if (regDetailCache != null) { + regDetailCache.removeHandler(); + regDetailCache = null; + } super.onUnload(); } @@ -147,7 +152,7 @@ public class ChangeScreen extends Screen ChangeCache cache = ChangeCache.get(changeId); detailCache = cache.getChangeDetailCache(); - detailCache.addValueChangeHandler(this); + regDetailCache = detailCache.addValueChangeHandler(this); addStyleName(Gerrit.RESOURCES.css().changeScreen()); addStyleName(Gerrit.RESOURCES.css().screenNoHeader()); @@ -258,7 +263,7 @@ public class ChangeScreen extends Screen @Override public void onValueChange(final ValueChangeEvent event) { - if (isAttached()) { + if (isAttached() && isLastValueChangeHandler()) { // Until this screen is fully migrated to the new API, this call must be // sequential, because we can't start an async get at the source of every // call that might trigger a value change. @@ -274,6 +279,15 @@ public class ChangeScreen extends Screen } } + // Find the last attached screen. + // When DialogBox is used (i. e. CommentedActionDialog) then the original + // ChangeScreen is still in attached state. + // Use here the fact, that the handlers (ChangeScreen) are sorted. + private boolean isLastValueChangeHandler() { + int count = detailCache.getHandlerCount(); + return count > 0 && detailCache.getHandler(count - 1) == this; + } + private void display(final ChangeDetail detail) { displayTitle(detail.getChange().getKey(), detail.getChange().getSubject()); discardDiffBaseIfNotApplicable(detail.getChange().getId()); diff --git a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/ListenableValue.java b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/ListenableValue.java index 6dad875b60..d834eb21fb 100644 --- a/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/ListenableValue.java +++ b/gerrit-gwtui/src/main/java/com/google/gerrit/client/ui/ListenableValue.java @@ -45,4 +45,13 @@ public class ListenableValue implements HasValueChangeHandlers { ValueChangeHandler handler) { return manager.addHandler(ValueChangeEvent.getType(), handler); } + + public int getHandlerCount() { + return manager.getHandlerCount(ValueChangeEvent.getType()); + } + + public ValueChangeHandler getHandler(int index) { + return manager.getHandler(ValueChangeEvent.getType(), index); + } + } From c3c6e478c5dd0425e9f36762688da100fcc9005e Mon Sep 17 00:00:00 2001 From: Orgad Shaneh Date: Thu, 13 Jun 2013 14:45:49 +0300 Subject: [PATCH 4/4] Use user's full name even when email is not configured Change-Id: If29a9fc1e479c1c0413a43272c9f154dd59e8aba --- .../gerrit/server/mail/FromAddressGeneratorProvider.java | 9 +++++---- .../server/mail/FromAddressGeneratorProviderTest.java | 5 +++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/FromAddressGeneratorProvider.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/FromAddressGeneratorProvider.java index 86cebdce60..998fc3b0e5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/FromAddressGeneratorProvider.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/FromAddressGeneratorProvider.java @@ -98,10 +98,11 @@ public class FromAddressGeneratorProvider implements @Override public Address from(final Account.Id fromId) { if (fromId != null) { - final Account a = accountCache.get(fromId).getAccount(); - if (a.getPreferredEmail() != null) { - return new Address(a.getFullName(), a.getPreferredEmail()); - } + Account a = accountCache.get(fromId).getAccount(); + String userEmail = a.getPreferredEmail(); + return new Address( + a.getFullName(), + userEmail != null ? userEmail : srvAddr.getEmail()); } return srvAddr; } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/mail/FromAddressGeneratorProviderTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/mail/FromAddressGeneratorProviderTest.java index 29e3df0b14..2db2b6749c 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/mail/FromAddressGeneratorProviderTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/mail/FromAddressGeneratorProviderTest.java @@ -102,12 +102,13 @@ public class FromAddressGeneratorProviderTest extends TestCase { public void testUSER_NoPreferredEmailUser() { setFrom("USER"); - final Account.Id user = user("A U. Thor", null); + final String name = "A U. Thor"; + final Account.Id user = user(name, null); replay(accountCache); final Address r = create().from(user); assertNotNull(r); - assertEquals(ident.getName(), r.name); + assertEquals(name, r.name); assertEquals(ident.getEmailAddress(), r.email); verify(accountCache); }