Merge pull request #2406 from benjaminapetersen/bugzilla/1507730/serviceaccount-rolebinding-double-delete

Automatic merge from submit-queue.

Fix bugzilla 15077030 rolebindings for serviceaccounts

Fix bugzilla 15077030 where deleting a rolebinding for a serviceaccount can delete additional rolebindings for serviceaccounts from another namespace.

Hopefully will follow-up with some tests soon.  Time elapse is def showing some subtle bugs here.

Author: openshift-merge-robot

app/scripts/controllers/membership.js

@@ -274,7 +274,8 @@ angular
             project: project,
             subjectKinds: subjectKinds,
             canUpdateRolebindings: canI('rolebindings', 'update', projectName),
-            confirmRemove: function(subjectName, kindName, roleName) {
+            confirmRemove: function(subjectName, kindName, roleName, namespace) {
+
               var redirectToProjectList = null;
               var modalScope = createModalScope(subjectName, kindName, roleName, $scope.user.metadata.name);
               if(_.isEqual(subjectName, $scope.user.metadata.name)) {
@@ -294,7 +295,7 @@ angular
               })
               .result.then(function() {
                 RoleBindingsService
-                  .removeSubject(subjectName, roleName, $scope.roleBindings, requestContext)
+                  .removeSubject(subjectName, roleName, namespace, $scope.roleBindings, requestContext)
                   .then(function(updateRolebinding) {
                     if(redirectToProjectList) {
                       $location.url("./");

app/scripts/services/membership/roleBindings.js

@@ -87,20 +87,27 @@ angular
     };
 
     // has to handle multiple bindings or multiple reference to a subject within a single binding
-    var removeSubject = function(subjectName, role, roleBindings, context) {
-      var matches = _.filter(roleBindings, {roleRef: {name: role}});
+    var removeSubject = function(subjectName, role, namespace, roleBindings, context) {
+      var matchingBindings = _.filter(roleBindings, {roleRef: {name: role}});
+
       return $q.all(
-        _.map(matches, function(binding) {
+        _.map(matchingBindings, function(binding) {
           var tpl = bindingTPL();
           binding = _.extend(tpl, binding);
           cleanBinding(binding);
-          binding.subjects = _.reject(binding.subjects, {name: subjectName});
+          var toMatch = { name: subjectName };
+          if(namespace) {
+            toMatch.namespace = namespace;
+          }
+
+          binding.subjects = _.reject(binding.subjects, toMatch);
+
           return binding.subjects.length ?
                   DataService.update('rolebindings', binding.metadata.name, binding, context) :
                   DataService.delete('rolebindings', binding.metadata.name, context)
                   // For a delete, resp is simply a 201 or less useful object.
                   // Instead, this intercepts the response & returns the binding object
-                  // with the empty .subjects[] list. 
+                  // with the empty .subjects[] list.
                   .then(function() {
                     return binding;
                   });

app/views/membership.html

@@ -113,7 +113,7 @@ <h3>
                     key="role.metadata.name"
                     key-help="roleHelp(role)"
                     show-action="mode.edit"
-                    action="confirmRemove(subject.name, subjectKind.name, role.metadata.name)"
+                    action="confirmRemove(subject.name, subjectKind.name, role.metadata.name, subject.namespace)"
                     action-title="Remove role {{role.metadata.name}} from {{subject.name}}"></action-chip>
                 </div>
                 <div

dist/scripts/scripts.js

@@ -2514,17 +2514,19 @@ l.subjects.push(n);
 } else l.subjects = [ n ];
 return i(l), t.update("rolebindings", l.metadata.name, l, s);
 },
-removeSubject: function(n, a, o, s) {
-var c = _.filter(o, {
+removeSubject: function(n, a, o, s, c) {
+var l = _.filter(s, {
 roleRef: {
 name: a
 }
 });
-return e.all(_.map(c, function(e) {
+return e.all(_.map(l, function(e) {
 var a = r();
-return e = _.extend(a, e), i(e), e.subjects = _.reject(e.subjects, {
+e = _.extend(a, e), i(e);
+var s = {
 name: n
-}), e.subjects.length ? t.update("rolebindings", e.metadata.name, e, s) : t.delete("rolebindings", e.metadata.name, s).then(function() {
+};
+return o && (s.namespace = o), e.subjects = _.reject(e.subjects, s), e.subjects.length ? t.update("rolebindings", e.metadata.name, e, c) : t.delete("rolebindings", e.metadata.name, c).then(function() {
 return e;
 });
 }));
@@ -5218,20 +5220,20 @@ f = r, P(), k(f), angular.extend(a, {
 project: n,
 subjectKinds: E,
 canUpdateRolebindings: y("rolebindings", "update", g),
-confirmRemove: function(n, r, i) {
-var c = null, l = T(n, r, i, a.user.metadata.name);
-_.isEqual(n, a.user.metadata.name) && u.isLastRole(a.user.metadata.name, a.roleBindings) && (c = !0), o.open({
+confirmRemove: function(n, r, i, c) {
+var l = null, d = T(n, r, i, a.user.metadata.name);
+_.isEqual(n, a.user.metadata.name) && u.isLastRole(a.user.metadata.name, a.roleBindings) && (l = !0), o.open({
 animation: !0,
 templateUrl: "views/modals/confirm.html",
 controller: "ConfirmModalController",
 resolve: {
 modalConfig: function() {
-return l;
+return d;
 }
 }
 }).result.then(function() {
-m.removeSubject(n, i, a.roleBindings, f).then(function(e) {
-c ? t.url("./") : (s.getProjectRules(g, !0).then(function() {
+m.removeSubject(n, i, c, a.roleBindings, f).then(function(e) {
+l ? t.url("./") : (s.getProjectRules(g, !0).then(function() {
 P(e[0]);
 var t = y("rolebindings", "update", g);
 angular.extend(a, {

dist/scripts/templates.js

@@ -10703,7 +10703,7 @@ angular.module('openshiftConsoleTemplates', []).run(['$templateCache', function(
     "</div>\n" +
     "<div class=\"action-set\">\n" +
     "<div class=\"col-roles\">\n" +
-    "<action-chip ng-repeat=\"role in subject.roles\" key=\"role.metadata.name\" key-help=\"roleHelp(role)\" show-action=\"mode.edit\" action=\"confirmRemove(subject.name, subjectKind.name, role.metadata.name)\" action-title=\"Remove role {{role.metadata.name}} from {{subject.name}}\"></action-chip>\n" +
+    "<action-chip ng-repeat=\"role in subject.roles\" key=\"role.metadata.name\" key-help=\"roleHelp(role)\" show-action=\"mode.edit\" action=\"confirmRemove(subject.name, subjectKind.name, role.metadata.name, subject.namespace)\" action-title=\"Remove role {{role.metadata.name}} from {{subject.name}}\"></action-chip>\n" +
     "</div>\n" +
     "<div ng-if=\"mode.edit\" class=\"col-add-role\">\n" +
     "<div row>\n" +

test/spec/services/membership/roleBindingsSpec.js

@@ -88,10 +88,13 @@ describe('RoleBindingsService', function() {
     it('should update the rolebinding by removing the subject from the subject list', function() {
       var bindings = [{
         roleRef: { name: 'admin'},
-        subjects: [{name: 'jane', kind: 'user'}, {name: 'jack', kind: 'user'}]
+        subjects: [
+          {name: 'jane', kind: 'user'},
+          {name: 'jack', kind: 'user'}
+        ]
       }];
       RoleBindingsService
-        .removeSubject('jane', 'admin', bindings)
+        .removeSubject('jane', 'admin', null, bindings)
         .then(function(resp) {
           expect(resp[0].subjects).toEqual([{
               name: 'jack',
@@ -102,13 +105,61 @@ describe('RoleBindingsService', function() {
       $rootScope.$digest();
     });
 
+    it('should honor the namespace of a subject if provided', function() {
+      var bindings = [{
+        roleRef: { name: 'admin'},
+        subjects: [
+          // multiple jim, only one should be removed in test 1
+          {name: 'jim',  kind: 'user'},
+          {name: 'jim',  kind: 'user', namespace: 'yourproject'},
+          {name: 'jim',  kind: 'user', namespace: 'myproject'},
+          {name: 'jack', kind: 'user'}
+        ]
+      }, {
+        roleRef: { name: 'view'},
+        subjects: [
+          // multiple jane, only one should be removed in test 2
+          {name: 'jane', kind: 'user', namespace: 'myproject'},
+          {name: 'jack', kind: 'user', namespace: 'yourproject'},
+          {name: 'jane', kind: 'user', namespace: 'herproject'},
+          {name: 'jack', kind: 'user', namespace: 'hisproject'}
+        ]
+      }];
+
+      // test 1
+      RoleBindingsService
+        .removeSubject('jim', 'admin', 'myproject', bindings)
+        .then(function(resp) {
+          expect(resp[0].subjects).toEqual([
+            {name: 'jim',  kind: 'user'},
+            {name: 'jim',  kind: 'user', namespace: 'yourproject'},
+            {name: 'jack', kind: 'user'}
+          ]);
+
+        });
+
+      // test 2
+      RoleBindingsService
+        .removeSubject('jane', 'view', 'herproject', bindings)
+        .then(function(resp) {
+          expect(resp[0].subjects).toEqual([
+            {name: 'jane', kind: 'user', namespace: 'myproject'},
+            {name: 'jack', kind: 'user', namespace: 'yourproject'},
+            {name: 'jack', kind: 'user', namespace: 'hisproject'}
+          ]);
+        });
+      // resolve $q.all() via digest loop
+      $rootScope.$digest();
+
+    });
+
     it('should delete the rolebinding if the removed subject was the only subject', function() {
       var bindings = [{
         roleRef: { name: 'admin'},
         subjects: [{name: 'jane', kind: 'user'}]
       }];
       RoleBindingsService
-        .removeSubject('jane', 'admin', bindings)
+        .removeSubject('jane', 'admin', null, bindings)
         .then(function(resp) {
           expect(resp[0]).toBe(undefined);
         });
@@ -130,7 +181,7 @@ describe('RoleBindingsService', function() {
         subjects: [{name: 'jane', kind: 'user'}, {name: 'jack', kind: 'user'}]
       }];
       RoleBindingsService
-        .removeSubject('jane', 'admin', bindings)
+        .removeSubject('jane', 'admin', null, bindings)
         .then(function(resp) {
           _.each(resp, function(roleBinding) {
             expect(roleBinding.subjects).toEqual([ {name: 'jack', kind: 'user'}]);
@@ -154,7 +205,7 @@ describe('RoleBindingsService', function() {
         subjects: [{name: 'jane', kind: 'user'}]
       }];
       RoleBindingsService
-        .removeSubject('jane', 'admin', bindings)
+        .removeSubject('jane', 'admin', null, bindings)
         .then(function(resp) {
           expect(resp[0]).toBe(undefined);
           expect(resp[1].subjects).toEqual([ {name: 'jack', kind: 'user'}]);