fix: merge requested scopes instead of replacing them

backend/internal/oidc/authorization_service.go

@@ -16,7 +16,6 @@ import (
 	datatype "github.com/pocket-id/pocket-id/backend/internal/model/types"
 	"github.com/pocket-id/pocket-id/backend/internal/utils"
 	"gorm.io/gorm"
-	"gorm.io/gorm/clause"
 )
 
 func newAuthorizationService(db *gorm.DB, interactionSessionService *interactionSessionService, claimsService *ClaimsService, reauth ReauthenticationTokenConsumer, auditLog AuditLogger, apiAccess APIAccessProvider) *authorizationService {
@@ -768,41 +767,53 @@ func requiresReauthenticationForMaxAge(maxAgeRaw string, authenticationTime time
 func (s *authorizationService) consent(ctx context.Context, userID string, clientID string, scope []string) (hasAlreadyAuthorizedClient bool, err error) {
 	db := dbFromContext(ctx, s.db)
 
-	hasAlreadyAuthorizedClient, err = s.hasAuthorizedClient(ctx, clientID, userID, scope)
-	if err != nil {
-		return false, err
-	}
+	now := datatype.DateTime(time.Now())
 
-	if hasAlreadyAuthorizedClient {
-		err = db.
-			Model(&model.UserAuthorizedOidcClient{}).
-			Where("user_id = ? AND client_id = ?", userID, clientID).
-			Update("last_used_at", datatype.DateTime(time.Now())).
-			Error
+	var existing model.UserAuthorizedOidcClient
+	err = db.
+		First(&existing, "client_id = ? AND user_id = ?", clientID, userID).
+		Error
+	if err == nil {
+		// If the user has already authorized the client with the same or a superset of the requested scopes, we just update the last_used_at timestamp and return
+		hasAlreadyAuthorizedClient = consentScopesInclude(existing.Scope, scope)
+		if hasAlreadyAuthorizedClient {
+			err = db.
+				Model(&model.UserAuthorizedOidcClient{}).
+				Where("user_id = ? AND client_id = ?", userID, clientID).
+				Update("last_used_at", now).
+				Error
 
-		if err != nil {
 			return hasAlreadyAuthorizedClient, err
 		}
 
-		return hasAlreadyAuthorizedClient, nil
+		// If the user has already authorized the client but with a different set of scopes, we merge the scopes and update the record
+		err = db.
+			Model(&model.UserAuthorizedOidcClient{}).
+			Where("user_id = ? AND client_id = ?", userID, clientID).
+			Updates(map[string]any{
+				"scope":        mergeConsentScopes(existing.Scope, scope),
+				"last_used_at": now,
+			}).
+			Error
+
+		return hasAlreadyAuthorizedClient, err
+	}
+	if !errors.Is(err, gorm.ErrRecordNotFound) {
+		return false, err
 	}
 
-	userAuthorizedClient := model.UserAuthorizedOidcClient{
+	// The user has not authorized the client yet, so we create a new record
+	err = db.Create(&model.UserAuthorizedOidcClient{
 		UserID:     userID,
 		ClientID:   clientID,
-		Scope:      scope,
-		LastUsedAt: datatype.DateTime(time.Now()),
+		Scope:      mergeConsentScopes(nil, scope),
+		LastUsedAt: now,
+	}).Error
+	if errors.Is(err, gorm.ErrDuplicatedKey) {
+		return s.consent(ctx, userID, clientID, scope)
 	}
 
-	err = db.
-		Clauses(clause.OnConflict{
-			Columns:   []clause.Column{{Name: "user_id"}, {Name: "client_id"}},
-			DoUpdates: clause.AssignmentColumns([]string{"scope"}),
-		}).
-		Create(&userAuthorizedClient).
-		Error
-
-	return hasAlreadyAuthorizedClient, err
+	return false, err
 }
 
 func (s *authorizationService) hasAuthorizedClient(ctx context.Context, clientID, userID string, scope []string) (bool, error) {
@@ -817,14 +828,41 @@ func (s *authorizationService) hasAuthorizedClient(ctx context.Context, clientID
 		return false, err
 	}
 
-	authorizedScopes := userAuthorizedOidcClient.Scope
-	for _, requestedScope := range scope {
+	return consentScopesInclude(userAuthorizedOidcClient.Scope, scope), nil
+}
+
+func consentScopesInclude(authorizedScopes []string, requestedScopes []string) bool {
+	for _, requestedScope := range requestedScopes {
 		if !slices.Contains(authorizedScopes, requestedScope) {
-			return false, nil
+			return false
 		}
 	}
 
-	return true, nil
+	return true
+}
+
+// mergeConsentScopes merges the authorized scopes with the requested scopes, ensuring that there are no duplicates.
+// Merging is necessary because the user may authorize scopes for multiple resources.
+// If we would just replace the authorized scopes with the requested scopes, we would lose the previously authorized scopes for other resources.
+func mergeConsentScopes(authorizedScopes []string, requestedScopes []string) datatype.StringList {
+	merged := make(datatype.StringList, 0, len(authorizedScopes)+len(requestedScopes))
+	seen := make(map[string]struct{}, len(authorizedScopes)+len(requestedScopes))
+	for _, scope := range authorizedScopes {
+		if _, ok := seen[scope]; ok {
+			continue
+		}
+		seen[scope] = struct{}{}
+		merged = append(merged, scope)
+	}
+	for _, scope := range requestedScopes {
+		if _, ok := seen[scope]; ok {
+			continue
+		}
+		seen[scope] = struct{}{}
+		merged = append(merged, scope)
+	}
+
+	return merged
 }
 
 // IsUserGroupAllowedToAuthorize reports whether the user may use the group-restricted client.

backend/internal/oidc/authorization_service_test.go

@@ -171,6 +171,68 @@ func TestAuthorizationServiceConsentStepLogsNewClientAuthorization(t *testing.T)
 	require.Equal(t, model.AuditLogData{"clientName": "Test Client"}, auditLogger.data[0])
 }
 
+func TestAuthorizationServiceConsentMergesAudienceQualifiedScopeKeys(t *testing.T) {
+	db := testutils.NewDatabaseForTest(t)
+	auditLogger := &fakeAuditLogger{}
+
+	const (
+		userID   = "test-user"
+		clientID = "test-client"
+		apiA     = "https://api-a.example.com"
+		apiB     = "https://api-b.example.com"
+	)
+
+	service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, auditLogger, userAccess(map[string][]string{
+		apiA: {"read"},
+		apiB: {"read"},
+	}))
+
+	require.NoError(t, db.Create(&model.User{Base: model.Base{ID: userID}}).Error)
+	require.NoError(t, db.Create(&model.OidcClient{
+		Base: model.Base{ID: clientID},
+		Name: "Test Client",
+	}).Error)
+
+	apiAConsent := consentScopeKeys(apiA, []string{"openid", "read"})
+	apiBConsent := consentScopeKeys(apiB, []string{"openid", "read"})
+
+	hasAlreadyAuthorized, err := service.consent(t.Context(), userID, clientID, apiAConsent)
+	require.NoError(t, err)
+	require.False(t, hasAlreadyAuthorized)
+
+	hasAlreadyAuthorized, err = service.consent(t.Context(), userID, clientID, apiBConsent)
+	require.NoError(t, err)
+	require.False(t, hasAlreadyAuthorized)
+
+	var authorizedClient model.UserAuthorizedOidcClient
+	require.NoError(t, db.First(&authorizedClient, "user_id = ? AND client_id = ?", userID, clientID).Error)
+	require.ElementsMatch(t, []string{"openid", consentScopeKey(apiA, "read"), consentScopeKey(apiB, "read")}, authorizedClient.Scope)
+
+	hasAuthorizedAPIA, err := service.hasAuthorizedClient(t.Context(), clientID, userID, apiAConsent)
+	require.NoError(t, err)
+	require.True(t, hasAuthorizedAPIA)
+	hasAuthorizedAPIB, err := service.hasAuthorizedClient(t.Context(), clientID, userID, apiBConsent)
+	require.NoError(t, err)
+	require.True(t, hasAuthorizedAPIB)
+
+	form := url.Values{
+		"prompt":   {"none"},
+		"resource": {apiA},
+	}
+	requester := newTestAuthorizeRequesterWithForm("silent-api-a-request", clientID, form)
+	requester.(*fosite.AuthorizeRequest).RequestedScope = fosite.Arguments{"openid", "read"}
+
+	authorization, err := service.authorize(t.Context(), authorizeInput{
+		userID:             userID,
+		authenticationTime: time.Now().UTC(),
+		requester:          requester,
+		meta:               requestMeta{IPAddress: "203.0.113.1", UserAgent: "test-agent"},
+	})
+	require.NoError(t, err)
+	require.False(t, authorization.RequiresInteraction)
+	require.Equal(t, []model.AuditLogEvent{model.AuditLogEventClientAuthorization}, auditLogger.events)
+}
+
 func TestAuthorizationServiceAuthorizeConsumesInteractionSession(t *testing.T) {
 	db := testutils.NewDatabaseForTest(t)
 	service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil), nil, nil, nil)