Use correct sql condition when listing clients for update (#1824)

* Use correct sql condition * Improve message
kyma-incubator · Apr 7, 2021 · 2a5a26c · 2a5a26c
1 parent e29dd54
commit 2a5a26c
Show file tree

Hide file tree

Showing 3 changed files with 75 additions and 5 deletions.
diff --git a/chart/compass/values.yaml b/chart/compass/values.yaml
@@ -53,7 +53,7 @@ global:
       version: "PR-1808"
     director:
       dir:
-      version: "PR-1823"
+      version: "PR-1824"
     gateway:
       dir:
       version: "PR-1783"

diff --git a/components/director/internal/scopes_sync/service.go b/components/director/internal/scopes_sync/service.go
@@ -59,6 +59,12 @@ func (s *service) SynchronizeClientScopes(ctx context.Context) error {
 
 	areAllClientsUpdated := true
 	for _, auth := range auths {
+		log.C(ctx).Infof("Synchronizing oauth client of system auth with ID %s", auth.ID)
+		if auth.Value == nil || auth.Value.Credential.Oauth == nil {
+			log.C(ctx).Infof("System auth with ID %s does not have oauth client for update", auth.ID)
+			continue
+		}
+
 		clientID := auth.Value.Credential.Oauth.ClientID
 
 		objType, err := auth.GetReferenceObjectType()
@@ -115,7 +121,7 @@ func (s *service) systemAuthsWithOAuth(ctx context.Context) ([]model.SystemAuth,
 	ctx = persistence.SaveToContext(ctx, tx)
 
 	conditions := repo.Conditions{
-		repo.NewNotNullCondition("(value -> 'Credential' -> 'Oauth')"),
+		repo.NewNotEqualCondition("(value -> 'Credential' -> 'Oauth')", "null"),
 	}
 	auths, err := s.repo.ListGlobalWithConditions(ctx, conditions)
 	if err != nil {

diff --git a/components/director/internal/scopes_sync/service_test.go b/components/director/internal/scopes_sync/service_test.go
@@ -20,7 +20,7 @@ func TestSyncService_UpdateClientScopes(t *testing.T) {
 
 	const clientID = "client-id"
 	selectCondition := repo.Conditions{
-		repo.NewNotNullCondition("(value -> 'Credential' -> 'Oauth')"),
+		repo.NewNotEqualCondition("(value -> 'Credential' -> 'Oauth')", "null"),
 	}
 
 	t.Run("fails when oauth service cannot list clients", func(t *testing.T) {
@@ -106,7 +106,7 @@ func TestSyncService_UpdateClientScopes(t *testing.T) {
 		// WHEN
 		err := scopeSyncSvc.SynchronizeClientScopes(context.TODO())
 		// THEN
-		assert.Error(t, err, "Not all clients were updated successfully")
+		assert.EqualError(t, err, "Not all clients were updated successfully")
 	})
 
 	t.Run("won't update client when getting client credentials scopes fails", func(t *testing.T) {
@@ -135,7 +135,7 @@ func TestSyncService_UpdateClientScopes(t *testing.T) {
 		// WHEN
 		err := scopeSyncSvc.SynchronizeClientScopes(context.TODO())
 		// THEN
-		assert.Error(t, err, "Not all clients were updated successfully")
+		assert.EqualError(t, err, "Not all clients were updated successfully")
 	})
 
 	t.Run("fails when client does not present in hydra", func(t *testing.T) {
@@ -201,6 +201,70 @@ func TestSyncService_UpdateClientScopes(t *testing.T) {
 		assert.Nil(t, err)
 	})
 
+	t.Run("won't update scopes when returned client is not for update", func(t *testing.T) {
+		// GIVEN
+		oauthSvc := &automock.OAuthService{}
+		systemAuthRepo := &automock.SystemAuthRepo{}
+		oauthSvc.On("ListClients").Return([]*models.OAuth2Client{
+			{
+				ClientID: clientID,
+				Scope:    "scope",
+			},
+		}, nil)
+		mockedTx, transactioner := txtest.NewTransactionContextGenerator(nil).ThatSucceeds()
+		systemAuthRepo.On("ListGlobalWithConditions", mock.Anything, selectCondition).Return([]model.SystemAuth{
+			{
+				AppID: str.Ptr("app-id"),
+				Value: &model.Auth{
+					Credential: model.CredentialData{},
+				},
+			},
+		}, nil)
+		defer mockedTx.AssertExpectations(t)
+		defer transactioner.AssertExpectations(t)
+		defer oauthSvc.AssertExpectations(t)
+		scopeSyncSvc := NewService(oauthSvc, transactioner, systemAuthRepo)
+		// WHEN
+		err := scopeSyncSvc.SynchronizeClientScopes(context.TODO())
+		// THEN
+		assert.Nil(t, err)
+	})
+
+	t.Run("fails when client update in Hydra fails", func(t *testing.T) {
+		// GIVEN
+		oauthSvc := &automock.OAuthService{}
+		systemAuthRepo := &automock.SystemAuthRepo{}
+		oauthSvc.On("ListClients").Return([]*models.OAuth2Client{
+			{
+				ClientID: clientID,
+				Scope:    "first",
+			},
+		}, nil)
+		oauthSvc.On("GetClientCredentialScopes", model.ApplicationReference).Return([]string{"first", "second"}, nil)
+		oauthSvc.On("UpdateClientScopes", mock.Anything, "client-id", model.ApplicationReference).Return(errors.New("fail"))
+		mockedTx, transactioner := txtest.NewTransactionContextGenerator(errors.New("error")).ThatSucceeds()
+		systemAuthRepo.On("ListGlobalWithConditions", mock.Anything, selectCondition).Return([]model.SystemAuth{
+			{
+				AppID: str.Ptr("app-id"),
+				Value: &model.Auth{
+					Credential: model.CredentialData{
+						Oauth: &model.OAuthCredentialData{
+							ClientID: clientID,
+						},
+					},
+				},
+			},
+		}, nil)
+		defer mockedTx.AssertExpectations(t)
+		defer transactioner.AssertExpectations(t)
+		defer oauthSvc.AssertExpectations(t)
+		scopeSyncSvc := NewService(oauthSvc, transactioner, systemAuthRepo)
+		// WHEN
+		err := scopeSyncSvc.SynchronizeClientScopes(context.TODO())
+		// THEN
+		assert.EqualError(t, err, "Not all clients were updated successfully")
+	})
+
 	t.Run("will update scopes successfully", func(t *testing.T) {
 		// GIVEN
 		oauthSvc := &automock.OAuthService{}