Merge pull request #10002 from reasonerjt/groups-review-token-filter

populate group list when doing token review

src/core/auth/authproxy/auth.go

@@ -34,7 +34,6 @@ import (
 	"github.com/goharbor/harbor/src/core/auth"
 	"github.com/goharbor/harbor/src/core/config"
 	"github.com/goharbor/harbor/src/pkg/authproxy"
-	k8s_api_v1beta1 "k8s.io/api/authentication/v1beta1"
 )
 
 const refreshDuration = 2 * time.Second
@@ -101,31 +100,12 @@ func (a *Auth) Authenticate(m models.AuthModel) (*models.User, error) {
 		s := session{}
 		err = json.Unmarshal(data, &s)
 		if err != nil {
-			log.Errorf("failed to read session %v", err)
+			return nil, auth.NewErrAuth(fmt.Sprintf("failed to read session %v", err))
 		}
-
+		if err := a.tokenReview(s.SessionID, user); err != nil {
-		reviewResponse, err := a.tokenReview(s.SessionID)
+			return nil, auth.NewErrAuth(err.Error())
-		if err != nil {
-			return nil, err
-		}
-		if reviewResponse == nil {
-			return nil, auth.ErrAuth{}
-		}
-
-		// Attach user group ID information
-		ugList := reviewResponse.Status.User.Groups
-		log.Debugf("user groups %+v", ugList)
-		if len(ugList) > 0 {
-			userGroups := models.UserGroupsFromName(ugList, common.HTTPGroupType)
-			groupIDList, err := group.PopulateGroup(userGroups)
-			if err != nil {
-				return nil, err
-			}
-			log.Debugf("current user's group ID list is %+v", groupIDList)
-			user.GroupIDs = groupIDList
 		}
 		return user, nil
-
 	} else if resp.StatusCode == http.StatusUnauthorized {
 		return nil, auth.ErrAuth{}
 	} else {
@@ -134,17 +114,24 @@ func (a *Auth) Authenticate(m models.AuthModel) (*models.User, error) {
 			log.Warningf("Failed to read response body, error: %v", err)
 		}
 		return nil, fmt.Errorf("failed to authenticate, status code: %d, text: %s", resp.StatusCode, string(data))
-
+	}
 }
 
