Fix override clients bug (#23)

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/mocks/HydraClientInterface.go

@@ -54,6 +54,29 @@ func (_m *HydraClientInterface) GetOAuth2Client(id string) (*hydra.OAuth2ClientJ
 	return r0, r1, r2
 }
 
+// ListOAuth2Client provides a mock function with given fields:
+func (_m *HydraClientInterface) ListOAuth2Client() ([]*hydra.OAuth2ClientJSON, error) {
+	ret := _m.Called()
+
+	var r0 []*hydra.OAuth2ClientJSON
+	if rf, ok := ret.Get(0).(func() []*hydra.OAuth2ClientJSON); ok {
+		r0 = rf()
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).([]*hydra.OAuth2ClientJSON)
+		}
+	}
+
+	var r1 error
+	if rf, ok := ret.Get(1).(func() error); ok {
+		r1 = rf()
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
 // PostOAuth2Client provides a mock function with given fields: o
 func (_m *HydraClientInterface) PostOAuth2Client(o *hydra.OAuth2ClientJSON) (*hydra.OAuth2ClientJSON, error) {
 	ret := _m.Called(o)

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 {
+			if registerErr := r.unregisterOAuth2Clients(ctx, req.Name, req.Namespace); registerErr != nil {
-				return ctrl.Result{}, err
+				return ctrl.Result{}, registerErr
 			}
 			return ctrl.Result{}, nil
 		}
@@ -75,7 +75,10 @@ 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
 		}
@@ -83,24 +86,36 @@ func (r *OAuth2ClientReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error
 		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 updateErr := r.updateReconciliationStatusError(ctx, &oauth2client, hydrav1alpha1.StatusInvalidSecret, err); updateErr != nil {
+				return ctrl.Result{}, updateErr
+			}
+			return ctrl.Result{}, nil
 		}
 
-		if err := r.labelSecretWithOwnerReference(ctx, &oauth2client, secret); err != nil {
+		fetched, found, err := r.HydraClient.GetOAuth2Client(string(credentials.ID))
-			return ctrl.Result{}, err
-		}
-
-		_, 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 {
+	for _, c := range clients {
-		return nil
+		if c.Owner == fmt.Sprintf("%s/%s", name, namespace) {
-	}
+			if err := r.HydraClient.DeleteOAuth2Client(*c.ClientID); err != nil {
-
+				return err
-	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
 		}
 	}
 
@@ -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)
 	}

hydra/client_test.go

