Skip to content

Commit

Permalink
Merge branch 'master' into ks20
Browse files Browse the repository at this point in the history
  • Loading branch information
@fbm3307
fbm3307 authored Mar 5, 2024
2 parents bae8e12 + 8397901 commit e79f28e
Show file tree
Hide file tree
Showing 2 changed files with 198 additions and 1 deletion.
77 changes: 76 additions & 1 deletion controllers/usersignupcleanup/usersignup_cleanup_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ package usersignupcleanup

import (
"context"
"fmt"
"github.com/codeready-toolchain/toolchain-common/pkg/hash"
"github.com/redhat-cop/operator-utils/pkg/util"
"strconv"
"time"

toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
Expand All @@ -22,6 +25,12 @@ import (
"sigs.k8s.io/controller-runtime/pkg/reconcile"
)

const (
// MaxActivationsForCleanup is the maximum number of activations that a user may have in order to be considered
// for cleanup. If the number of user activations exceeds this constant value, then cleanup will not occur.
MaxActivationsForCleanup = 1
)

type StatusUpdater func(userAcc *toolchainv1alpha1.UserSignup, message string) error

// SetupWithManager sets up the controller with the Manager.
Expand Down Expand Up @@ -136,7 +145,27 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
// then delete the UserSignup
deactivatedThreshold := time.Now().Add(-time.Duration(config.Deactivation().UserSignupDeactivatedRetentionDays()*24) * time.Hour)

if cond.LastTransitionTime.Time.Before(deactivatedThreshold) {
meetsActivationCriteria := false
if !activationsAnnotationPresent {
// Meets the criteria if there is no activations annotation
meetsActivationCriteria = true
} else {
activationCount, err := strconv.Atoi(activations)
if err != nil {
// If there was an error converting the activation count to an integer, then assume it meets the criteria for deletion
meetsActivationCriteria = true
} else {
// Otherwise it meets the criteria if we can determine there has only ever been one activation in total
meetsActivationCriteria = activationCount <= MaxActivationsForCleanup
}
}

banned, err := r.isUserBanned(ctx, instance)
if err != nil {
return reconcile.Result{}, err
}

if cond.LastTransitionTime.Time.Before(deactivatedThreshold) && meetsActivationCriteria && !banned {
reqLogger.Info("Deleting UserSignup due to exceeding deactivated retention period")
return reconcile.Result{}, r.DeleteUserSignup(ctx, instance)
}
Expand All @@ -155,6 +184,52 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
return reconcile.Result{}, nil
}

func (r *Reconciler) isUserBanned(ctx context.Context, userSignup *toolchainv1alpha1.UserSignup) (bool, error) {
banned := false
// Lookup the user email
if userSignup.Spec.IdentityClaims.Email != "" {

// Lookup the email hash label
if emailHashLbl, exists := userSignup.Labels[toolchainv1alpha1.UserSignupUserEmailHashLabelKey]; exists {

labels := map[string]string{toolchainv1alpha1.BannedUserEmailHashLabelKey: emailHashLbl}
opts := runtimeclient.MatchingLabels(labels)
bannedUserList := &toolchainv1alpha1.BannedUserList{}

// Query BannedUser for resources that match the same email hash
if err := r.Client.List(ctx, bannedUserList, opts); err != nil {
return false, err
}

// One last check to confirm that the e-mail addresses match also (in case of the infinitesimal chance of a hash collision)
for _, bannedUser := range bannedUserList.Items {
if bannedUser.Spec.Email == userSignup.Spec.IdentityClaims.Email {
banned = true
break
}
}

hashIsValid := validateEmailHash(userSignup.Spec.IdentityClaims.Email, emailHashLbl)
if !hashIsValid {
err := fmt.Errorf("email hash is invalid for UserSignup [%s]", userSignup.Name)
return banned, err
}
} else {
// If there isn't an email-hash label, then the state is invalid
err := fmt.Errorf("missing email-hash label for UserSignup [%s]", userSignup.Name)
return banned, err
}
} else {
err := fmt.Errorf("could not determine email address for UserSignup [%s]", userSignup.Name)
return banned, err
}
return banned, nil
}

func validateEmailHash(userEmail, userEmailHash string) bool {
return hash.EncodeString(userEmail) == userEmailHash
}

// DeleteUserSignup deletes the specified UserSignup
func (r *Reconciler) DeleteUserSignup(ctx context.Context, userSignup *toolchainv1alpha1.UserSignup) error {
// before deleting the resource, we want to "remember" if the user triggered a phone verification or not,
Expand Down
122 changes: 122 additions & 0 deletions controllers/usersignupcleanup/usersignup_cleanup_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"context"
"errors"
"fmt"
commonconfig "github.com/codeready-toolchain/toolchain-common/pkg/configuration"
testconfig "github.com/codeready-toolchain/toolchain-common/pkg/test/config"
"os"
"testing"
"time"
Expand Down Expand Up @@ -31,6 +33,12 @@ func TestUserCleanup(t *testing.T) {
// A creation time five years in the past
fiveYears := time.Duration(time.Hour * 24 * 365 * 5)

// A creation time two years in the past
twoYears := time.Duration(time.Hour * 24 * 365 * 2)

// A creation time one year in the past
oneYear := time.Duration(time.Hour * 24 * 365 * 1)

t.Run("test that user cleanup doesn't delete an active UserSignup", func(t *testing.T) {

userSignup := commonsignup.NewUserSignup(
Expand Down Expand Up @@ -241,6 +249,120 @@ func TestUserCleanup(t *testing.T) {
require.False(t, res.Requeue)
})

t.Run("test old deactivated UserSignup cleanup", func(t *testing.T) {
config := commonconfig.NewToolchainConfigObjWithReset(t, testconfig.AutomaticApproval().Enabled(true),
testconfig.Deactivation().UserSignupDeactivatedRetentionDays(720))

tests := map[string]struct {
userSignup *toolchainv1alpha1.UserSignup
banned bool
expectedError string
expectedToBeDeleted bool
}{
"test that a UserSignup older than 2 years, with 1 activation and not banned, is deleted": {
userSignup: commonsignup.NewUserSignup(
commonsignup.DeactivatedWithLastTransitionTime(twoYears),
commonsignup.WithActivations("1")),
expectedError: "",
expectedToBeDeleted: true,
},
"test that a UserSignup older than 2 years, with indeterminate activations and not banned, is deleted": {
userSignup: commonsignup.NewUserSignup(
commonsignup.DeactivatedWithLastTransitionTime(twoYears),
commonsignup.WithActivations("unknown")),
expectedError: "",
expectedToBeDeleted: true,
},
"test that a UserSignup 1 year old, with 1 activation and not banned, is not deleted": {
userSignup: commonsignup.NewUserSignup(
commonsignup.DeactivatedWithLastTransitionTime(oneYear),
commonsignup.WithActivations("1")),
expectedError: "",
expectedToBeDeleted: false,
},
"test that a UserSignup older than 2 years, with 2 activations and not banned, is not deleted": {
userSignup: commonsignup.NewUserSignup(
commonsignup.DeactivatedWithLastTransitionTime(twoYears),
commonsignup.WithActivations("2")),
expectedError: "",
expectedToBeDeleted: false,
},
"test that a UserSignup older than 2 years, with 1 activation but has been banned, is not deleted": {
userSignup: commonsignup.NewUserSignup(
commonsignup.DeactivatedWithLastTransitionTime(twoYears),
commonsignup.WithActivations("1")),
banned: true,
expectedError: "",
expectedToBeDeleted: false,
},
"test that a banned UserSignup with an invalid email hash returns an error and is not deleted": {
userSignup: commonsignup.NewUserSignup(
commonsignup.DeactivatedWithLastTransitionTime(twoYears),
commonsignup.WithActivations("1"),
commonsignup.WithName("invalid-email-user"),
commonsignup.WithLabel(toolchainv1alpha1.UserSignupUserEmailHashLabelKey, "INVALID")),
banned: true,
expectedError: "email hash is invalid for UserSignup [invalid-email-user]",
expectedToBeDeleted: false,
},
"test that a UserSignup without an email address returns an error and is not deleted": {
userSignup: commonsignup.NewUserSignup(
commonsignup.DeactivatedWithLastTransitionTime(twoYears),
commonsignup.WithActivations("1"),
commonsignup.WithName("without-email-user"),
commonsignup.WithEmail("")),
expectedError: "could not determine email address for UserSignup [without-email-user]",
expectedToBeDeleted: false,
},
}

for k, tc := range tests {
t.Run(k, func(t *testing.T) {
var r *Reconciler
var req reconcile.Request
if tc.banned {
bannedUser := &toolchainv1alpha1.BannedUser{
ObjectMeta: corev1.ObjectMeta{
Labels: map[string]string{
toolchainv1alpha1.BannedUserEmailHashLabelKey: tc.userSignup.Labels[toolchainv1alpha1.UserSignupUserEmailHashLabelKey],
},
},
Spec: toolchainv1alpha1.BannedUserSpec{
Email: tc.userSignup.Spec.IdentityClaims.Email,
},
}
r, req, _ = prepareReconcile(t, tc.userSignup.Name, tc.userSignup, config, bannedUser)
} else {
r, req, _ = prepareReconcile(t, tc.userSignup.Name, tc.userSignup, config)
}

_, err := r.Reconcile(context.TODO(), req)
if tc.expectedError != "" {
require.Error(t, err)
require.Equal(t, tc.expectedError, err.Error())
} else {
require.NoError(t, err)
}

key := test.NamespacedName(test.HostOperatorNs, tc.userSignup.Name)
err = r.Client.Get(context.Background(), key, tc.userSignup)
if tc.expectedToBeDeleted {
// Confirm the UserSignup has been deleted
require.Error(t, err)
require.True(t, apierrors.IsNotFound(err))
require.IsType(t, &apierrors.StatusError{}, err)
statusErr := &apierrors.StatusError{}
require.True(t, errors.As(err, &statusErr))
require.Equal(t, fmt.Sprintf("usersignups.toolchain.dev.openshift.com \"%s\" not found", key.Name), statusErr.Error())
} else {
// Confirm the UserSignup has not been deleted
require.NoError(t, err)
require.NotNil(t, tc.userSignup)
}
})
}
})

t.Run("test propagation policy", func(t *testing.T) {
userSignup := commonsignup.NewUserSignup(
commonsignup.CreatedBefore(fiveYears),
Expand Down

0 comments on commit e79f28e

Please sign in to comment.