Merge "CheckAccess: don't catch PermissionBackendException"
diff --git a/java/com/google/gerrit/server/permissions/DefaultRefFilter.java b/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
index 84a3d87..5905800 100644
--- a/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
+++ b/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
@@ -105,8 +105,7 @@
permissionBackend.user(user).database(db).project(projectState.getNameKey());
}
- Map<String, Ref> filter(Map<String, Ref> refs, Repository repo, RefFilterOptions opts)
- throws PermissionBackendException {
+ Map<String, Ref> filter(Map<String, Ref> refs, Repository repo, RefFilterOptions opts) {
if (projectState.isAllUsers()) {
refs = addUsersSelfSymref(refs);
}
@@ -283,7 +282,10 @@
.ref(visibleChanges.get(id).get())
.check(RefPermission.READ_PRIVATE_CHANGES);
return true;
- } catch (PermissionBackendException | AuthException e) {
+ } catch (AuthException e) {
+ return false;
+ } catch (PermissionBackendException e) {
+ log.error("Failed to check permission for " + id + " in " + projectState.getName(), e);
return false;
}
}
@@ -336,7 +338,7 @@
return r.notes();
}
} catch (PermissionBackendException e) {
- log.warn("Failed to check permission for " + r.id() + " in " + projectState.getName(), e);
+ log.error("Failed to check permission for " + r.id() + " in " + projectState.getName(), e);
}
return null;
}
diff --git a/java/com/google/gerrit/server/schema/GroupBundle.java b/java/com/google/gerrit/server/schema/GroupBundle.java
index 302ea55..3c1c409 100644
--- a/java/com/google/gerrit/server/schema/GroupBundle.java
+++ b/java/com/google/gerrit/server/schema/GroupBundle.java
@@ -195,7 +195,7 @@
Timestamp createdOn = rs.getTimestamp(3);
String description = rs.getString(4);
AccountGroup.UUID ownerGroupUuid = new AccountGroup.UUID(rs.getString(5));
- boolean visibleToAll = rs.getBoolean(6);
+ boolean visibleToAll = "Y".equals(rs.getString(6));
AccountGroup group = new AccountGroup(groupName, groupId, groupUuid, createdOn);
group.setDescription(description);
diff --git a/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.js b/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.js
index ff6f2d6..20d4c43 100644
--- a/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.js
+++ b/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view.js
@@ -42,6 +42,8 @@
view: 'gr-plugin-list',
}];
+ const INTERNAL_GROUP_REGEX = /^[\da-f]{40}$/;
+
const ACCOUNT_CAPABILITIES = ['createProject', 'createGroup', 'viewPlugins'];
Polymer({
@@ -58,6 +60,7 @@
type: Number,
observer: '_computeGroupName',
},
+ _groupIsInternal: Boolean,
_groupName: String,
_groupOwner: {
type: Boolean,
@@ -160,16 +163,17 @@
name: this._groupName,
view: Gerrit.Nav.View.GROUP,
url: Gerrit.Nav.getUrlForGroup(this._groupId),
- children: [
- {
- name: 'Members',
- detailType: Gerrit.Nav.GroupDetailView.MEMBERS,
- view: Gerrit.Nav.View.GROUP,
- url: Gerrit.Nav.getUrlForGroupMembers(this._groupId),
- },
- ],
+ children: [],
};
- if (this._isAdmin || this._groupOwner) {
+ if (this._groupIsInternal) {
+ linkCopy.subsection.children.push({
+ name: 'Members',
+ detailType: Gerrit.Nav.GroupDetailView.MEMBERS,
+ view: Gerrit.Nav.View.GROUP,
+ url: Gerrit.Nav.getUrlForGroupMembers(this._groupId),
+ });
+ }
+ if (this._groupIsInternal && (this._isAdmin || this._groupOwner)) {
linkCopy.subsection.children.push(
{
name: 'Audit Log',
@@ -295,6 +299,7 @@
if (!group || !group.name) { return; }
this._groupName = group.name;
+ this._groupIsInternal = !!group.id.match(INTERNAL_GROUP_REGEX);
this.reload();
promises.push(this.$.restAPI.getIsAdmin().then(isAdmin => {
diff --git a/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view_test.html b/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view_test.html
index a488245..4cdf8c8 100644
--- a/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view_test.html
+++ b/polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view_test.html
@@ -312,6 +312,7 @@
sandbox.stub(element.$.restAPI, 'getGroupConfig')
.returns(Promise.resolve({
name: 'foo',
+ id: 'c0f83e941ce90caea30e6ad88f0d4ea0e841a7a9',
}));
sandbox.stub(element.$.restAPI, 'getIsGroupOwner')
.returns(Promise.resolve(true));
@@ -329,7 +330,7 @@
assert.equal(selected.textContent.trim(), 'Groups');
});
- test('group', () => {
+ test('internal group', () => {
element.params = {
view: Gerrit.Nav.View.GROUP,
groupId: 1234,
@@ -337,6 +338,34 @@
element._groupName = 'foo';
return element.reload().then(() => {
flushAsynchronousOperations();
+ const subsectionItems = Polymer.dom(element.root)
+ .querySelectorAll('.subsectionItem');
+ assert.equal(subsectionItems.length, 2);
+ assert.isTrue(element._groupIsInternal);
+ const selected = element.$$('gr-page-nav .selected');
+ assert.isOk(selected);
+ assert.equal(selected.textContent.trim(), 'foo');
+ });
+ });
+
+ test('external group', () => {
+ element.$.restAPI.getGroupConfig.restore();
+ sandbox.stub(element.$.restAPI, 'getGroupConfig')
+ .returns(Promise.resolve({
+ name: 'foo',
+ id: 'external-id',
+ }));
+ element.params = {
+ view: Gerrit.Nav.View.GROUP,
+ groupId: 1234,
+ };
+ element._groupName = 'foo';
+ return element.reload().then(() => {
+ flushAsynchronousOperations();
+ const subsectionItems = Polymer.dom(element.root)
+ .querySelectorAll('.subsectionItem');
+ assert.equal(subsectionItems.length, 0);
+ assert.isFalse(element._groupIsInternal);
const selected = element.$$('gr-page-nav .selected');
assert.isOk(selected);
assert.equal(selected.textContent.trim(), 'foo');
diff --git a/polygerrit-ui/app/elements/admin/gr-group/gr-group.html b/polygerrit-ui/app/elements/admin/gr-group/gr-group.html
index 21978d5..e9b7aa3 100644
--- a/polygerrit-ui/app/elements/admin/gr-group/gr-group.html
+++ b/polygerrit-ui/app/elements/admin/gr-group/gr-group.html
@@ -72,9 +72,9 @@
<gr-autocomplete
id="groupNameInput"
text="{{_groupConfig.name}}"
- disabled="[[_computeGroupDisabled(_groupOwner, _isAdmin)]]"></gr-autocomplete>
+ disabled="[[_computeGroupDisabled(_groupOwner, _isAdmin, _groupIsInternal)]]"></gr-autocomplete>
</span>
- <span class="value" disabled$="[[_computeGroupDisabled(_groupOwner, _isAdmin)]]">
+ <span class="value" disabled$="[[_computeGroupDisabled(_groupOwner, _isAdmin, _groupIsInternal)]]">
<gr-button
id="inputUpdateNameBtn"
on-tap="_handleSaveName"
@@ -90,10 +90,10 @@
<gr-autocomplete
text="{{_groupConfig.owner}}"
query="[[_query]]"
- disabled="[[_computeGroupDisabled(_groupOwner, _isAdmin)]]">
+ disabled="[[_computeGroupDisabled(_groupOwner, _isAdmin, _groupIsInternal)]]">
</gr-autocomplete>
</span>
- <span class="value" disabled$="[[_computeGroupDisabled(_groupOwner, _isAdmin)]]">
+ <span class="value" disabled$="[[_computeGroupDisabled(_groupOwner, _isAdmin, _groupIsInternal)]]">
<gr-button
on-tap="_handleSaveOwner"
disabled="[[!_owner]]">
@@ -109,9 +109,9 @@
class="description"
autocomplete="on"
bind-value="{{_groupConfig.description}}"
- disabled="[[_computeGroupDisabled(_groupOwner, _isAdmin)]]"></iron-autogrow-textarea>
+ disabled="[[_computeGroupDisabled(_groupOwner, _isAdmin, _groupIsInternal)]]"></iron-autogrow-textarea>
</div>
- <span class="value" disabled$="[[_computeGroupDisabled(_groupOwner, _isAdmin)]]">
+ <span class="value" disabled$="[[_computeGroupDisabled(_groupOwner, _isAdmin, _groupIsInternal)]]">
<gr-button
on-tap="_handleSaveDescription"
disabled="[[!_description]]">
@@ -131,7 +131,7 @@
<gr-select
id="visibleToAll"
bind-value="{{_groupConfig.options.visible_to_all}}">
- <select disabled$="[[_computeGroupDisabled(_groupOwner, _isAdmin)]]">
+ <select disabled$="[[_computeGroupDisabled(_groupOwner, _isAdmin, _groupIsInternal)]]">
<template is="dom-repeat" items="[[_submitTypes]]">
<option value="[[item.value]]">[[item.label]]</option>
</template>
@@ -139,7 +139,7 @@
</gr-select>
</span>
</section>
- <span class="value" disabled$="[[_computeGroupDisabled(_groupOwner, _isAdmin)]]">
+ <span class="value" disabled$="[[_computeGroupDisabled(_groupOwner, _isAdmin, _groupIsInternal)]]">
<gr-button
on-tap="_handleSaveOptions"
disabled="[[!_options]]">
diff --git a/polygerrit-ui/app/elements/admin/gr-group/gr-group.js b/polygerrit-ui/app/elements/admin/gr-group/gr-group.js
index 1e16720..5c02691 100644
--- a/polygerrit-ui/app/elements/admin/gr-group/gr-group.js
+++ b/polygerrit-ui/app/elements/admin/gr-group/gr-group.js
@@ -17,6 +17,8 @@
(function() {
'use strict';
+ const INTERNAL_GROUP_REGEX = /^[\da-f]{40}$/;
+
const OPTIONS = {
submitFalse: {
value: false,
@@ -43,6 +45,7 @@
type: Boolean,
value: false,
},
+ _groupIsInternal: Boolean,
_description: {
type: Boolean,
value: false,
@@ -109,6 +112,7 @@
if (!config || !config.name) { return Promise.resolve(); }
this._groupName = config.name;
+ this._groupIsInternal = !!config.id.match(INTERNAL_GROUP_REGEX);
promises.push(this.$.restAPI.getIsAdmin().then(isAdmin => {
this._isAdmin = isAdmin ? true : false;
@@ -149,7 +153,8 @@
.then(config => {
if (config.status === 200) {
this._groupName = this._groupConfig.name;
- this.fire('name-changed', {name: this._groupConfig.name});
+ this.fire('name-changed', {name: this._groupConfig.name,
+ external: this._groupIsExtenral});
this._rename = false;
}
});
@@ -222,8 +227,8 @@
});
},
- _computeGroupDisabled(owner, admin) {
- return admin || owner ? false : true;
+ _computeGroupDisabled(owner, admin, groupIsInternal) {
+ return groupIsInternal && (admin || owner) ? false : true;
},
});
})();
diff --git a/polygerrit-ui/app/elements/admin/gr-group/gr-group_test.html b/polygerrit-ui/app/elements/admin/gr-group/gr-group_test.html
index 4ee7414a..a0fc71a 100644
--- a/polygerrit-ui/app/elements/admin/gr-group/gr-group_test.html
+++ b/polygerrit-ui/app/elements/admin/gr-group/gr-group_test.html
@@ -40,8 +40,7 @@
const group = {
id: '6a1e70e1a88782771a91808c8af9bbb7a9871389',
url: '#/admin/groups/uuid-6a1e70e1a88782771a91808c8af9bbb7a9871389',
- options: {
- },
+ options: {},
description: 'Gerrit Site Administrators',
group_id: 1,
owner: 'Administrators',
@@ -72,12 +71,31 @@
.display === 'none');
});
- test('default values are populated', done => {
+ test('default values are populated with internal group', done => {
sandbox.stub(element.$.restAPI, 'getIsGroupOwner', () => {
return Promise.resolve(true);
});
element.groupId = 1;
element._loadGroup().then(() => {
+ assert.isTrue(element._groupIsInternal);
+ assert.isFalse(element.$.visibleToAll.bindValue);
+ done();
+ });
+ });
+
+ test('default values with external group', done => {
+ const groupExternal = Object.assign({}, group);
+ groupExternal.id = 'external-group-id';
+ groupStub.restore();
+ groupStub = sandbox.stub(element.$.restAPI, 'getGroupConfig', () => {
+ return Promise.resolve(groupExternal);
+ });
+ sandbox.stub(element.$.restAPI, 'getIsGroupOwner', () => {
+ return Promise.resolve(true);
+ });
+ element.groupId = 1;
+ element._loadGroup().then(() => {
+ assert.isFalse(element._groupIsInternal);
assert.isFalse(element.$.visibleToAll.bindValue);
done();
});
@@ -161,28 +179,32 @@
});
});
- test('_computeGroupDisabled return false for admin', () => {
- const admin = true;
- const owner = false;
- assert.equal(element._computeGroupDisabled(owner, admin), false);
- });
+ test('_computeGroupDisabled', () => {
+ let admin = true;
+ let owner = false;
+ let groupIsInternal = true;
+ assert.equal(element._computeGroupDisabled(owner, admin,
+ groupIsInternal), false);
- test('_computeGroupDisabled return true for admin', () => {
- const admin = false;
- const owner = false;
- assert.equal(element._computeGroupDisabled(owner, admin), true);
- });
+ admin = false;
+ assert.equal(element._computeGroupDisabled(owner, admin,
+ groupIsInternal), true);
- test('_computeGroupDisabled return false for owner', () => {
- const admin = false;
- const owner = true;
- assert.equal(element._computeGroupDisabled(owner, admin), false);
- });
+ owner = true;
+ assert.equal(element._computeGroupDisabled(owner, admin,
+ groupIsInternal), false);
- test('_computeGroupDisabled return true for owner', () => {
- const admin = false;
- const owner = false;
- assert.equal(element._computeGroupDisabled(owner, admin), true);
+ owner = false;
+ assert.equal(element._computeGroupDisabled(owner, admin,
+ groupIsInternal), true);
+
+ groupIsInternal = false;
+ assert.equal(element._computeGroupDisabled(owner, admin,
+ groupIsInternal), true);
+
+ admin = true;
+ assert.equal(element._computeGroupDisabled(owner, admin,
+ groupIsInternal), true);
});
test('_computeLoadingClass', () => {
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
index 42404aa..ff4562b 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.html
@@ -611,6 +611,7 @@
on-send="_handleReplySent"
on-cancel="_handleReplyCancel"
on-autogrow="_handleReplyAutogrow"
+ on-send-disabled-changed="_resetReplyOverlayFocusStops"
hidden$="[[!_loggedIn]]">
</gr-reply-dialog>
</gr-overlay>
diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
index 9be7db4..5c6859d 100644
--- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
+++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
@@ -999,7 +999,7 @@
*/
_openReplyDialog(opt_section) {
this.$.replyOverlay.open().then(() => {
- this.$.replyOverlay.setFocusStops(this.$.replyDialog.getFocusStops());
+ this._resetReplyOverlayFocusStops();
this.$.replyDialog.open(opt_section);
Polymer.dom.flush();
this.$.replyOverlay.center();
@@ -1554,5 +1554,9 @@
_handleStopEditTap() {
Gerrit.Nav.navigateToChange(this._change, this._patchRange.patchNum);
},
+
+ _resetReplyOverlayFocusStops() {
+ this.$.replyOverlay.setFocusStops(this.$.replyDialog.getFocusStops());
+ },
});
})();
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.html b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.html
index 0cfae94..af3acc4 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.html
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.html
@@ -115,7 +115,7 @@
sandbox.restore();
});
- test('send blocked when invalid email is supplied to ccs', () => {
+ test('_submit blocked when invalid email is supplied to ccs', () => {
const sendStub = sandbox.stub(element, 'send').returns(Promise.resolve());
// Stub the below function to avoid side effects from the send promise
// resolving.
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html
index 088875f..8225dac 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.html
@@ -64,6 +64,8 @@
display: flex;
justify-content: space-between;
position: sticky;
+ /* @see Issue 8602 */
+ z-index: 1;
}
.actions .right gr-button {
margin-left: 1em;
@@ -291,9 +293,10 @@
class="action cancel"
on-tap="_cancelTapHandler">Cancel</gr-button>
<gr-button
+ id="sendButton"
link
primary
- disabled="[[_computeSendButtonDisabled(_sendButtonLabel, diffDrafts, draft, _reviewersMutated, _labelsChanged, _includeComments)]]"
+ disabled="[[_sendDisabled]]"
class="action send"
has-tooltip
title$="[[_computeSendButtonTooltip(canBeStarted)]]"
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
index 3875e25..e8c720a 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.js
@@ -52,6 +52,8 @@
// googlesource.com.
const START_REVIEW_MESSAGE = 'This change is ready for review.';
+ const EMPTY_REPLY_MESSAGE = 'Cannot send an empty reply.';
+
Polymer({
is: 'gr-reply-dialog',
@@ -87,6 +89,12 @@
* @event comment-refresh
*/
+ /**
+ * Fires when the state of the send button (enabled/disabled) changes.
+ *
+ * @event send-disabled-changed
+ */
+
properties: {
/**
* @type {{ _number: number, removable_reviewers: Array }}
@@ -206,6 +214,12 @@
type: String,
value: '',
},
+ _sendDisabled: {
+ type: Boolean,
+ computed: '_computeSendButtonDisabled(_sendButtonLabel, diffDrafts, ' +
+ 'draft, _reviewersMutated, _labelsChanged, _includeComments)',
+ observer: '_sendDisabledChanged',
+ },
},
FocusTarget,
@@ -273,9 +287,10 @@
},
getFocusStops() {
+ const end = this._sendDisabled ? this.$.cancelButton : this.$.sendButton;
return {
start: this.$.reviewers.focusStart,
- end: this.$.sendButton,
+ end,
};
},
@@ -726,6 +741,13 @@
// the text field of the CC entry.
return;
}
+ if (this._sendDisabled) {
+ this.dispatchEvent(new CustomEvent('show-alert', {
+ bubbles: true,
+ detail: {message: EMPTY_REPLY_MESSAGE},
+ }));
+ return;
+ }
return this.send(this._includeComments, this.canBeStarted)
.then(keepReviewers => {
this._purgeReviewersPendingRemove(false, keepReviewers);
@@ -856,5 +878,9 @@
setPluginMessage(message) {
this._pluginMessage = message;
},
+
+ _sendDisabledChanged(sendDisabled) {
+ this.dispatchEvent(new CustomEvent('send-disabled-changed'));
+ },
});
})();
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html
index b9eebdc..41d7b49 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog_test.html
@@ -1100,6 +1100,33 @@
assert.isFalse(fn('Send', {}, '', false, true, false));
});
+ test('_submit blocked when no mutations exist', () => {
+ const sendStub = sandbox.stub(element, 'send').returns(Promise.resolve());
+ // Stub the below function to avoid side effects from the send promise
+ // resolving.
+ sandbox.stub(element, '_purgeReviewersPendingRemove');
+ element.diffDrafts = {};
+ flushAsynchronousOperations();
+
+ MockInteractions.tap(element.$$('gr-button.send'));
+ assert.isFalse(sendStub.called);
+
+ element.diffDrafts = {test: true};
+ flushAsynchronousOperations();
+
+ MockInteractions.tap(element.$$('gr-button.send'));
+ assert.isTrue(sendStub.called);
+ });
+
+ test('getFocusStops', () => {
+ // Setting diffDrafts to an empty object causes _sendDisabled to be
+ // computed to false.
+ element.diffDrafts = {};
+ assert.equal(element.getFocusStops().end, element.$.cancelButton);
+ element.diffDrafts = {test: true};
+ assert.equal(element.getFocusStops().end, element.$.sendButton);
+ });
+
test('setPluginMessage', () => {
element.setPluginMessage('foo');
assert.equal(element.$.pluginMessage.textContent, 'foo');