@@ -19,10 +19,13 @@ import (
 const (
 	testID            = "test-id"
 	schemeHTTP        = "http"
-	testClient        = `{"client_id":"test-id","client_name":"test-name","scope":"some,scopes","grant_types":["type1"]}`
+	testClient        = `{"client_id":"test-id","owner":"test-name","scope":"some,scopes","grant_types":["type1"]}`
-	testClientCreated = `{"client_id":"test-id-2", "client_secret": "TmGkvcY7k526","client_name":"test-name-2","scope":"some,other,scopes","grant_types":["type2"]}`
+	testClientCreated = `{"client_id":"test-id-2","client_secret":"TmGkvcY7k526","owner":"test-name-2","scope":"some,other,scopes","grant_types":["type2"]}`
-	testClientUpdated = `{"client_id":"test-id-3", "client_secret": "xFoPPm654por","client_name":"test-name-3","scope":"yet,another,scope","grant_types":["type3"]}`
+	testClientUpdated = `{"client_id":"test-id-3","client_secret":"xFoPPm654por","owner":"test-name-3","scope":"yet,another,scope","grant_types":["type3"]}`
+	testClientList    = `{"client_id":"test-id-4","owner":"test-name-4","scope":"scope1 scope2","grant_types":["type4"]}`
+	testClientList2   = `{"client_id":"test-id-5","owner":"test-name-5","scope":"scope3 scope4","grant_types":["type5"]}`
 	emptyBody         = `{}`
+	emptyBodyList     = `[]`
 	clientsEndpoint   = "/clients"
 )
 
@@ -33,16 +36,16 @@ type server struct {
 }
 
 var testOAuthJSONPost = &hydra.OAuth2ClientJSON{
-	Name:       "test-name-2",
 	Scope:      "some,other,scopes",
 	GrantTypes: []string{"type2"},
+	Owner:      "test-name-2",
 }
 
 var testOAuthJSONPut = &hydra.OAuth2ClientJSON{
 	ClientID:   pointer.StringPtr("test-id-3"),
-	Name:       "test-name-3",
 	Scope:      "yet,another,scope",
 	GrantTypes: []string{"type3"},
+	Owner:      "test-name-3",
 }
 
 func TestCRUD(t *testing.T) {
@@ -160,9 +163,9 @@ func TestCRUD(t *testing.T) {
 				if new {
 					require.NotNil(t, o)
 
-					assert.Equal(testOAuthJSONPost.Name, o.Name)
 					assert.Equal(testOAuthJSONPost.Scope, o.Scope)
 					assert.Equal(testOAuthJSONPost.GrantTypes, o.GrantTypes)
+					assert.Equal(testOAuthJSONPost.Owner, o.Owner)
 					assert.NotNil(o.Secret)
 					assert.NotNil(o.ClientID)
 				}
@@ -213,10 +216,10 @@ func TestCRUD(t *testing.T) {
 				if ok {
 					require.NotNil(t, o)
 
-					assert.Equal(testOAuthJSONPut.Name, o.Name)
 					assert.Equal(testOAuthJSONPut.Scope, o.Scope)
 					assert.Equal(testOAuthJSONPut.GrantTypes, o.GrantTypes)
 					assert.Equal(testOAuthJSONPut.ClientID, o.ClientID)
+					assert.Equal(testOAuthJSONPut.Owner, o.Owner)
 					assert.NotNil(o.Secret)
 				}
 			})
@@ -260,6 +263,61 @@ func TestCRUD(t *testing.T) {
 			})
 		}
 	})
+
+	t.Run("method=list", func(t *testing.T) {
+
+		for d, tc := range map[string]server{
+			"no clients": {
+				http.StatusOK,
+				emptyBodyList,
+				nil,
+			},
+			"one client": {
+				http.StatusOK,
+				fmt.Sprintf("[%s]", testClientList),
+				nil,
+			},
+			"more clients": {
+				http.StatusOK,
+				fmt.Sprintf("[%s,%s]", testClientList, testClientList2),
+				nil,
+			},
+			"internal server error when requesting": {
+				http.StatusInternalServerError,
+				emptyBodyList,
+				errors.New("http request returned unexpected status code"),
+			},
+		} {
+			t.Run(fmt.Sprintf("case/%s", d), func(t *testing.T) {
+
+				//given
+				h := http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+					assert.Equal(c.HydraURL.String(), fmt.Sprintf("%s://%s%s", schemeHTTP, req.Host, req.URL.Path))
+					assert.Equal(http.MethodGet, req.Method)
+					w.WriteHeader(tc.statusCode)
+					w.Write([]byte(tc.respBody))
+					w.Header().Set("Content-type", "application/json")
+
+				})
+				runServer(&c, h)
+
+				//when
+				list, err := c.ListOAuth2Client()
+
+				//then
+				if tc.err == nil {
+					require.NoError(t, err)
+					require.NotNil(t, list)
+					var expectedList []*hydra.OAuth2ClientJSON
+					json.Unmarshal([]byte(tc.respBody), &expectedList)
+					assert.Equal(expectedList, list)
+				} else {
+					require.Error(t, err)
+					assert.Contains(err.Error(), tc.err.Error())
+				}
+			})
+		}
+	})
 }
 
 func runServer(c *hydra.Client, h http.HandlerFunc) {

hydra/types.go

@@ -6,10 +6,10 @@ import "k8s.io/utils/pointer"
 type OAuth2ClientJSON struct {
 	ClientID      *string  `json:"client_id,omitempty"`
 	Secret        *string  `json:"client_secret,omitempty"`
-	Name          string   `json:"client_name"`
 	GrantTypes    []string `json:"grant_types"`
 	ResponseTypes []string `json:"response_types,omitempty"`
 	Scope         string   `json:"scope"`
+	Owner         string   `json:"owner"`
 }
 
 // Oauth2ClientCredentials represents client ID and password fetched from a Kubernetes secret