feat(teammemberships): refactor SCIM pkg (#792)

* (chore): regenerate mocks * (chore): refactor SCIM pkg * (chore): refactor SCIM pkg * (chore): update scim usage in org and team membership * (chore): update unit tests * (chore): fix unused args * (chore): tidy up! * (chore): ignore lint errors * (chore): fix code editor refactoring * (chore): implicit pagination for groups and users * Apply suggestions from code review Co-authored-by: IvoGoman <[email protected]> * (chore): simplify scim usage organization controller checks for group exists with len check and corresponding not available condition * (chore): fix lint * (chore): fix fmt --------- Co-authored-by: IvoGoman <[email protected]>
cloudoperators · Dec 13, 2024 · 399513f · 399513f
1 parent 7e98462
commit 399513f
Show file tree

Hide file tree

Showing 13 changed files with 921 additions and 892 deletions.
diff --git a/go.mod b/go.mod
@@ -37,7 +37,6 @@ require (
 	go.uber.org/zap v1.27.0
 	golang.org/x/text v0.20.0
 	gopkg.in/yaml.v3 v3.0.1
-	gotest.tools/v3 v3.5.1
 	helm.sh/helm/v3 v3.15.4
 	k8s.io/api v0.31.2
 	k8s.io/apiextensions-apiserver v0.31.2
@@ -83,6 +82,7 @@ require (
 	go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.54.0 // indirect
 	google.golang.org/genproto/googleapis/api v0.0.0-20240827150818-7e3bb234dfed // indirect
 	gopkg.in/evanphx/json-patch.v4 v4.12.0 // indirect
+	gotest.tools/v3 v3.5.1 // indirect
 )
 
 require (

diff --git a/pkg/controllers/organization/organization_controller.go b/pkg/controllers/organization/organization_controller.go
@@ -265,23 +265,34 @@ func (r *OrganizationReconciler) checkSCIMAPIAvailability(ctx context.Context, o
 	if err != nil {
 		return greenhousesapv1alpha1.FalseCondition(greenhousesapv1alpha1.SCIMAPIAvailableCondition, greenhousesapv1alpha1.SecretNotFoundReason, "BasicAuthPw missing")
 	}
-	clientConfig := scim.Config{
-		RawURL:   scimConfig.BaseURL,
+	config := &scim.Config{
+		URL:      scimConfig.BaseURL,
 		AuthType: scim.Basic,
-		BasicAuthConfig: &scim.BasicAuthConfig{
-			BasicAuthUser: basicAuthUser,
-			BasicAuthPw:   basicAuthPw,
+		BasicAuth: &scim.BasicAuthConfig{
+			Username: basicAuthUser,
+			Password: basicAuthPw,
 		},
 	}
-	scimClient, err := scim.NewScimClient(clientConfig)
+	logger := ctrl.LoggerFrom(ctx)
+	scimClient, err := scim.NewSCIMClient(logger, config)
 	if err != nil {
 		return greenhousesapv1alpha1.FalseCondition(greenhousesapv1alpha1.SCIMAPIAvailableCondition, greenhousesapv1alpha1.SCIMRequestFailedReason, "Failed to create SCIM client")
 	}
 
-	_, err = scimClient.GetTeamMembers(org.Spec.MappedOrgAdminIDPGroup)
+	// verify that the SCIM API can be accessed
+	opts := &scim.QueryOptions{
+		Filter:             scim.GroupFilterByDisplayName(org.Spec.MappedOrgAdminIDPGroup),
+		ExcludedAttributes: scim.SetAttributes(scim.AttrMembers),
+	}
+
+	groups, err := scimClient.GetGroups(ctx, opts)
 	if err != nil {
+		logger.Error(err, "Failed to request data from SCIM API")
 		return greenhousesapv1alpha1.FalseCondition(greenhousesapv1alpha1.SCIMAPIAvailableCondition, greenhousesapv1alpha1.SCIMRequestFailedReason, "Failed to request data from SCIM API")
 	}
+	if len(groups) == 0 {
+		return greenhousesapv1alpha1.FalseCondition(greenhousesapv1alpha1.SCIMAPIAvailableCondition, greenhousesapv1alpha1.SCIMRequestFailedReason, org.Spec.MappedOrgAdminIDPGroup+" Group not found in SCIM API")
+	}
 
 	return greenhousesapv1alpha1.TrueCondition(greenhousesapv1alpha1.SCIMAPIAvailableCondition, lifecycle.CreatedReason, "SCIM API is available")
 }

diff --git a/pkg/controllers/organization/organization_controller_test.go b/pkg/controllers/organization/organization_controller_test.go
@@ -146,7 +146,7 @@ var _ = Describe("Test Organization reconciliation", Ordered, func() {
 				func(o *greenhousev1alpha1.Organization) {
 					o.Spec.Authentication = &greenhousev1alpha1.Authentication{
 						SCIMConfig: &greenhousev1alpha1.SCIMConfig{
-							BaseURL: groupsServer.URL,
+							BaseURL: groupsServer.URL + "/scim",
 							BasicAuthUser: greenhousev1alpha1.ValueFromSource{
 								Secret: &greenhousev1alpha1.SecretKeyReference{
 									Name: "test-secret",
@@ -199,7 +199,7 @@ var _ = Describe("Test Organization reconciliation", Ordered, func() {
 				g.Expect(err).ToNot(HaveOccurred(), "there should be no error getting the Organization")
 				testOrg.Spec.Authentication = &greenhousev1alpha1.Authentication{
 					SCIMConfig: &greenhousev1alpha1.SCIMConfig{
-						BaseURL: groupsServer.URL,
+						BaseURL: groupsServer.URL + "/scim",
 						BasicAuthUser: greenhousev1alpha1.ValueFromSource{
 							Secret: &greenhousev1alpha1.SecretKeyReference{
 								Name: "test-secret",

diff --git a/pkg/controllers/teammembership/suite_test.go b/pkg/controllers/teammembership/suite_test.go
@@ -16,7 +16,7 @@ import (
 )
 
 var (
-	groupsServer *httptest.Server
+	usersServer *httptest.Server
 )
 
 func TestTeammembership(t *testing.T) {
@@ -26,7 +26,7 @@ func TestTeammembership(t *testing.T) {
 
 var _ = BeforeSuite(func() {
 	By("mocking SCIM server")
-	groupsServer = scim.ReturnDefaultGroupResponseMockServer()
+	usersServer = scim.ReturnUserResponseMockServer()
 
 	test.RegisterController("teammembershipUpdaterController",
 		(&teammembership.TeamMembershipUpdaterController{}).SetupWithManager)
@@ -35,7 +35,7 @@ var _ = BeforeSuite(func() {
 
 var _ = AfterSuite(func() {
 	By("tearing down the test environment")
-	groupsServer.Close()
+	usersServer.Close()
 
 	test.TestAfterSuite()
 })
diff --git a/pkg/controllers/teammembership/teammembership_updater_controller.go b/pkg/controllers/teammembership/teammembership_updater_controller.go
@@ -32,7 +32,7 @@ import (
 	"github.com/cloudoperators/greenhouse/pkg/scim"
 )
 
-const TeamMembershipRequeueInterval = 10 * time.Minute
+const RequeueInterval = 10 * time.Minute
 
 var (
 	membersCountMetric = prometheus.NewGaugeVec(
@@ -102,7 +102,7 @@ func (r *TeamMembershipUpdaterController) Reconcile(ctx context.Context, req ctr
 	return lifecycle.Reconcile(ctx, r.Client, req.NamespacedName, &greenhousev1alpha1.Team{}, r, nil) // status function is nil because it updates the other entity inside.
 }
 
-func (r *TeamMembershipUpdaterController) EnsureDeleted(ctx context.Context, object lifecycle.RuntimeObject) (ctrl.Result, lifecycle.ReconcileResult, error) {
+func (r *TeamMembershipUpdaterController) EnsureDeleted(_ context.Context, _ lifecycle.RuntimeObject) (ctrl.Result, lifecycle.ReconcileResult, error) {
 	return ctrl.Result{}, lifecycle.Success, nil
 }
 
@@ -180,7 +180,7 @@ func (r *TeamMembershipUpdaterController) EnsureCreated(ctx context.Context, obj
 		return ctrl.Result{}, lifecycle.Failed, err
 	}
 
-	users, membersValidCondition, err := r.getUsersFromSCIM(scimClient, team.Spec.MappedIDPGroup)
+	users, membersValidCondition, err := r.getUsersFromSCIM(ctx, scimClient, team.Spec.MappedIDPGroup)
 	if err != nil {
 		log.FromContext(ctx).Info("failed processing team-membership for team", "error", err)
 		teamMembershipStatus.SetConditions(greenhousev1alpha1.FalseCondition(greenhousev1alpha1.SCIMAccessReadyCondition, greenhousev1alpha1.SCIMRequestFailedReason, ""))
@@ -217,7 +217,7 @@ func (r *TeamMembershipUpdaterController) EnsureCreated(ctx context.Context, obj
 	teamMembershipStatus.LastChangedTime = &now
 	teamMembershipStatus.SetConditions(greenhousev1alpha1.TrueCondition(greenhousev1alpha1.SCIMAccessReadyCondition, "", ""))
 	return ctrl.Result{
-			RequeueAfter: wait.Jitter(TeamMembershipRequeueInterval, 0.1),
+			RequeueAfter: wait.Jitter(RequeueInterval, 0.1),
 		},
 		lifecycle.Success, nil
 }
@@ -227,7 +227,7 @@ func (r *TeamMembershipUpdaterController) createSCIMClient(
 	namespace string,
 	teamMembershipStatus *greenhousev1alpha1.TeamMembershipStatus,
 	scimConfig *greenhousev1alpha1.SCIMConfig,
-) (*scim.ScimClient, error) {
+) (scim.ISCIMClient, error) {
 
 	basicAuthUser, err := clientutil.GetSecretKeyFromSecretKeyReference(ctx, r.Client, namespace, *scimConfig.BasicAuthUser.Secret)
 	if err != nil {
@@ -239,27 +239,50 @@ func (r *TeamMembershipUpdaterController) createSCIMClient(
 		teamMembershipStatus.SetConditions(greenhousev1alpha1.FalseCondition(greenhousev1alpha1.SCIMAccessReadyCondition, greenhousev1alpha1.SecretNotFoundReason, "BasicAuthPw missing"))
 		return nil, err
 	}
-	clientConfig := scim.Config{
-		RawURL:   scimConfig.BaseURL,
+	clientConfig := &scim.Config{
+		URL:      scimConfig.BaseURL,
 		AuthType: scim.Basic,
-		BasicAuthConfig: &scim.BasicAuthConfig{
-			BasicAuthUser: basicAuthUser,
-			BasicAuthPw:   basicAuthPw,
+		BasicAuth: &scim.BasicAuthConfig{
+			Username: basicAuthUser,
+			Password: basicAuthPw,
 		},
 	}
-	return scim.NewScimClient(clientConfig)
+	logger := ctrl.LoggerFrom(ctx)
+	return scim.NewSCIMClient(logger, clientConfig)
 }
 
-func (r *TeamMembershipUpdaterController) getUsersFromSCIM(scimClient *scim.ScimClient, mappedIDPGroup string) ([]greenhousev1alpha1.User, greenhousev1alpha1.Condition, error) {
+func (r *TeamMembershipUpdaterController) getUsersFromSCIM(ctx context.Context, scimClient scim.ISCIMClient, mappedIDPGroup string) ([]greenhousev1alpha1.User, greenhousev1alpha1.Condition, error) {
 	condition := greenhousev1alpha1.UnknownCondition(greenhousev1alpha1.SCIMAllMembersValidCondition, "", "")
-	members, err := scimClient.GetTeamMembers(mappedIDPGroup)
-	if err != nil {
-		return nil, condition, err
+	opts := &scim.QueryOptions{
+		Filter:     scim.UserFilterByGroupDisplayName(mappedIDPGroup),
+		Attributes: scim.SetAttributes(scim.AttrName, scim.AttrEmails, scim.AttrDisplayName, scim.AttrActive),
+		StartID:    scim.InitialStartID,
 	}
-	users, inactive, malformed, err := scimClient.GetUsers(members)
+	resources, err := scimClient.GetUsers(ctx, opts)
 	if err != nil {
 		return nil, condition, err
 	}
+	users := make([]greenhousev1alpha1.User, 0)
+	malformed := 0
+	inactive := 0
+	for _, resource := range resources {
+		user := greenhousev1alpha1.User{
+			ID:        resource.UserName,
+			FirstName: resource.FirstName(),
+			LastName:  resource.LastName(),
+			Email:     resource.PrimaryEmail(),
+		}
+		if user.ID == "" || user.FirstName == "" || user.LastName == "" || user.Email == "" {
+			malformed++
+			continue
+		}
+
+		if !resource.ActiveUser() {
+			inactive++
+			continue
+		}
+		users = append(users, user)
+	}
 	if inactive+malformed > 0 {
 		msg := fmt.Sprintf("SCIM members with issues: %d inactive, %d malformed", inactive, malformed)
 		condition = greenhousev1alpha1.FalseCondition(greenhousev1alpha1.SCIMAllMembersValidCondition, "", msg)

diff --git a/pkg/controllers/teammembership/teammembership_updater_controller_test.go b/pkg/controllers/teammembership/teammembership_updater_controller_test.go
@@ -218,7 +218,7 @@ var _ = Describe("TeammembershipUpdaterController", Ordered, func() {
 
 			By("ensuring logger was called correctly")
 			failedProcessingLog := "failed processing team-membership for team"
-			reasonLog := "no mapped group found for " + nonExistingGroupName
+			reasonLog := "unexpected status code 404"
 			Eventually(func(g Gomega) {
 				g.Expect(tee.Contents()).To(ContainSubstring(failedProcessingLog), "logger should log failed processing")
 				g.Expect(tee.Contents()).To(ContainSubstring(reasonLog), "logger should log reason")
@@ -244,7 +244,7 @@ var _ = Describe("TeammembershipUpdaterController", Ordered, func() {
 
 			By("ensuring logger was called correctly")
 			failedProcessingLog := "failed processing team-membership for team"
-			reasonLog := "could not retrieve TeamMembers from"
+			reasonLog := "unexpected status code 404"
 			Eventually(func(g Gomega) {
 				g.Expect(tee.Contents()).To(ContainSubstring(failedProcessingLog), "logger should log failed processing")
 				g.Expect(tee.Contents()).To(ContainSubstring(reasonLog), "logger should log reason")
@@ -465,7 +465,7 @@ var _ = Describe("TeammembershipUpdaterController", Ordered, func() {
 			By("updating SCIMConfig in Organization")
 			organization.Spec.Authentication = &greenhousev1alpha1.Authentication{
 				SCIMConfig: &greenhousev1alpha1.SCIMConfig{
-					BaseURL: groupsServer.URL,
+					BaseURL: usersServer.URL + "/scim",
 					BasicAuthUser: greenhousev1alpha1.ValueFromSource{
 						Secret: &greenhousev1alpha1.SecretKeyReference{
 							Name: "test-secret",
@@ -508,7 +508,7 @@ func createTestOrgWithSecret(namespace string) {
 	org := setup.CreateOrganization(test.Ctx, namespace, func(o *greenhousev1alpha1.Organization) {
 		o.Spec.Authentication = &greenhousev1alpha1.Authentication{
 			SCIMConfig: &greenhousev1alpha1.SCIMConfig{
-				BaseURL: groupsServer.URL,
+				BaseURL: usersServer.URL + "/scim",
 				BasicAuthUser: greenhousev1alpha1.ValueFromSource{
 					Secret: &greenhousev1alpha1.SecretKeyReference{
 						Name: "test-secret",

diff --git a/pkg/mocks/mock_Client.go b/pkg/mocks/mock_Client.go
diff --git a/pkg/mocks/mock_Reconciler.go b/pkg/mocks/mock_Reconciler.go
diff --git a/pkg/mocks/mock_SubResourceWriter.go b/pkg/mocks/mock_SubResourceWriter.go