-}
+func (a *Auth) tokenReview(sessionID string, user *models.User) error {
-
-func (a *Auth) tokenReview(sessionID string) (*k8s_api_v1beta1.TokenReview, error) {
 	httpAuthProxySetting, err := config.HTTPAuthProxySetting()
 	if err != nil {
-		return nil, err
+		return err
 	}
-	return authproxy.TokenReview(sessionID, httpAuthProxySetting)
+	reviewStatus, err := authproxy.TokenReview(sessionID, httpAuthProxySetting)
+	if err != nil {
+		return err
+	}
+	u2, err := authproxy.UserFromReviewStatus(reviewStatus)
+	if err != nil {
+		return err
+	}
+	user.GroupIDs = u2.GroupIDs
+	return nil
 }
 
 // OnBoardUser delegates to dao pkg to insert/update data in DB.

src/core/filter/security.go

@@ -325,16 +325,16 @@ func (ap *authProxyReqCtxModifier) Modify(ctx *beegoctx.Context) bool {
 		log.Errorf("fail to get auth proxy settings, %v", err)
 		return false
 	}
-	tokenReviewResponse, err := authproxy.TokenReview(proxyPwd, httpAuthProxyConf)
+	tokenReviewStatus, err := authproxy.TokenReview(proxyPwd, httpAuthProxyConf)
 	if err != nil {
 		log.Errorf("fail to review token, %v", err)
 		return false
 	}
-
+	if rawUserName != tokenReviewStatus.User.Username {
-	if !tokenReviewResponse.Status.Authenticated {
+		log.Errorf("user name doesn't match with token: %s", rawUserName)
-		log.Errorf("fail to auth user: %s", rawUserName)
 		return false
 	}
+
 	user, err := dao.GetUser(models.User{
 		Username: rawUserName,
 	})
@@ -346,11 +346,12 @@ func (ap *authProxyReqCtxModifier) Modify(ctx *beegoctx.Context) bool {
 		log.Errorf("User: %s has not been on boarded yet.", rawUserName)
 		return false
 	}
-	if rawUserName != tokenReviewResponse.Status.User.Username {
+	u2, err := authproxy.UserFromReviewStatus(tokenReviewStatus)
-		log.Errorf("user name doesn't match with token: %s", rawUserName)
+	if err != nil {
+		log.Errorf("Failed to get user information from token review status, error: %v", err)
 		return false
 	}
-
+	user.GroupIDs = u2.GroupIDs
 	pm := config.GlobalProjectMgr
 	log.Debug("creating local database security context for auth proxy...")
 	securCtx := local.NewSecurityContext(user, pm)

src/pkg/authproxy/http.go

@@ -2,6 +2,9 @@ package authproxy
 
 import (
 	"encoding/json"
+	"fmt"
+	"github.com/goharbor/harbor/src/common"
+	"github.com/goharbor/harbor/src/common/dao/group"
 	"github.com/goharbor/harbor/src/common/models"
 	"github.com/goharbor/harbor/src/common/utils/log"
 	k8s_api_v1beta1 "k8s.io/api/authentication/v1beta1"
@@ -13,8 +16,9 @@ import (
 )
 
 // TokenReview ...
-func TokenReview(sessionID string, authProxyConfig *models.HTTPAuthProxy) (*k8s_api_v1beta1.TokenReview, error) {
+func TokenReview(rawToken string, authProxyConfig *models.HTTPAuthProxy) (k8s_api_v1beta1.TokenReviewStatus, error) {
 
+	emptyStatus := k8s_api_v1beta1.TokenReviewStatus{}
 	// Init auth client with the auth proxy endpoint.
 	authClientCfg := &rest.Config{
 		Host: authProxyConfig.TokenReviewEndpoint,
@@ -22,14 +26,14 @@ func TokenReview(sessionID string, authProxyConfig *models.HTTPAuthProxy) (*k8s_
 			GroupVersion:         &schema.GroupVersion{},
 			NegotiatedSerializer: serializer.DirectCodecFactory{CodecFactory: scheme.Codecs},
 		},
-		BearerToken: sessionID,
+		BearerToken: rawToken,
 		TLSClientConfig: rest.TLSClientConfig{
 			Insecure: !authProxyConfig.VerifyCert,
 		},
 	}
 	authClient, err := rest.RESTClientFor(authClientCfg)
 	if err != nil {
-		return nil, err
+		return emptyStatus, err
 	}
 
 	// Do auth with the token.
@@ -39,27 +43,49 @@ func TokenReview(sessionID string, authProxyConfig *models.HTTPAuthProxy) (*k8s_
 			APIVersion: "authentication.k8s.io/v1beta1",
 		},
 		Spec: k8s_api_v1beta1.TokenReviewSpec{
-			Token: sessionID,
+			Token: rawToken,
 		},
 	}
 	res := authClient.Post().Body(tokenReviewRequest).Do()
 	err = res.Error()
 	if err != nil {
 		log.Errorf("fail to POST auth request, %v", err)
-		return nil, err
+		return emptyStatus, err
 	}
 	resRaw, err := res.Raw()
 	if err != nil {
 		log.Errorf("fail to get raw data of token review, %v", err)
-		return nil, err
+		return emptyStatus, err
 	}
 	// Parse the auth response, check the user name and authenticated status.
 	tokenReviewResponse := &k8s_api_v1beta1.TokenReview{}
-	err = json.Unmarshal(resRaw, &tokenReviewResponse)
+	err = json.Unmarshal(resRaw, tokenReviewResponse)
 	if err != nil {
 		log.Errorf("fail to decode token review, %v", err)
-		return nil, err
+		return emptyStatus, err
 	}
-	return tokenReviewResponse, nil
+	return tokenReviewResponse.Status, nil
+
+}
+
+// UserFromReviewStatus transform a review status to a user model.
+// Group entries will be populated if needed.
+func UserFromReviewStatus(status k8s_api_v1beta1.TokenReviewStatus) (*models.User, error) {
+	if !status.Authenticated {
+		return nil, fmt.Errorf("failed to authenticate the token, error in status: %s", status.Error)
+	}
+	user := &models.User{
+		Username: status.User.Username,
+	}
+	if len(status.User.Groups) > 0 {
+		userGroups := models.UserGroupsFromName(status.User.Groups, common.HTTPGroupType)
+		groupIDList, err := group.PopulateGroup(userGroups)
+		if err != nil {
+			return nil, err
+		}
+		log.Debugf("current user's group ID list is %+v", groupIDList)
+		user.GroupIDs = groupIDList
+	}
+	return user, nil
 
 }

src/pkg/authproxy/http_test.go

@@ -0,0 +1,87 @@
+package authproxy
+
+import (
+	"github.com/goharbor/harbor/src/common/dao"
+	"github.com/goharbor/harbor/src/common/dao/group"
+	"github.com/stretchr/testify/assert"
+	"k8s.io/api/authentication/v1beta1"
+	"os"
+	"testing"
+)
+
+func TestMain(m *testing.M) {
+	dao.PrepareTestForPostgresSQL()
+	result := m.Run()
+	if result != 0 {
+		os.Exit(result)
+	}
+}
+
+func TestUserFromReviewStatus(t *testing.T) {
+	type result struct {
+		hasErr   bool
+		username string
+		groupLen int
+	}
+	cases := []struct {
+		input  v1beta1.TokenReviewStatus
+		expect result
+	}{
+		{
+			input: v1beta1.TokenReviewStatus{
+				Authenticated: false,
+				Error:         "connection error",
+			},
+			expect: result{
+				hasErr: true,
+			},
+		},
+		{
+			input: v1beta1.TokenReviewStatus{
+				Authenticated: true,
+				User: v1beta1.UserInfo{
+					Username: "jack",
+					UID:      "u-1",
+				},
+			},
+			expect: result{
+				hasErr:   false,
+				username: "jack",
+				groupLen: 0,
+			},
+		},
+		{
+			input: v1beta1.TokenReviewStatus{
+				Authenticated: true,
+				User: v1beta1.UserInfo{
+					Username: "daniel",
+					Groups:   []string{"group1", "group2"},
+				},
+				Error: "",
+			},
+			expect: result{
+				hasErr:   false,
+				username: "daniel",
+				groupLen: 2,
+			},
+		},
+	}
+	for _, c := range cases {
+		u, err := UserFromReviewStatus(c.input)
+		if c.expect.hasErr == true {
+			assert.NotNil(t, err)
+		} else {
+			assert.Nil(t, err)
+			assert.Equal(t, c.expect.username, u.Username)
+			assert.Equal(t, c.expect.groupLen, len(u.GroupIDs))
+		}
+		if u != nil {
+			for _, gid := range u.GroupIDs {
+				t.Logf("Deleting group %d", gid)
+				if err := group.DeleteUserGroup(gid); err != nil {
+					panic(err)
+				}
+			}
+		}
+	}
+}