From 4457f61a225c57e3ab673eb1a60324a1178eca76 Mon Sep 17 00:00:00 2001 From: hongming Date: Tue, 1 Mar 2022 11:36:17 +0800 Subject: [PATCH] Fix cannot change user status to disabled --- pkg/controller/user/user_controller.go | 47 +++++++++---------- pkg/controller/user/user_controller_test.go | 6 +-- pkg/models/auth/password.go | 4 +- pkg/models/auth/password_test.go | 3 +- pkg/models/iam/im/im.go | 9 +++- .../kubesphere.io/api/iam/v1alpha2/types.go | 2 +- .../api/iam/v1alpha2/zz_generated.deepcopy.go | 5 -- 7 files changed, 38 insertions(+), 38 deletions(-) diff --git a/pkg/controller/user/user_controller.go b/pkg/controller/user/user_controller.go index 41e66d3bb..d758aba0c 100644 --- a/pkg/controller/user/user_controller.go +++ b/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()}, } diff --git a/pkg/controller/user/user_controller_test.go b/pkg/controller/user/user_controller_test.go index b79e81655..58b59033f 100644 --- a/pkg/controller/user/user_controller_test.go +++ b/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) } diff --git a/pkg/models/auth/password.go b/pkg/models/auth/password.go index aab74f85a..b4b186912 100644 --- a/pkg/models/auth/password.go +++ b/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 { diff --git a/pkg/models/auth/password_test.go b/pkg/models/auth/password_test.go index bb0af5667..829ac6aad 100644 --- a/pkg/models/auth/password_test.go +++ b/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 } diff --git a/pkg/models/iam/im/im.go b/pkg/models/iam/im/im.go index 6145d3bad..7d7bc23f1 100644 --- a/pkg/models/iam/im/im.go +++ b/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) diff --git a/staging/src/kubesphere.io/api/iam/v1alpha2/types.go b/staging/src/kubesphere.io/api/iam/v1alpha2/types.go index 7a22797a1..994dd3b40 100644 --- a/staging/src/kubesphere.io/api/iam/v1alpha2/types.go +++ b/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 diff --git a/staging/src/kubesphere.io/api/iam/v1alpha2/zz_generated.deepcopy.go b/staging/src/kubesphere.io/api/iam/v1alpha2/zz_generated.deepcopy.go index 53810ab25..2920eccd3 100644 --- a/staging/src/kubesphere.io/api/iam/v1alpha2/zz_generated.deepcopy.go +++ b/staging/src/kubesphere.io/api/iam/v1alpha2/zz_generated.deepcopy.go @@ -770,11 +770,6 @@ func (in *UserSpec) DeepCopy() *UserSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *UserStatus) DeepCopyInto(out *UserStatus) { *out = *in - if in.State != nil { - in, out := &in.State, &out.State - *out = new(UserState) - **out = **in - } if in.LastTransitionTime != nil { in, out := &in.LastTransitionTime, &out.LastTransitionTime *out = (*in).DeepCopy()