Merge pull request kubesphere#4695 from wansir/fix-4442

Fix cannot change user status to disabled

pkg/controller/user/user_controller.go

@@ -231,7 +231,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (reco
 	r.Recorder.Event(user, corev1.EventTypeNormal, successSynced, messageResourceSynced)
 
 	// block user for AuthenticateRateLimiterDuration duration, after that put it back to the queue to unblock
-	if user.Status.State != nil && *user.Status.State == iamv1alpha2.UserAuthLimitExceeded {
+	if user.Status.State == iamv1alpha2.UserAuthLimitExceeded {
 		return ctrl.Result{Requeue: true, RequeueAfter: r.AuthenticationOptions.AuthenticateRateLimiterDuration}, nil
 	}
 
@@ -454,13 +454,17 @@ func (r *Reconciler) deleteLoginRecords(ctx context.Context, user *iamv1alpha2.U
 
 // syncUserStatus Update the user status
 func (r *Reconciler) syncUserStatus(ctx context.Context, user *iamv1alpha2.User) error {
+	// skip status sync if the user is disabled
+	if user.Status.State == iamv1alpha2.UserDisabled {
+		return nil
+	}
+
 	if user.Spec.EncryptedPassword == "" {
 		if user.Labels[iamv1alpha2.IdentifyProviderLabel] != "" {
 			// mapped user from other identity provider always active until disabled
-			if user.Status.State == nil || *user.Status.State != iamv1alpha2.UserActive {
-				active := iamv1alpha2.UserActive
+			if user.Status.State != iamv1alpha2.UserActive {
 				user.Status = iamv1alpha2.UserStatus{
-					State:              &active,
+					State:              iamv1alpha2.UserActive,
 					LastTransitionTime: &metav1.Time{Time: time.Now()},
 				}
 				err := r.Update(ctx, user, &client.UpdateOptions{})
@@ -469,11 +473,10 @@ func (r *Reconciler) syncUserStatus(ctx context.Context, user *iamv1alpha2.User)
 				}
 			}
 		} else {
-			// becomes disabled after setting a blank password
-			if user.Status.State == nil || *user.Status.State != iamv1alpha2.UserDisabled {
-				disabled := iamv1alpha2.UserDisabled
+			// empty password is not allowed for normal user
+			if user.Status.State != iamv1alpha2.UserDisabled {
 				user.Status = iamv1alpha2.UserStatus{
-					State:              &disabled,
+					State:              iamv1alpha2.UserDisabled,
 					LastTransitionTime: &metav1.Time{Time: time.Now()},
 				}
 				err := r.Update(ctx, user, &client.UpdateOptions{})
@@ -482,32 +485,29 @@ func (r *Reconciler) syncUserStatus(ctx context.Context, user *iamv1alpha2.User)
 				}
 			}
 		}
+		// skip auth limit check
 		return nil
 	}
 
 	// becomes active after password encrypted
-	if isEncrypted(user.Spec.EncryptedPassword) {
-		if user.Status.State == nil || *user.Status.State == iamv1alpha2.UserDisabled {
-			active := iamv1alpha2.UserActive
-			user.Status = iamv1alpha2.UserStatus{
-				State:              &active,
-				LastTransitionTime: &metav1.Time{Time: time.Now()},
-			}
-			err := r.Update(ctx, user, &client.UpdateOptions{})
-			if err != nil {
-				return err
-			}
+	if user.Status.State == "" && isEncrypted(user.Spec.EncryptedPassword) {
+		user.Status = iamv1alpha2.UserStatus{
+			State:              iamv1alpha2.UserActive,
+			LastTransitionTime: &metav1.Time{Time: time.Now()},
+		}
+		err := r.Update(ctx, user, &client.UpdateOptions{})
+		if err != nil {
+			return err
 		}
 	}
 
 	// blocked user, check if need to unblock user
-	if user.Status.State != nil && *user.Status.State == iamv1alpha2.UserAuthLimitExceeded {
+	if user.Status.State == iamv1alpha2.UserAuthLimitExceeded {
 		if user.Status.LastTransitionTime != nil &&
 			user.Status.LastTransitionTime.Add(r.AuthenticationOptions.AuthenticateRateLimiterDuration).Before(time.Now()) {
 			// unblock user
-			active := iamv1alpha2.UserActive
 			user.Status = iamv1alpha2.UserStatus{
-				State:              &active,
+				State:              iamv1alpha2.UserActive,
 				LastTransitionTime: &metav1.Time{Time: time.Now()},
 			}
 			err := r.Update(ctx, user, &client.UpdateOptions{})
@@ -538,9 +538,8 @@ func (r *Reconciler) syncUserStatus(ctx context.Context, user *iamv1alpha2.User)
 
 	// block user if failed login attempts exceeds maximum tries setting
 	if failedLoginAttempts >= r.AuthenticationOptions.AuthenticateRateLimiterMaxTries {
-		limitExceed := iamv1alpha2.UserAuthLimitExceeded
 		user.Status = iamv1alpha2.UserStatus{
-			State:              &limitExceed,
+			State:              iamv1alpha2.UserAuthLimitExceeded,
 			Reason:             fmt.Sprintf("Failed login attempts exceed %d in last %s", failedLoginAttempts, r.AuthenticationOptions.AuthenticateRateLimiterDuration),
 			LastTransitionTime: &metav1.Time{Time: time.Now()},
 		}

pkg/controller/user/user_controller_test.go

@@ -125,12 +125,12 @@ func TestDoNothing(t *testing.T) {
 	// becomes active after password encrypted
 	updateEvent = <-w.ResultChan()
 	user = updateEvent.Object.(*iamv1alpha2.User)
-	assert.Equal(t, iamv1alpha2.UserActive, *user.Status.State)
+	assert.Equal(t, iamv1alpha2.UserActive, user.Status.State)
 
 	// block user
 	updateEvent = <-w.ResultChan()
 	user = updateEvent.Object.(*iamv1alpha2.User)
-	assert.Equal(t, iamv1alpha2.UserAuthLimitExceeded, *user.Status.State)
+	assert.Equal(t, iamv1alpha2.UserAuthLimitExceeded, user.Status.State)
 	assert.True(t, result.Requeue)
 
 	time.Sleep(result.RequeueAfter + time.Second)
@@ -144,5 +144,5 @@ func TestDoNothing(t *testing.T) {
 	// unblock user
 	updateEvent = <-w.ResultChan()
 	user = updateEvent.Object.(*iamv1alpha2.User)
-	assert.Equal(t, iamv1alpha2.UserActive, *user.Status.State)
+	assert.Equal(t, iamv1alpha2.UserActive, user.Status.State)
 }

pkg/models/auth/password.go

@@ -112,8 +112,8 @@ func (p *passwordAuthenticator) Authenticate(_ context.Context, username, passwo
 	}
 
 	// check user status
-	if user != nil && (user.Status.State == nil || *user.Status.State != iamv1alpha2.UserActive) {
-		if user.Status.State != nil && *user.Status.State == iamv1alpha2.UserAuthLimitExceeded {
+	if user != nil && user.Status.State != iamv1alpha2.UserActive {
+		if user.Status.State == iamv1alpha2.UserAuthLimitExceeded {
 			klog.Errorf("%s, username: %s", RateLimitExceededError, username)
 			return nil, "", RateLimitExceededError
 		} else {

pkg/models/auth/password_test.go

@@ -242,7 +242,6 @@ func newActiveUser(username string, password string) *iamv1alpha2.User {
 	u := newUser(username, "", "")
 	password, _ = encrypt(password)
 	u.Spec.EncryptedPassword = password
-	s := iamv1alpha2.UserActive
-	u.Status.State = &s
+	u.Status.State = iamv1alpha2.UserActive
 	return u
 }

pkg/models/iam/im/im.go

@@ -18,6 +18,7 @@ package im
 import (
 	"context"
 	"fmt"
+	"time"
 
 	"kubesphere.io/kubesphere/pkg/apiserver/authentication"
 
@@ -70,7 +71,13 @@ func (im *imOperator) UpdateUser(new *iamv1alpha2.User) (*iamv1alpha2.User, erro
 	}
 	// keep encrypted password and user status
 	new.Spec.EncryptedPassword = old.Spec.EncryptedPassword
-	new.Status = old.Status
+	status := old.Status
+	// only support enable or disable
+	if new.Status.State == iamv1alpha2.UserDisabled || new.Status.State == iamv1alpha2.UserActive {
+		status.State = new.Status.State
+		status.LastTransitionTime = &metav1.Time{Time: time.Now()}
+	}
+	new.Status = status
 	updated, err := im.ksClient.IamV1alpha2().Users().Update(context.Background(), new, metav1.UpdateOptions{})
 	if err != nil {
 		klog.Error(err)

staging/src/kubesphere.io/api/iam/v1alpha2/types.go

@@ -164,7 +164,7 @@ const (
 type UserStatus struct {
 	// The user status
 	// +optional
-	State *UserState `json:"state,omitempty"`
+	State UserState `json:"state,omitempty"`
 	// +optional
 	Reason string `json:"reason,omitempty"`
 	// +optional