Fix override clients bug

api/v1alpha1/oauth2client_types.go

@@ -16,6 +16,8 @@ limitations under the License.
 package v1alpha1
 
 import (
+	"fmt"
+
 	"github.com/ory/hydra-maester/hydra"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -110,10 +112,10 @@ func init() {
 // ToOAuth2ClientJSON converts an OAuth2Client into a OAuth2ClientJSON object that represents an OAuth2 client digestible by ORY Hydra
 func (c *OAuth2Client) ToOAuth2ClientJSON() *hydra.OAuth2ClientJSON {
 	return &hydra.OAuth2ClientJSON{
-		Name:          c.Name,
 		GrantTypes:    grantToStringSlice(c.Spec.GrantTypes),
 		ResponseTypes: responseToStringSlice(c.Spec.ResponseTypes),
 		Scope:         c.Spec.Scope,
+		Owner:         fmt.Sprintf("%s/%s", c.Name, c.Namespace),
 	}
 }
 

controllers/oauth2client_controller.go

@@ -34,11 +34,11 @@ import (
 const (
 	ClientIDKey     = "client_id"
 	ClientSecretKey = "client_secret"
-	ownerLabel      = "owner"
 )
 
 type HydraClientInterface interface {
 	GetOAuth2Client(id string) (*hydra.OAuth2ClientJSON, bool, error)
+	ListOAuth2Client() ([]*hydra.OAuth2ClientJSON, error)
 	PostOAuth2Client(o *hydra.OAuth2ClientJSON) (*hydra.OAuth2ClientJSON, error)
 	PutOAuth2Client(o *hydra.OAuth2ClientJSON) (*hydra.OAuth2ClientJSON, error)
 	DeleteOAuth2Client(id string) error
@@ -62,8 +62,8 @@ func (r *OAuth2ClientReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error
 	var oauth2client hydrav1alpha1.OAuth2Client
 	if err := r.Get(ctx, req.NamespacedName, &oauth2client); err != nil {
 		if apierrs.IsNotFound(err) {
-			if err := r.unregisterOAuth2Clients(ctx, req.Name, req.Namespace); err != nil {
-				return ctrl.Result{}, err
+			if registerErr := r.unregisterOAuth2Clients(ctx, req.Name, req.Namespace); registerErr != nil {
+				return ctrl.Result{}, registerErr
 			}
 			return ctrl.Result{}, nil
 		}
@@ -75,32 +75,47 @@ func (r *OAuth2ClientReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error
 		var secret apiv1.Secret
 		if err := r.Get(ctx, types.NamespacedName{Name: oauth2client.Spec.SecretName, Namespace: req.Namespace}, &secret); err != nil {
 			if apierrs.IsNotFound(err) {
-				return ctrl.Result{}, r.registerOAuth2Client(ctx, &oauth2client, nil)
+				if registerErr := r.registerOAuth2Client(ctx, &oauth2client, nil); registerErr != nil {
+					return ctrl.Result{}, registerErr
+				}
+				return ctrl.Result{}, nil
 			}
 			return ctrl.Result{}, err
 		}
 
 		credentials, err := parseSecret(secret)
 		if err != nil {
 			r.Log.Error(err, fmt.Sprintf("secret %s/%s is invalid", secret.Name, secret.Namespace))
-			return ctrl.Result{}, r.updateReconciliationStatusError(ctx, &oauth2client, hydrav1alpha1.StatusInvalidSecret, err)
-		}
-
-		if err := r.labelSecretWithOwnerReference(ctx, &oauth2client, secret); err != nil {
-			return ctrl.Result{}, err
+			if updateErr := r.updateReconciliationStatusError(ctx, &oauth2client, hydrav1alpha1.StatusInvalidSecret, err); updateErr != nil {
+				return ctrl.Result{}, updateErr
+			}
+			return ctrl.Result{}, nil
 		}
 
-		_, found, err := r.HydraClient.GetOAuth2Client(string(credentials.ID))
+		fetched, found, err := r.HydraClient.GetOAuth2Client(string(credentials.ID))
 		if err != nil {
 			return ctrl.Result{}, err
 
 		}
 
 		if found {
-			return ctrl.Result{}, r.updateRegisteredOAuth2Client(ctx, &oauth2client, credentials)
+			if fetched.Owner != oauth2client.Name {
+				conflictErr := errors.Errorf("ID provided in secret %s/%s is assigned to another resource", secret.Name, secret.Namespace)
+				if updateErr := r.updateReconciliationStatusError(ctx, &oauth2client, hydrav1alpha1.StatusInvalidSecret, conflictErr); updateErr != nil {
+					return ctrl.Result{}, updateErr
+				}
+				return ctrl.Result{}, nil
+			}
+
+			if updateErr := r.updateRegisteredOAuth2Client(ctx, &oauth2client, credentials); updateErr != nil {
+				return ctrl.Result{}, updateErr
+			}
+			return ctrl.Result{}, nil
 		}
 
-		return ctrl.Result{}, r.registerOAuth2Client(ctx, &oauth2client, credentials)
+		if registerErr := r.registerOAuth2Client(ctx, &oauth2client, credentials); registerErr != nil {
+			return ctrl.Result{}, registerErr
+		}
 	}
 
 	return ctrl.Result{}, nil
@@ -119,21 +134,25 @@ func (r *OAuth2ClientReconciler) registerOAuth2Client(ctx context.Context, c *hy
 
 	if credentials != nil {
 		if _, err := r.HydraClient.PostOAuth2Client(c.ToOAuth2ClientJSON().WithCredentials(credentials)); err != nil {
-			return r.updateReconciliationStatusError(ctx, c, hydrav1alpha1.StatusRegistrationFailed, err)
+			if updateErr := r.updateReconciliationStatusError(ctx, c, hydrav1alpha1.StatusRegistrationFailed, err); updateErr != nil {
+				return updateErr
+			}
 		}
-		return nil
+		return r.ensureEmptyStatusError(ctx, c)
 	}
 
 	created, err := r.HydraClient.PostOAuth2Client(c.ToOAuth2ClientJSON())
 	if err != nil {
-		return r.updateReconciliationStatusError(ctx, c, hydrav1alpha1.StatusRegistrationFailed, err)
+		if updateErr := r.updateReconciliationStatusError(ctx, c, hydrav1alpha1.StatusRegistrationFailed, err); updateErr != nil {
+			return updateErr
+		}
+		return nil
 	}
 
 	clientSecret := apiv1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      c.Spec.SecretName,
 			Namespace: c.Namespace,
-			Labels:    map[string]string{ownerLabel: c.Name},
 		},
 		Data: map[string][]byte{
 			ClientIDKey:     []byte(*created.ClientID),
@@ -142,44 +161,35 @@ func (r *OAuth2ClientReconciler) registerOAuth2Client(ctx context.Context, c *hy
 	}
 
 	if err := r.Create(ctx, &clientSecret); err != nil {
-		return r.updateReconciliationStatusError(ctx, c, hydrav1alpha1.StatusCreateSecretFailed, err)
+		if updateErr := r.updateReconciliationStatusError(ctx, c, hydrav1alpha1.StatusCreateSecretFailed, err); updateErr != nil {
+			return updateErr
+		}
 	}
 
-	return nil
+	return r.ensureEmptyStatusError(ctx, c)
 }
 
 func (r *OAuth2ClientReconciler) updateRegisteredOAuth2Client(ctx context.Context, c *hydrav1alpha1.OAuth2Client, credentials *hydra.Oauth2ClientCredentials) error {
 	if _, err := r.HydraClient.PutOAuth2Client(c.ToOAuth2ClientJSON().WithCredentials(credentials)); err != nil {
-		return r.updateReconciliationStatusError(ctx, c, hydrav1alpha1.StatusUpdateFailed, err)
+		if updateErr := r.updateReconciliationStatusError(ctx, c, hydrav1alpha1.StatusUpdateFailed, err); updateErr != nil {
+			return updateErr
+		}
 	}
-	return nil
+	return r.ensureEmptyStatusError(ctx, c)
 }
 
 func (r *OAuth2ClientReconciler) unregisterOAuth2Clients(ctx context.Context, name, namespace string) error {
-	var secretList apiv1.SecretList
-
-	err := r.List(
-		ctx,
-		&secretList,
-		client.InNamespace(namespace),
-		client.MatchingLabels(map[string]string{ownerLabel: name}))
 
+	clients, err := r.HydraClient.ListOAuth2Client()
 	if err != nil {
 		return err
 	}
 
-	if len(secretList.Items) == 0 {
-		return nil
-	}
-
-	ids := make(map[string]struct{})
-	for _, s := range secretList.Items {
-		ids[string(s.Data[ClientIDKey])] = struct{}{}
-	}
-
-	for id := range ids {
-		if err := r.HydraClient.DeleteOAuth2Client(id); err != nil {
-			return err
+	for _, c := range clients {
+		if c.Owner == fmt.Sprintf("%s/%s", name, namespace) {
+			if err := r.HydraClient.DeleteOAuth2Client(*c.ClientID); err != nil {
+				return err
+			}
 		}
 	}
 
@@ -188,16 +198,25 @@ func (r *OAuth2ClientReconciler) unregisterOAuth2Clients(ctx context.Context, na
 
 func (r *OAuth2ClientReconciler) updateReconciliationStatusError(ctx context.Context, c *hydrav1alpha1.OAuth2Client, code hydrav1alpha1.StatusCode, err error) error {
 	r.Log.Error(err, fmt.Sprintf("error processing client %s/%s ", c.Name, c.Namespace), "oauth2client", "register")
-	c.Status.ObservedGeneration = c.Generation
 	c.Status.ReconciliationError = hydrav1alpha1.ReconciliationError{
 		Code:        code,
 		Description: err.Error(),
 	}
-	if updateErr := r.Status().Update(ctx, c); updateErr != nil {
-		r.Log.Error(updateErr, fmt.Sprintf("status update failed for client %s/%s ", c.Name, c.Namespace), "oauth2client", "update status")
-		return updateErr
-	}
 
+	return r.updateClientStatus(ctx, c)
+}
+
+func (r *OAuth2ClientReconciler) ensureEmptyStatusError(ctx context.Context, c *hydrav1alpha1.OAuth2Client) error {
+	c.Status.ReconciliationError = hydrav1alpha1.ReconciliationError{}
+	return r.updateClientStatus(ctx, c)
+}
+
+func (r *OAuth2ClientReconciler) updateClientStatus(ctx context.Context, c *hydrav1alpha1.OAuth2Client) error {
+	c.Status.ObservedGeneration = c.Generation
+	if err := r.Status().Update(ctx, c); err != nil {
+		r.Log.Error(err, fmt.Sprintf("status update failed for client %s/%s ", c.Name, c.Namespace), "oauth2client", "update status")
+		return err
+	}
 	return nil
 }
 
@@ -218,16 +237,3 @@ func parseSecret(secret apiv1.Secret) (*hydra.Oauth2ClientCredentials, error) {
 		Password: psw,
 	}, nil
 }
-
-func (r *OAuth2ClientReconciler) labelSecretWithOwnerReference(ctx context.Context, c *hydrav1alpha1.OAuth2Client, secret apiv1.Secret) error {
-
-	if secret.Labels[ownerLabel] != c.Name {
-		if secret.Labels == nil {
-			secret.Labels = make(map[string]string, 1)
-		}
-		secret.Labels[ownerLabel] = c.Name
-		return r.Update(ctx, &secret)
-	}
-
-	return nil
-}

controllers/oauth2client_controller_integration_test.go

@@ -61,14 +61,15 @@ var _ = Describe("OAuth2Client Controller", func() {
 
 				mch := &mocks.HydraClientInterface{}
 				mch.On("DeleteOAuth2Client", Anything).Return(nil)
+				mch.On("ListOAuth2Client", Anything).Return(nil, nil)
 				mch.On("PostOAuth2Client", AnythingOfType("*hydra.OAuth2ClientJSON")).Return(func(o *hydra.OAuth2ClientJSON) *hydra.OAuth2ClientJSON {
 					return &hydra.OAuth2ClientJSON{
 						ClientID:      &tstClientID,
 						Secret:        pointer.StringPtr(tstSecret),
-						Name:          o.Name,
 						GrantTypes:    o.GrantTypes,
 						ResponseTypes: o.ResponseTypes,
 						Scope:         o.Scope,
+						Owner:         o.Owner,
 					}
 				}, func(o *hydra.OAuth2ClientJSON) error {
 					return nil
@@ -137,6 +138,7 @@ var _ = Describe("OAuth2Client Controller", func() {
 				mch := &mocks.HydraClientInterface{}
 				mch.On("PostOAuth2Client", Anything).Return(nil, errors.New("error"))
 				mch.On("DeleteOAuth2Client", Anything).Return(nil)
+				mch.On("ListOAuth2Client", Anything).Return(nil, nil)
 
 				recFn, requests := SetupTestReconcile(getAPIReconciler(mgr, mch))
 
@@ -201,15 +203,16 @@ var _ = Describe("OAuth2Client Controller", func() {
 
 				mch := mocks.HydraClientInterface{}
 				mch.On("DeleteOAuth2Client", Anything).Return(nil)
+				mch.On("ListOAuth2Client", Anything).Return(nil, nil)
 				mch.On("GetOAuth2Client", Anything).Return(nil, false, nil)
 				mch.On("PostOAuth2Client", AnythingOfType("*hydra.OAuth2ClientJSON")).Return(func(o *hydra.OAuth2ClientJSON) *hydra.OAuth2ClientJSON {
 					postedClient = &hydra.OAuth2ClientJSON{
 						ClientID:      o.ClientID,
 						Secret:        o.Secret,
-						Name:          o.Name,
 						GrantTypes:    o.GrantTypes,
 						ResponseTypes: o.ResponseTypes,
 						Scope:         o.Scope,
+						Owner:         o.Owner,
 					}
 					return postedClient
 				}, func(o *hydra.OAuth2ClientJSON) error {
@@ -287,6 +290,7 @@ var _ = Describe("OAuth2Client Controller", func() {
 
 				mch := mocks.HydraClientInterface{}
 				mch.On("DeleteOAuth2Client", Anything).Return(nil)
+				mch.On("ListOAuth2Client", Anything).Return(nil, nil)
 
 				recFn, requests := SetupTestReconcile(getAPIReconciler(mgr, &mch))
 

hydra/client.go

@@ -39,6 +39,28 @@ func (c *Client) GetOAuth2Client(id string) (*OAuth2ClientJSON, bool, error) {
 	}
 }
 
+func (c *Client) ListOAuth2Client() ([]*OAuth2ClientJSON, error) {
+
+	var jsonClientList []*OAuth2ClientJSON
+
+	req, err := c.newRequest(http.MethodGet, "", nil)
+	if err != nil {
+		return nil, err
+	}
+
+	resp, err := c.do(req, &jsonClientList)
+	if err != nil {
+		return nil, err
+	}
+
+	switch resp.StatusCode {
+	case http.StatusOK:
+		return jsonClientList, nil
+	default:
+		return nil, fmt.Errorf("%s %s http request returned unexpected status code %s", req.Method, req.URL.String(), resp.Status)
+	}
+}
+
 func (c *Client) PostOAuth2Client(o *OAuth2ClientJSON) (*OAuth2ClientJSON, error) {
 
 	var jsonClient *OAuth2ClientJSON
@@ -57,7 +79,7 @@ func (c *Client) PostOAuth2Client(o *OAuth2ClientJSON) (*OAuth2ClientJSON, error
 	case http.StatusCreated:
 		return jsonClient, nil
 	case http.StatusConflict:
-		return nil, fmt.Errorf(" %s %s http request failed: requested ID already exists", req.Method, req.URL)
+		return nil, fmt.Errorf("%s %s http request failed: requested ID already exists", req.Method, req.URL)
 	default:
 		return nil, fmt.Errorf("%s %s http request returned unexpected status code: %s", req.Method, req.URL, resp.Status)
 	}