From e1225e0da4ce1ace15bbb3bff5d2039279fdbd7d Mon Sep 17 00:00:00 2001
From: Vishal Nayak <vishalnayak@users.noreply.github.com>
Date: Fri, 5 Apr 2019 09:12:39 -0400
Subject: [PATCH] Fix handling member group IDs (#6527)

* Process member_group_ids only if supplied
---
 vault/identity_store_groups_test.go | 60 +++++++++++++++++++++++++++++
 vault/identity_store_util.go        | 14 ++++++-
 2 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/vault/identity_store_groups_test.go b/vault/identity_store_groups_test.go
index d3043bf8a29f..4a6dc82dfa99 100644
--- a/vault/identity_store_groups_test.go
+++ b/vault/identity_store_groups_test.go
@@ -12,6 +12,66 @@ import (
 	"github.com/hashicorp/vault/logical"
 )
 
+func TestIdentityStore_FixOverwrittenMemberGroupIDs(t *testing.T) {
+	c, _, _ := TestCoreUnsealed(t)
+	ctx := namespace.RootContext(nil)
+
+	// Create a group
+	resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "group",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name": "group1",
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err: %v\nresp: %#v", err, resp)
+	}
+
+	groupID := resp.Data["id"].(string)
+
+	expectedMemberGroupIDs := []string{groupID}
+
+	// Create another group and add the above created group as its member
+	resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "group",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name":             "group2",
+			"policies":         "default",
+			"member_group_ids": expectedMemberGroupIDs,
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err: %v\nresp: %#v", err, resp)
+	}
+
+	// Create another group and add the above created group as its member
+	resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "group/name/group2",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"policies": "default,another-policy",
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err: %v\nresp: %#v", err, resp)
+	}
+
+	// Read the group and check if the member_group_ids is intact
+	resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{
+		Path:      "group/name/group2",
+		Operation: logical.ReadOperation,
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err: %v\nresp: %#v", err, resp)
+	}
+
+	if !reflect.DeepEqual(resp.Data["member_group_ids"], expectedMemberGroupIDs) {
+		t.Fatalf("bad: member_group_ids; expected: %#v\n, actual: %#v", expectedMemberGroupIDs, resp.Data["member_group_ids"])
+	}
+}
+
 func TestIdentityStore_GroupEntityMembershipUpgrade(t *testing.T) {
 	c, keys, rootToken := TestCoreUnsealed(t)
 
diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go
index 0e8341e1b190..484caaae8acb 100644
--- a/vault/identity_store_util.go
+++ b/vault/identity_store_util.go
@@ -1085,14 +1085,23 @@ func (i *IdentityStore) sanitizeAndUpsertGroup(ctx context.Context, group *ident
 	txn := i.db.Txn(true)
 	defer txn.Abort()
 
+	var currentMemberGroupIDs []string
+	var currentMemberGroups []*identity.Group
+
+	// If there are no member group IDs supplied, then it shouldn't be
+	// processed. If an empty set of member group IDs are supplied, then it
+	// should be processed. Hence the nil check instead of the length check.
+	if memberGroupIDs == nil {
+		goto ALIAS
+	}
+
 	memberGroupIDs = strutil.RemoveDuplicates(memberGroupIDs, false)
 
 	// For those group member IDs that are removed from the list, remove current
 	// group ID as their respective ParentGroupID.
 
 	// Get the current MemberGroups IDs for this group
-	var currentMemberGroupIDs []string
-	currentMemberGroups, err := i.MemDBGroupsByParentGroupID(group.ID, false)
+	currentMemberGroups, err = i.MemDBGroupsByParentGroupID(group.ID, false)
 	if err != nil {
 		return err
 	}
@@ -1183,6 +1192,7 @@ func (i *IdentityStore) sanitizeAndUpsertGroup(ctx context.Context, group *ident
 		}
 	}
 
+ALIAS:
 	// Sanitize the group alias
 	if group.Alias != nil {
 		group.Alias.CanonicalID = group.ID