From 12b63061c27557551f4d8aa19a3a1399083c10a0 Mon Sep 17 00:00:00 2001
From: Aditya Manthramurthy <donatello@users.noreply.github.com>
Date: Mon, 6 Dec 2021 15:55:11 -0800
Subject: [PATCH] Fix LDAP service account creation (#13849)

- when a user has only group permissions
- fixes regression from ac74237f0 (#13657)
- fixes https://github.com/minio/console/issues/1291
---
 cmd/admin-handlers-users.go |   3 ++
 cmd/sts-handlers_test.go    | 101 ++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+)

diff --git a/cmd/admin-handlers-users.go b/cmd/admin-handlers-users.go
index 77eaefd5c..524d1f76d 100644
--- a/cmd/admin-handlers-users.go
+++ b/cmd/admin-handlers-users.go
@@ -531,6 +531,7 @@ func (a adminAPIHandlers) AddServiceAccount(w http.ResponseWriter, r *http.Reque
 		// if there is no deny statement this call is implicitly enabled.
 		if !globalIAMSys.IsAllowed(iampolicy.Args{
 			AccountName:     requestorUser,
+			Groups:          requestorGroups,
 			Action:          iampolicy.CreateServiceAccountAdminAction,
 			ConditionValues: getConditionValues(r, "", cred.AccessKey, claims),
 			IsOwner:         owner,
@@ -564,10 +565,12 @@ func (a adminAPIHandlers) AddServiceAccount(w http.ResponseWriter, r *http.Reque
 		// user <> to the request sender
 		if !globalIAMSys.IsAllowed(iampolicy.Args{
 			AccountName:     requestorUser,
+			Groups:          requestorGroups,
 			Action:          iampolicy.CreateServiceAccountAdminAction,
 			ConditionValues: getConditionValues(r, "", cred.AccessKey, claims),
 			IsOwner:         owner,
 			Claims:          claims,
+			DenyOnly:        true,
 		}) {
 			writeErrorResponseJSON(ctx, w, errorCodes.ToAPIErr(ErrAccessDenied), r.URL)
 			return
diff --git a/cmd/sts-handlers_test.go b/cmd/sts-handlers_test.go
index aa9c562a6..bd2cdc73d 100644
--- a/cmd/sts-handlers_test.go
+++ b/cmd/sts-handlers_test.go
@@ -225,6 +225,7 @@ func TestIAMWithLDAPServerSuite(t *testing.T) {
 				suite.SetUpLDAP(c, ldapServer)
 				suite.TestLDAPSTS(c)
 				suite.TestLDAPSTSServiceAccounts(c)
+				suite.TestLDAPSTSServiceAccountsWithGroups(c)
 				suite.TearDownSuite(c)
 			},
 		)
@@ -443,6 +444,106 @@ func (s *TestSuiteIAM) TestLDAPSTSServiceAccounts(c *check) {
 	c.assertSvcAccDeletion(ctx, s, userAdmClient, value.AccessKeyID, bucket)
 }
 
+// In this test, the parent users gets their permissions from a group, rather
+// than having a policy set directly on them.
+func (s *TestSuiteIAM) TestLDAPSTSServiceAccountsWithGroups(c *check) {
+	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+	defer cancel()
+
+	bucket := getRandomBucketName()
+	err := s.client.MakeBucket(ctx, bucket, minio.MakeBucketOptions{})
+	if err != nil {
+		c.Fatalf("bucket create error: %v", err)
+	}
+
+	// Create policy
+	policy := "mypolicy"
+	policyBytes := []byte(fmt.Sprintf(`{
+ "Version": "2012-10-17",
+ "Statement": [
+  {
+   "Effect": "Allow",
+   "Action": [
+    "s3:PutObject",
+    "s3:GetObject",
+    "s3:ListBucket"
+   ],
+   "Resource": [
+    "arn:aws:s3:::%s/*"
+   ]
+  }
+ ]
+}`, bucket))
+	err = s.adm.AddCannedPolicy(ctx, policy, policyBytes)
+	if err != nil {
+		c.Fatalf("policy add error: %v", err)
+	}
+
+	groupDN := "cn=projecta,ou=groups,ou=swengg,dc=min,dc=io"
+	err = s.adm.SetPolicy(ctx, policy, groupDN, true)
+	if err != nil {
+		c.Fatalf("Unable to set policy: %v", err)
+	}
+
+	ldapID := cr.LDAPIdentity{
+		Client:       s.TestSuiteCommon.client,
+		STSEndpoint:  s.endPoint,
+		LDAPUsername: "dillon",
+		LDAPPassword: "dillon",
+	}
+
+	value, err := ldapID.Retrieve()
+	if err != nil {
+		c.Fatalf("Expected to generate STS creds, got err: %#v", err)
+	}
+
+	// Check that the LDAP sts cred is actually working.
+	minioClient, err := minio.New(s.endpoint, &minio.Options{
+		Creds:     cr.NewStaticV4(value.AccessKeyID, value.SecretAccessKey, value.SessionToken),
+		Secure:    s.secure,
+		Transport: s.TestSuiteCommon.client.Transport,
+	})
+	if err != nil {
+		c.Fatalf("Error initializing client: %v", err)
+	}
+
+	// Validate that the client from sts creds can access the bucket.
+	c.mustListObjects(ctx, minioClient, bucket)
+
+	// Create an madmin client with user creds
+	userAdmClient, err := madmin.NewWithOptions(s.endpoint, &madmin.Options{
+		Creds:  cr.NewStaticV4(value.AccessKeyID, value.SecretAccessKey, value.SessionToken),
+		Secure: s.secure,
+	})
+	if err != nil {
+		c.Fatalf("Err creating user admin client: %v", err)
+	}
+	userAdmClient.SetCustomTransport(s.TestSuiteCommon.client.Transport)
+
+	// Create svc acc
+	cr := c.mustCreateSvcAccount(ctx, value.AccessKeyID, userAdmClient)
+
+	// 1. Check that svc account appears in listing
+	c.assertSvcAccAppearsInListing(ctx, userAdmClient, value.AccessKeyID, cr.AccessKey)
+
+	// 2. Check that svc account info can be queried
+	c.assertSvcAccInfoQueryable(ctx, userAdmClient, value.AccessKeyID, cr.AccessKey, true)
+
+	// 3. Check S3 access
+	c.assertSvcAccS3Access(ctx, s, cr, bucket)
+
+	// 4. Check that svc account can restrict the policy, and that the
+	// session policy can be updated.
+	c.assertSvcAccSessionPolicyUpdate(ctx, s, userAdmClient, value.AccessKeyID, bucket)
+
+	// 4. Check that service account's secret key and account status can be
+	// updated.
+	c.assertSvcAccSecretKeyAndStatusUpdate(ctx, s, userAdmClient, value.AccessKeyID, bucket)
+
+	// 5. Check that service account can be deleted.
+	c.assertSvcAccDeletion(ctx, s, userAdmClient, value.AccessKeyID, bucket)
+}
+
 func (s *TestSuiteIAM) TestOpenIDSTS(c *check) {
 	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
 	defer cancel()