From 01a9e9d4be44d8743f97ddcd232ce203b9b903ec Mon Sep 17 00:00:00 2001
From: Becky Siegel <beckysiegel@google.com>
Date: Thu, 22 Feb 2018 12:01:23 -0800
Subject: [PATCH] Show helpful message when adding group member 404s

Previosuly, 404 errors showed a generic error message, making it unclear
what could have gone wrong. For security reasons, we don't want people
to know if the group does not exist or if the group exists but the user
does not have permission. This change updates the error message to
reflect both of these possibilities.

Bug: Issue 8252
Change-Id: I6940d53b342a81ca10ec78977de4ec548fd10a6f
---
 .../admin/gr-group-members/gr-group-members.js | 13 ++++++++++++-
 .../gr-group-members_test.html                 | 18 ++++++++++++++++++
 .../gr-rest-api-interface.js                   | 10 +++++++---
 3 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/polygerrit-ui/app/elements/admin/gr-group-members/gr-group-members.js b/polygerrit-ui/app/elements/admin/gr-group-members/gr-group-members.js
index 1ce05b1649..5257e69c95 100644
--- a/polygerrit-ui/app/elements/admin/gr-group-members/gr-group-members.js
+++ b/polygerrit-ui/app/elements/admin/gr-group-members/gr-group-members.js
@@ -15,6 +15,8 @@
   'use strict';
 
   const SUGGESTIONS_LIMIT = 15;
+  const SAVING_ERROR_TEXT = 'Group may not exist, or you may not have '+
+      'permission to add it';
 
   const URL_REGEX = '^(?:[a-z]+:)?//';
 
@@ -186,7 +188,16 @@
 
     _handleSavingIncludedGroups() {
       return this.$.restAPI.saveIncludedGroup(this._groupName,
-          this._includedGroupSearch)
+          this._includedGroupSearch, err => {
+            if (err.status === 404) {
+              this.dispatchEvent(new CustomEvent('show-alert', {
+                detail: {message: SAVING_ERROR_TEXT},
+                bubbles: true,
+              }));
+              return err;
+            }
+            throw Error(err.statusText);
+          })
           .then(config => {
             if (!config) {
               return;
diff --git a/polygerrit-ui/app/elements/admin/gr-group-members/gr-group-members_test.html b/polygerrit-ui/app/elements/admin/gr-group-members/gr-group-members_test.html
index 194750effe..d670d4d546 100644
--- a/polygerrit-ui/app/elements/admin/gr-group-members/gr-group-members_test.html
+++ b/polygerrit-ui/app/elements/admin/gr-group-members/gr-group-members_test.html
@@ -183,6 +183,24 @@ limitations under the License.
       });
     });
 
+    test('add included group 404 shows helpful error text', () => {
+      element._groupOwner = true;
+
+      const memberName = 'bad-name';
+      const alertStub = sandbox.stub();
+      element.addEventListener('show-alert', alertStub);
+
+      sandbox.stub(element.$.restAPI, 'saveGroupMembers',
+          () => Promise.reject({status: 404}));
+
+      element.$.groupMemberSearchInput.text = memberName;
+      element.$.groupMemberSearchInput.value = 1234;
+
+      return element._handleSavingIncludedGroups().then(() => {
+        assert.isTrue(alertStub.called);
+      });
+    });
+
     test('_getAccountSuggestions empty', () => {
       return element._getAccountSuggestions('nonexistent').then(accounts => {
         assert.equal(accounts.length, 0);
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
index 6749437e0a..bcc16ad5b0 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
@@ -437,12 +437,16 @@
           .then(response => this.getResponseObject(response));
     },
 
-    saveIncludedGroup(groupName, includedGroup) {
+    saveIncludedGroup(groupName, includedGroup, opt_errFn) {
       const encodeName = encodeURIComponent(groupName);
       const encodeIncludedGroup = encodeURIComponent(includedGroup);
       return this.send('PUT',
-          `/groups/${encodeName}/groups/${encodeIncludedGroup}`)
-          .then(response => this.getResponseObject(response));
+          `/groups/${encodeName}/groups/${encodeIncludedGroup}`, null,
+          opt_errFn).then(response => {
+            if (response.ok) {
+              return this.getResponseObject(response);
+            }
+          });
     },
 
     deleteGroupMembers(groupName, groupMembers) {