diff --git a/backend/internal/oidc/authorization_handler.go b/backend/internal/oidc/authorization_handler.go
index 9085a2cd..442b628b 100644
--- a/backend/internal/oidc/authorization_handler.go
+++ b/backend/internal/oidc/authorization_handler.go
@@ -128,7 +128,7 @@ func (h *authorizationHandler) completeInteraction(c *gin.Context) {
}
reauthenticationToken, _ := c.Cookie(cookie.ReauthenticationTokenCookieName)
- response, err := h.authorizationService.completeInteractionStep(c.Request.Context(), interactionID, principal.UserID, request.Step, reauthenticationToken, principal.AuthenticationTime, requestMetaFromGin(c))
+ response, err := h.authorizationService.completeInteractionStep(c.Request.Context(), interactionID, principal.UserID, request.Step, request.Scopes, reauthenticationToken, principal.AuthenticationTime, requestMetaFromGin(c))
if err != nil {
_ = c.Error(err)
return
diff --git a/backend/internal/oidc/authorization_service.go b/backend/internal/oidc/authorization_service.go
index 1cf16003..6e151cad 100644
--- a/backend/internal/oidc/authorization_service.go
+++ b/backend/internal/oidc/authorization_service.go
@@ -581,7 +581,7 @@ func (s *authorizationService) resolveScopeInfoForRequest(ctx context.Context, r
return infos, nil
}
-func (s *authorizationService) completeInteractionStep(ctx context.Context, interactionSessionID, userID string, step interactionStep, reauthenticationToken string, authenticationTime time.Time, meta requestMeta) (completeInteractionResponse, error) {
+func (s *authorizationService) completeInteractionStep(ctx context.Context, interactionSessionID, userID string, step interactionStep, approvedScopes []string, reauthenticationToken string, authenticationTime time.Time, meta requestMeta) (completeInteractionResponse, error) {
var interactionSession InteractionSession
var response completeInteractionResponse
err := withTx(ctx, s.db, func(ctx context.Context) error {
@@ -614,7 +614,7 @@ func (s *authorizationService) completeInteractionStep(ctx context.Context, inte
return apperror.ValidationMessage("expected interaction step " + string(requiredSteps[0]) + " but got " + string(step))
}
- if err := s.applyInteractionStep(ctx, &interactionSession, userID, step, reauthenticationToken, authenticationTime, meta); err != nil {
+ if err := s.applyInteractionStep(ctx, &interactionSession, userID, step, approvedScopes, reauthenticationToken, authenticationTime, meta); err != nil {
return err
}
@@ -639,7 +639,7 @@ func (s *authorizationService) completeInteractionStep(ctx context.Context, inte
return completeInteractionResponse{Interaction: &interaction}, nil
}
-func (s *authorizationService) applyInteractionStep(ctx context.Context, interactionSession *InteractionSession, userID string, step interactionStep, reauthenticationToken string, authenticationTime time.Time, meta requestMeta) error {
+func (s *authorizationService) applyInteractionStep(ctx context.Context, interactionSession *InteractionSession, userID string, step interactionStep, approvedScopes []string, reauthenticationToken string, authenticationTime time.Time, meta requestMeta) error {
switch step {
case interactionStepAuthenticate:
if err := bindInteractionSessionUser(interactionSession, userID); err != nil {
@@ -656,7 +656,7 @@ func (s *authorizationService) applyInteractionStep(ctx context.Context, interac
case interactionStepReauthenticate:
return s.completeReauthenticationStep(ctx, interactionSession, userID, reauthenticationToken)
case interactionStepConsent:
- return s.completeConsentStep(ctx, interactionSession, userID, meta)
+ return s.completeConsentStep(ctx, interactionSession, userID, approvedScopes, meta)
default:
return apperror.ValidationMessage("unknown interaction step " + string(step))
}
@@ -679,10 +679,17 @@ func (s *authorizationService) completeReauthenticationStep(ctx context.Context,
return nil
}
-func (s *authorizationService) completeConsentStep(ctx context.Context, interactionSession *InteractionSession, userID string, meta requestMeta) error {
+func (s *authorizationService) completeConsentStep(ctx context.Context, interactionSession *InteractionSession, userID string, approvedScopes []string, meta requestMeta) error {
if err := bindInteractionSessionUser(interactionSession, userID); err != nil {
return err
}
+
+ // Narrow the request to the scopes the user approved before recording consent, so deselected scopes are neither consented to nor granted
+ if err := narrowInteractionScopes(interactionSession, approvedScopes); err != nil {
+ return err
+ }
+
+ // Record consent for the approved scopes only
resource := interactionSession.Parameters["resource"]
_, _, consentKeys, err := s.resolveGrant(ctx, interactionSession.ClientID, resource, interactionSession.Scopes)
if err != nil {
@@ -699,6 +706,56 @@ func (s *authorizationService) completeConsentStep(ctx context.Context, interact
return nil
}
+// requiredConsentScopes are the scopes a user cannot deselect on the consent screen because the client cannot sign the user in without them
+var requiredConsentScopes = []string{"openid"}
+
+// narrowApprovedScopes restricts the requested scopes to the ones the user approved on a consent screen
+// The browser and device consent flows share it so both apply the same rules to a selection
+// A nil approval keeps every requested scope, so callers that send no selection keep the all-or-nothing behavior
+func narrowApprovedScopes(requestedScopes, approvedScopes []string) ([]string, error) {
+ if approvedScopes == nil {
+ return requestedScopes, nil
+ }
+
+ // The approval may only remove scopes, never add ones the client did not request
+ for _, scope := range approvedScopes {
+ if !slices.Contains(requestedScopes, scope) {
+ return nil, apperror.ValidationMessage("scope " + scope + " was not requested")
+ }
+ }
+
+ // Keep the requested order so the narrowed request only differs from the original by the removed scopes
+ narrowed := make([]string, 0, len(requestedScopes))
+ for _, scope := range requestedScopes {
+ if slices.Contains(approvedScopes, scope) || slices.Contains(requiredConsentScopes, scope) {
+ narrowed = append(narrowed, scope)
+ }
+ }
+
+ return narrowed, nil
+}
+
+// narrowInteractionScopes restricts the interaction session to the scopes the user approved on the consent screen
+// The stored scope parameter is rewritten too because the resumed authorize request is rebuilt from it, which keeps deselected scopes out of the granted scopes and the issued tokens
+func narrowInteractionScopes(interactionSession *InteractionSession, approvedScopes []string) error {
+ if approvedScopes == nil {
+ return nil
+ }
+
+ narrowed, err := narrowApprovedScopes(interactionSession.Scopes, approvedScopes)
+ if err != nil {
+ return err
+ }
+ interactionSession.Scopes = narrowed
+
+ if interactionSession.Parameters == nil {
+ interactionSession.Parameters = InteractionSessionParameters{}
+ }
+ interactionSession.Parameters["scope"] = strings.Join(narrowed, " ")
+
+ return nil
+}
+
func (s *authorizationService) populatePostAuthenticationRequirements(ctx context.Context, userID string, interactionSession *InteractionSession, authenticationTime time.Time) error {
if userID == "" {
return errors.New("user is required to complete authentication")
diff --git a/backend/internal/oidc/authorization_service_test.go b/backend/internal/oidc/authorization_service_test.go
index 73188d6f..603f0e62 100644
--- a/backend/internal/oidc/authorization_service_test.go
+++ b/backend/internal/oidc/authorization_service_test.go
@@ -165,7 +165,7 @@ func TestAuthorizationServiceConsentStepLogsNewClientAuthorization(t *testing.T)
Parameters: map[string]string{},
}).Error)
- response, err := service.completeInteractionStep(t.Context(), interactionID, userID, interactionStepConsent, "", time.Now().UTC(), requestMeta{})
+ response, err := service.completeInteractionStep(t.Context(), interactionID, userID, interactionStepConsent, nil, "", time.Now().UTC(), requestMeta{})
require.NoError(t, err)
require.NotEmpty(t, response.RedirectURL)
@@ -173,6 +173,147 @@ func TestAuthorizationServiceConsentStepLogsNewClientAuthorization(t *testing.T)
require.Equal(t, auditlogs.Data{"clientName": "Test Client"}, auditLogger.data[0])
}
+// TestAuthorizationServiceConsentStepGrantsOnlyApprovedScopes guards that scopes the user deselects on the consent screen are neither consented to nor granted when the request resumes
+func TestAuthorizationServiceConsentStepGrantsOnlyApprovedScopes(t *testing.T) {
+ db := testutils.NewDatabaseForTest(t)
+
+ const (
+ userID = "test-user"
+ clientID = "test-client"
+ interactionID = "test-interaction"
+ api = "https://api.example.com"
+ )
+
+ service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, userAccess(map[string][]string{
+ api: {"users.read", "users.write"},
+ }))
+
+ 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)
+ require.NoError(t, db.Create(&InteractionSession{
+ Base: model.Base{ID: interactionID},
+ Scopes: datatype.StringList{"openid", "email", "users.read", "users.write"},
+ ClientID: clientID,
+ ConsentRequired: true,
+ RequestedAt: datatype.DateTime(time.Now().UTC()),
+ Parameters: map[string]string{"scope": "openid email users.read users.write", "resource": api},
+ }).Error)
+
+ // The user deselects users.write and does not send openid, which must still be kept
+ response, err := service.completeInteractionStep(t.Context(), interactionID, userID, interactionStepConsent, []string{"email", "users.read"}, "", time.Now().UTC(), requestMeta{})
+ require.NoError(t, err)
+ require.NotEmpty(t, response.RedirectURL)
+
+ // The interaction session and the parameters the resumed request is rebuilt from only carry the approved scopes
+ var interactionSession InteractionSession
+ require.NoError(t, db.First(&interactionSession, "id = ?", interactionID).Error)
+ require.Equal(t, datatype.StringList{"openid", "email", "users.read"}, interactionSession.Scopes)
+ require.Equal(t, "openid email users.read", interactionSession.Parameters["scope"])
+
+ // Consent is only recorded for the approved scopes
+ var authorizedClient model.UserAuthorizedOidcClient
+ require.NoError(t, db.First(&authorizedClient, "user_id = ? AND client_id = ?", userID, clientID).Error)
+ require.Contains(t, authorizedClient.Scope, consentScopeKey(api, "users.read"))
+ require.NotContains(t, authorizedClient.Scope, consentScopeKey(api, "users.write"))
+
+ authorize := func(scopes ...string) (fosite.AuthorizeRequester, authorizationResult, error) {
+ requester := newTestAuthorizeRequesterWithForm("resumed-request", clientID, url.Values{"resource": {api}})
+ requester.(*fosite.AuthorizeRequest).RequestedScope = fosite.Arguments(scopes)
+ authorization, err := service.authorize(t.Context(), authorizeInput{
+ userID: userID,
+ authenticationTime: time.Now().UTC(),
+ requester: requester,
+ interactionID: interactionID,
+ })
+ return requester, authorization, err
+ }
+
+ // Replaying the original scopes must not grant the deselected one
+ _, _, err = authorize("openid", "email", "users.read", "users.write")
+ require.ErrorIs(t, err, fosite.ErrInvalidRequest)
+
+ // The resumed request grants exactly the approved scopes
+ requester, authorization, err := authorize("openid", "email", "users.read")
+ require.NoError(t, err)
+ require.False(t, authorization.RequiresInteraction)
+ require.ElementsMatch(t, []string{"openid", "email", "users.read"}, []string(requester.GetGrantedScopes()))
+}
+
+func TestAuthorizationServiceConsentStepRejectsUnrequestedScopes(t *testing.T) {
+ db := testutils.NewDatabaseForTest(t)
+ service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, "", nil, appconfig.StaticResolver{}), nil, nil, nil)
+
+ const (
+ userID = "test-user"
+ clientID = "test-client"
+ interactionID = "test-interaction"
+ )
+
+ 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)
+ require.NoError(t, db.Create(&InteractionSession{
+ Base: model.Base{ID: interactionID},
+ Scopes: datatype.StringList{"openid", "email"},
+ ClientID: clientID,
+ ConsentRequired: true,
+ RequestedAt: datatype.DateTime(time.Now().UTC()),
+ Parameters: map[string]string{"scope": "openid email"},
+ }).Error)
+
+ // The approval may only remove scopes, so a scope the client never requested is rejected
+ _, err := service.completeInteractionStep(t.Context(), interactionID, userID, interactionStepConsent, []string{"email", "groups"}, "", time.Now().UTC(), requestMeta{})
+ require.Error(t, err)
+
+ // Nothing is consented to and the interaction is left untouched
+ var interactionSession InteractionSession
+ require.NoError(t, db.First(&interactionSession, "id = ?", interactionID).Error)
+ require.True(t, interactionSession.ConsentRequired)
+ require.Equal(t, datatype.StringList{"openid", "email"}, interactionSession.Scopes)
+ require.ErrorIs(t, db.First(&model.UserAuthorizedOidcClient{}, "user_id = ? AND client_id = ?", userID, clientID).Error, gorm.ErrRecordNotFound)
+}
+
+func TestNarrowInteractionScopes(t *testing.T) {
+ newSession := func() *InteractionSession {
+ return &InteractionSession{
+ Scopes: datatype.StringList{"openid", "profile", "email", "offline_access"},
+ Parameters: map[string]string{"scope": "openid profile email offline_access"},
+ }
+ }
+
+ t.Run("nil approval keeps every requested scope", func(t *testing.T) {
+ session := newSession()
+ require.NoError(t, narrowInteractionScopes(session, nil))
+ require.Equal(t, datatype.StringList{"openid", "profile", "email", "offline_access"}, session.Scopes)
+ require.Equal(t, "openid profile email offline_access", session.Parameters["scope"])
+ })
+
+ t.Run("empty approval keeps only the required scopes", func(t *testing.T) {
+ session := newSession()
+ require.NoError(t, narrowInteractionScopes(session, []string{}))
+ require.Equal(t, datatype.StringList{"openid"}, session.Scopes)
+ require.Equal(t, "openid", session.Parameters["scope"])
+ })
+
+ t.Run("approval keeps the requested order", func(t *testing.T) {
+ session := newSession()
+ require.NoError(t, narrowInteractionScopes(session, []string{"offline_access", "profile"}))
+ require.Equal(t, datatype.StringList{"openid", "profile", "offline_access"}, session.Scopes)
+ require.Equal(t, "openid profile offline_access", session.Parameters["scope"])
+ })
+
+ t.Run("approval of an unrequested scope is rejected", func(t *testing.T) {
+ session := newSession()
+ require.Error(t, narrowInteractionScopes(session, []string{"groups"}))
+ require.Equal(t, datatype.StringList{"openid", "profile", "email", "offline_access"}, session.Scopes)
+ })
+
+ t.Run("nil parameters are initialized", func(t *testing.T) {
+ session := &InteractionSession{Scopes: datatype.StringList{"openid", "email"}}
+ require.NoError(t, narrowInteractionScopes(session, []string{}))
+ require.Equal(t, "openid", session.Parameters["scope"])
+ })
+}
+
func TestAuthorizationServiceConsentMergesAudienceQualifiedScopeKeys(t *testing.T) {
db := testutils.NewDatabaseForTest(t)
auditLogger := &fakeAuditLogger{}
@@ -382,7 +523,7 @@ func TestInteractionAPIClassifiesMissingSession(t *testing.T) {
_, err := service.getInteractionSession(t.Context(), "missing-interaction")
require.True(t, apperror.IsCode(err, apperror.CodeNotFound))
- _, err = service.completeInteractionStep(t.Context(), "missing-interaction", "user", interactionStepConsent, "", time.Now().UTC(), requestMeta{})
+ _, err = service.completeInteractionStep(t.Context(), "missing-interaction", "user", interactionStepConsent, nil, "", time.Now().UTC(), requestMeta{})
require.True(t, apperror.IsCode(err, apperror.CodeNotFound))
}
@@ -599,7 +740,7 @@ func TestAuthorizationServiceCompleteInteractionBindsUserToSession(t *testing.T)
Parameters: map[string]string{},
}).Error)
- response, err := service.completeInteractionStep(t.Context(), interactionID, userID, interactionStepConsent, "", time.Now().UTC(), requestMeta{})
+ response, err := service.completeInteractionStep(t.Context(), interactionID, userID, interactionStepConsent, nil, "", time.Now().UTC(), requestMeta{})
require.NoError(t, err)
require.NotEmpty(t, response.RedirectURL)
@@ -644,7 +785,7 @@ func TestAuthorizationServiceCompleteInteractionSwitchesUserAndResetsRequirement
},
}).Error)
- response, err := service.completeInteractionStep(t.Context(), interactionID, otherUserID, interactionStepSelectAccount, "", time.Now().UTC(), requestMeta{})
+ response, err := service.completeInteractionStep(t.Context(), interactionID, otherUserID, interactionStepSelectAccount, nil, "", time.Now().UTC(), requestMeta{})
require.NoError(t, err)
require.Empty(t, response.RedirectURL)
require.NotNil(t, response.Interaction)
@@ -692,7 +833,7 @@ func TestAuthorizationServiceSelectAccountRecomputesConsentForSelectedUser(t *te
require.NoError(t, err)
require.True(t, authorization.RequiresInteraction)
- response, err := service.completeInteractionStep(t.Context(), authorization.InteractionID, switchedID, interactionStepSelectAccount, "", time.Now().UTC(), requestMeta{})
+ response, err := service.completeInteractionStep(t.Context(), authorization.InteractionID, switchedID, interactionStepSelectAccount, nil, "", time.Now().UTC(), requestMeta{})
require.NoError(t, err)
// The flow must not be granted yet: consent is still pending for the switched-to user.
diff --git a/backend/internal/oidc/device_handler.go b/backend/internal/oidc/device_handler.go
index 330ef4bb..65323348 100644
--- a/backend/internal/oidc/device_handler.go
+++ b/backend/internal/oidc/device_handler.go
@@ -9,9 +9,16 @@ import (
"github.com/pocket-id/fosite"
"github.com/pocket-id/pocket-id/backend/internal/apperror"
"github.com/pocket-id/pocket-id/backend/internal/authz"
+ "github.com/pocket-id/pocket-id/backend/internal/httpserver"
"github.com/pocket-id/pocket-id/backend/internal/utils/cookie"
)
+type verifyDeviceCodeRequest struct {
+ // Scopes are the scopes the user approved on the consent screen
+ // Omitting them approves every requested scope
+ Scopes []string `json:"scopes"`
+}
+
type deviceHandler struct {
provider fosite.OAuth2Provider
deviceService *deviceService
@@ -48,12 +55,20 @@ func (h *deviceHandler) verifyDeviceCode(c *gin.Context) {
return
}
+ // The body is optional so callers that send no scope selection approve every requested scope
+ var request verifyDeviceCodeRequest
+ if err := httpserver.BindOptionalJSON(c, &request); err != nil {
+ _ = c.Error(err)
+ return
+ }
+
err := h.deviceService.acceptDeviceCode(
c.Request.Context(),
userCode,
principal.UserID,
principal.AuthenticationMethod,
principal.AuthenticationTime,
+ request.Scopes,
reauthenticationToken,
requestMetaFromGin(c),
)
diff --git a/backend/internal/oidc/device_openid_session_test.go b/backend/internal/oidc/device_openid_session_test.go
index 40576daa..cf96a055 100644
--- a/backend/internal/oidc/device_openid_session_test.go
+++ b/backend/internal/oidc/device_openid_session_test.go
@@ -18,7 +18,7 @@ func TestDeviceCodeExchangeConsumesOpenIDConnectSession(t *testing.T) {
clientID = "test-client"
)
service, store, provider, userCode, deviceCode := newTestDeviceServiceWithCode(t, clientID, userID, false, nil)
- require.NoError(t, service.acceptDeviceCode(t.Context(), userCode, userID, "phr", time.Now().UTC(), "", requestMeta{}))
+ require.NoError(t, service.acceptDeviceCode(t.Context(), userCode, userID, "phr", time.Now().UTC(), nil, "", requestMeta{}))
signature, err := provider.deviceStrategy.DeviceCodeSignature(t.Context(), deviceCode)
require.NoError(t, err)
diff --git a/backend/internal/oidc/device_service.go b/backend/internal/oidc/device_service.go
index 598689b5..25e04473 100644
--- a/backend/internal/oidc/device_service.go
+++ b/backend/internal/oidc/device_service.go
@@ -4,6 +4,7 @@ import (
"context"
"errors"
"net/http"
+ "slices"
"strings"
"time"
@@ -85,7 +86,7 @@ func (s *deviceService) createDeviceAuthorization(ctx context.Context, req *http
}, request, nil
}
-func (s *deviceService) acceptDeviceCode(ctx context.Context, userCode, userID, authenticationMethod string, authenticationTime time.Time, reauthenticationToken string, meta requestMeta) error {
+func (s *deviceService) acceptDeviceCode(ctx context.Context, userCode, userID, authenticationMethod string, authenticationTime time.Time, approvedScopes []string, reauthenticationToken string, meta requestMeta) error {
request, userCodeSignature, err := s.deviceRequestFromUserCode(ctx, userCode)
if err != nil {
return err
@@ -107,6 +108,11 @@ func (s *deviceService) acceptDeviceCode(ctx context.Context, userCode, userID,
return fosite.ErrAccessDenied.WithHint("You are not allowed to access this service.")
}
+ // Narrow the request to the scopes the user approved before resolving the grant, so deselected scopes are neither consented to nor granted
+ if err := narrowDeviceRequestScopes(request, approvedScopes); err != nil {
+ return err
+ }
+
resource, err := request.GetResource()
if err != nil {
return err
@@ -169,6 +175,31 @@ func (s *deviceService) acceptDeviceCode(ctx context.Context, userCode, userID,
})
}
+// narrowDeviceRequestScopes restricts a pending device authorization to the scopes the user approved on the consent screen
+// The device authorization endpoint already granted every allowed scope, so the grants of deselected scopes are dropped as well
+func narrowDeviceRequestScopes(request fosite.DeviceRequester, approvedScopes []string) error {
+ if approvedScopes == nil {
+ return nil
+ }
+
+ narrowed, err := narrowApprovedScopes(request.GetRequestedScopes(), approvedScopes)
+ if err != nil {
+ return err
+ }
+
+ // The requester interface can only add grants, so the stored device request is changed directly
+ deviceRequest, ok := request.(*fosite.DeviceRequest)
+ if !ok {
+ return errors.New("unexpected device request type")
+ }
+ deviceRequest.SetRequestedScopes(narrowed)
+ deviceRequest.GrantedScope = slices.DeleteFunc(deviceRequest.GrantedScope, func(scope string) bool {
+ return !slices.Contains(narrowed, scope)
+ })
+
+ return nil
+}
+
// copyDeviceCodeExpiry copies the device and user code expiry from the pending device authorization's session to the session that replaces it
func copyDeviceCodeExpiry(from fosite.Session, to *Session) {
if from == nil {
diff --git a/backend/internal/oidc/device_service_test.go b/backend/internal/oidc/device_service_test.go
index 78229cbc..99b39348 100644
--- a/backend/internal/oidc/device_service_test.go
+++ b/backend/internal/oidc/device_service_test.go
@@ -52,7 +52,7 @@ func TestDeviceServiceAcceptRequiresReauthenticationTokenWhenClientRequiresIt(t
}
service, _, _, userCode, _ := newTestDeviceServiceWithCode(t, clientID, userID, true, reauth)
- err := service.acceptDeviceCode(t.Context(), userCode, userID, "phr", time.Now().UTC(), "", requestMeta{})
+ err := service.acceptDeviceCode(t.Context(), userCode, userID, "phr", time.Now().UTC(), nil, "", requestMeta{})
require.True(t, apperror.IsCode(err, apperror.CodeReauthenticationRequired))
require.Zero(t, reauth.calls)
@@ -60,7 +60,7 @@ func TestDeviceServiceAcceptRequiresReauthenticationTokenWhenClientRequiresIt(t
require.NoError(t, err)
require.True(t, info.ReauthenticationRequired)
- err = service.acceptDeviceCode(t.Context(), userCode, userID, "phr", time.Now().UTC(), reauth.token, requestMeta{})
+ err = service.acceptDeviceCode(t.Context(), userCode, userID, "phr", time.Now().UTC(), nil, reauth.token, requestMeta{})
require.NoError(t, err)
require.Equal(t, 1, reauth.calls)
}
@@ -86,7 +86,7 @@ func TestDeviceServiceAcceptUsesReauthenticationTimeForDeviceSession(t *testing.
}
service, store, provider, userCode, deviceCode := newTestDeviceServiceWithCode(t, clientID, userID, true, reauth)
- err := service.acceptDeviceCode(t.Context(), userCode, userID, "phr", time.Now().Add(-time.Hour).UTC(), reauth.token, requestMeta{})
+ err := service.acceptDeviceCode(t.Context(), userCode, userID, "phr", time.Now().Add(-time.Hour).UTC(), nil, reauth.token, requestMeta{})
require.NoError(t, err)
deviceCodeSignature, err := provider.deviceStrategy.DeviceCodeSignature(t.Context(), deviceCode)
@@ -97,6 +97,46 @@ func TestDeviceServiceAcceptUsesReauthenticationTimeForDeviceSession(t *testing.
require.Equal(t, reauthenticatedAt, session.IDTokenClaims().AuthTime)
}
+// TestDeviceServiceAcceptGrantsOnlyApprovedScopes guards that scopes the user deselects on the device consent screen are neither consented to nor granted, even though the device authorization endpoint granted them up front
+func TestDeviceServiceAcceptGrantsOnlyApprovedScopes(t *testing.T) {
+ const (
+ userID = "test-user"
+ clientID = "test-client"
+ )
+ service, store, provider := newTestDeviceService(t, clientID, userID, false, nil, nil)
+ userCode, deviceCode := createTestDeviceCode(t, service, clientID, "openid email groups")
+
+ // The user deselects email and does not send openid, which must still be kept
+ require.NoError(t, service.acceptDeviceCode(t.Context(), userCode, userID, "phr", time.Now().UTC(), []string{"groups"}, "", requestMeta{}))
+
+ // Consent is only recorded for the approved scopes
+ var authorizedClient model.UserAuthorizedOidcClient
+ require.NoError(t, store.db.First(&authorizedClient, "user_id = ? AND client_id = ?", userID, clientID).Error)
+ require.ElementsMatch(t, []string{"openid", "groups"}, []string(authorizedClient.Scope))
+
+ // The token the device receives only carries the approved scopes
+ response, err := exchangeDeviceCode(t, provider, clientID, deviceCode)
+ require.NoError(t, err)
+ require.Equal(t, "openid groups", response.GetExtra("scope"))
+}
+
+func TestDeviceServiceAcceptRejectsUnrequestedScopes(t *testing.T) {
+ const (
+ userID = "test-user"
+ clientID = "test-client"
+ )
+ service, store, _ := newTestDeviceService(t, clientID, userID, false, nil, nil)
+ userCode, _ := createTestDeviceCode(t, service, clientID, "openid email")
+
+ // The approval may only remove scopes, so a scope the device never requested is rejected
+ err := service.acceptDeviceCode(t.Context(), userCode, userID, "phr", time.Now().UTC(), []string{"email", "groups"}, "", requestMeta{})
+ require.Error(t, err)
+
+ // Nothing is consented to and the user code can still be approved
+ require.ErrorIs(t, store.db.First(&model.UserAuthorizedOidcClient{}, "user_id = ? AND client_id = ?", userID, clientID).Error, gorm.ErrRecordNotFound)
+ require.NoError(t, service.acceptDeviceCode(t.Context(), userCode, userID, "phr", time.Now().UTC(), nil, "", requestMeta{}))
+}
+
func TestDeviceServiceCreateRejectsResourceWithoutUserDelegatedGrant(t *testing.T) {
const (
userID = "test-user"
@@ -129,17 +169,25 @@ func newTestDeviceServiceWithCode(t *testing.T, clientID, userID string, require
t.Helper()
service, store, provider := newTestDeviceService(t, clientID, userID, requiresReauthentication, reauth, nil)
+ userCode, deviceCode := createTestDeviceCode(t, service, clientID, "openid")
+
+ return service, store, provider, userCode, deviceCode
+}
+
+// createTestDeviceCode starts a device authorization for the given scope like a device would
+func createTestDeviceCode(t *testing.T, service *deviceService, clientID, scope string) (userCode string, deviceCode string) {
+ t.Helper()
form := url.Values{
"client_id": {clientID},
- "scope": {"openid"},
+ "scope": {scope},
}
req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/api/oidc/device/authorize", strings.NewReader(form.Encode()))
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
response, _, err := service.createDeviceAuthorization(t.Context(), req)
require.NoError(t, err)
- return service, store, provider, response.UserCode, response.DeviceCode
+ return response.UserCode, response.DeviceCode
}
func newTestDeviceService(t *testing.T, clientID, userID string, requiresReauthentication bool, reauth ReauthenticationTokenConsumer, apiAccess APIAccessProvider) (*deviceService, *Store, *oidcProvider) {
diff --git a/backend/internal/oidc/interaction_session_dto.go b/backend/internal/oidc/interaction_session_dto.go
index 9680174c..4ea3d856 100644
--- a/backend/internal/oidc/interaction_session_dto.go
+++ b/backend/internal/oidc/interaction_session_dto.go
@@ -32,6 +32,9 @@ type interactionClient struct {
type completeInteractionRequest struct {
Step interactionStep `json:"step"`
+ // Scopes are the scopes the user approved on the consent screen
+ // Omitting them approves every requested scope
+ Scopes []string `json:"scopes"`
}
type completeInteractionResponse struct {
diff --git a/backend/internal/oidc/interaction_session_service.go b/backend/internal/oidc/interaction_session_service.go
index a7d28b85..d7154436 100644
--- a/backend/internal/oidc/interaction_session_service.go
+++ b/backend/internal/oidc/interaction_session_service.go
@@ -59,6 +59,7 @@ func (s *interactionSessionService) update(ctx context.Context, interactionSessi
"reauthentication_required": interactionSession.ReauthenticationRequired,
"consent_required": interactionSession.ConsentRequired,
"user_id": interactionSession.UserID,
+ "scopes": interactionSession.Scopes,
"reauthenticated_at": interactionSession.ReauthenticatedAt,
"parameters": interactionSession.Parameters,
}).
diff --git a/frontend/src/lib/components/scope-item.svelte b/frontend/src/lib/components/scope-item.svelte
index 4ea4a00f..4eba5eae 100644
--- a/frontend/src/lib/components/scope-item.svelte
+++ b/frontend/src/lib/components/scope-item.svelte
@@ -1,19 +1,23 @@
-
+{#snippet content()}
@@ -21,4 +25,30 @@
{name}
{description}
-
+{/snippet}
+
+{#if onSelectedChange}
+
+
+ {#snippet child({ props })}
+
+ {/snippet}
+
+{:else}
+
+ {@render content()}
+
+{/if}
diff --git a/frontend/src/lib/components/scope-list.svelte b/frontend/src/lib/components/scope-list.svelte
index 57096a77..6a63ff69 100644
--- a/frontend/src/lib/components/scope-list.svelte
+++ b/frontend/src/lib/components/scope-list.svelte
@@ -7,23 +7,49 @@
let {
scopes,
- scopeInfo = []
- }: { scopes?: string[] | null; scopeInfo?: InteractionScopeInfo[] | null } = $props();
+ scopeInfo = [],
+ selectedScopes = $bindable()
+ }: {
+ scopes?: string[] | null;
+ scopeInfo?: InteractionScopeInfo[] | null;
+ // When bound, every displayed scope gets a checkbox so the user can deselect it
+ selectedScopes?: string[];
+ } = $props();
const standardScopes = ['openid', 'profile', 'email', 'groups', 'offline_access'];
const infoByKey = $derived(new Map((scopeInfo || []).map((info) => [info.key, info])));
const customScopes = $derived((scopes || []).filter((scope) => !standardScopes.includes(scope)));
+
+ function selectionProps(scope: string) {
+ if (!selectedScopes) {
+ return {};
+ }
+
+ return {
+ selected: selectedScopes.includes(scope),
+ onSelectedChange: (selected: boolean) => {
+ const others = (selectedScopes ?? []).filter((s) => s !== scope);
+ selectedScopes = selected ? [...others, scope] : others;
+ }
+ };
+ }
{#if (scopes || []).includes('email')}
-
+
{/if}
{#if (scopes || []).includes('profile')}
{/if}
{#if (scopes || []).includes('groups')}
@@ -31,6 +57,7 @@
icon={LucideUsers}
name={m.groups()}
description={m.view_the_groups_you_are_a_member_of()}
+ {...selectionProps('groups')}
/>
{/if}
{#each customScopes as scope (scope)}
@@ -38,6 +65,7 @@
icon={LucideKeyRound}
name={infoByKey.get(scope)?.name ?? scope}
description={infoByKey.get(scope)?.description || m.access_an_api_on_your_behalf()}
+ {...selectionProps(scope)}
/>
{/each}
diff --git a/frontend/src/lib/services/oidc-service.ts b/frontend/src/lib/services/oidc-service.ts
index e09d2ce6..018c7d3a 100644
--- a/frontend/src/lib/services/oidc-service.ts
+++ b/frontend/src/lib/services/oidc-service.ts
@@ -27,10 +27,14 @@ class OidcService extends APIService {
return data;
};
- completeAuthorizeInteractionStep = async (id: string, step: InteractionStep) => {
+ completeAuthorizeInteractionStep = async (
+ id: string,
+ step: InteractionStep,
+ scopes?: string[]
+ ) => {
const { data } = await this.api.post(
`/oidc/interactions/${id}/complete`,
- { step }
+ { step, scopes }
);
return data;
};
@@ -116,8 +120,8 @@ class OidcService extends APIService {
return res.data as OidcClientWithAllowedUserGroups;
};
- verifyDeviceCode = async (userCode: string) => {
- return await this.api.post(`/oidc/device/verify?code=${userCode}`);
+ verifyDeviceCode = async (userCode: string, scopes?: string[]) => {
+ return await this.api.post(`/oidc/device/verify?code=${userCode}`, { scopes });
};
getDeviceCodeInfo = async (userCode: string): Promise => {
diff --git a/frontend/src/routes/device/+page.svelte b/frontend/src/routes/device/+page.svelte
index b41a3124..63942789 100644
--- a/frontend/src/routes/device/+page.svelte
+++ b/frontend/src/routes/device/+page.svelte
@@ -37,6 +37,7 @@
let deviceLoginDecision: 'approve' | 'deny' | undefined = $state();
let errorMessage: string | null = $state(null);
let authorizationRequired = $state(false);
+ let selectedScopes: string[] = $state([]);
let reauthenticationRequired = $state(false);
let reauthenticated = $state(false);
let normalizedUserCode = $derived(
@@ -79,6 +80,8 @@
if (info.authorizationRequired && !authorizationRequired) {
authorizationRequired = true;
+ // Every requested scope starts selected so the user only has to deselect the ones they do not want to grant
+ selectedScopes = [...(info.scope ?? [])];
return;
}
@@ -92,7 +95,10 @@
reauthenticated = true;
}
- await oidcService.verifyDeviceCode(normalizedUserCode);
+ await oidcService.verifyDeviceCode(
+ normalizedUserCode,
+ authorizationRequired ? selectedScopes : undefined
+ );
success = true;
} catch (error) {
errorMessage = getWebauthnErrorMessage(error);
@@ -236,7 +242,11 @@
-
+
diff --git a/frontend/src/routes/interaction/+page.svelte b/frontend/src/routes/interaction/+page.svelte
index 020ca818..3b5d7f84 100644
--- a/frontend/src/routes/interaction/+page.svelte
+++ b/frontend/src/routes/interaction/+page.svelte
@@ -30,6 +30,8 @@
let errorMessage: string | null = $state(null);
let errorCode: string | undefined = $state();
let currentStep = $derived(interactionSession.currentStep);
+ // Every requested scope starts selected so the user only has to deselect the ones they do not want to grant
+ let selectedScopes = $state([...(data.interactionSession.scopes ?? [])]);
const fullName = $derived.by(() => {
if (!$userStore) {
@@ -93,7 +95,11 @@
}
async function completeInteraction(step: InteractionStep, skipRedirect = false) {
- const result = await oidcService.completeAuthorizeInteractionStep(interactionSession.id, step);
+ const result = await oidcService.completeAuthorizeInteractionStep(
+ interactionSession.id,
+ step,
+ step === 'consent' ? selectedScopes : undefined
+ );
if (result.interaction) {
interactionSession = result.interaction;
if (interactionSession.currentStep == 'reauthenticate') {
@@ -193,6 +199,7 @@
diff --git a/tests/specs/oidc.spec.ts b/tests/specs/oidc.spec.ts
index 1d1c71e8..59280068 100644
--- a/tests/specs/oidc.spec.ts
+++ b/tests/specs/oidc.spec.ts
@@ -120,6 +120,38 @@ test('Authorize client requesting offline_access scope', async ({ page }) => {
expect(callbackUrl.searchParams.get('error')).toBeNull();
});
+test('Authorize new client with a deselected scope', async ({ page }) => {
+ const oidcClient = oidcClients.immich;
+ const urlParams = createUrlParams(oidcClient);
+ await page.goto(`/authorize?${urlParams.toString()}`);
+
+ await expectScopes(page, ['Email', 'Profile']);
+ await page.getByRole('checkbox', { name: 'Email' }).click();
+ await expect(page.getByRole('checkbox', { name: 'Email' })).not.toBeChecked();
+ await expect(page.getByRole('checkbox', { name: 'Profile' })).toBeChecked();
+
+ const callbackUrl = await expectCallbackRedirect(page, oidcClient.callbackUrl, () =>
+ page.getByRole('button', { name: 'Sign in' }).click()
+ );
+
+ // The deselected scope must be missing from the issued token and therefore from the userinfo claims
+ const token = (await oidcUtil.exchangeCode(page, {
+ grant_type: 'authorization_code',
+ code: callbackUrl.searchParams.get('code')!,
+ redirect_uri: oidcClient.callbackUrl,
+ client_id: oidcClient.id,
+ client_secret: oidcClient.secret
+ })) as { access_token?: string; scope?: string };
+ expect(token.scope?.split(' ').sort()).toEqual(['openid', 'profile']);
+
+ const userinfo = await page.request
+ .get('/api/oidc/userinfo', { headers: { Authorization: `Bearer ${token.access_token}` } })
+ .then((r) => r.json());
+ expect(userinfo.sub).toBe(users.tim.id);
+ expect(userinfo.given_name).toBeTruthy();
+ expect(userinfo.email).toBeUndefined();
+});
+
test('Authorize existing client without scopes', async ({ page }) => {
const oidcClient = oidcClients.nextcloud;
const urlParams = new URLSearchParams({
@@ -678,6 +710,47 @@ test('Authorize new client with device authorization flow', async ({ page }) =>
).toBeVisible();
});
+test('Authorize new client with device authorization flow and a deselected scope', async ({
+ page
+}) => {
+ const client = oidcClients.immich;
+ const credentials = { client_id: client.id, client_secret: client.secret };
+ const device = await page.request
+ .post('/api/oidc/device/authorize', {
+ form: { ...credentials, scope: 'openid profile email' }
+ })
+ .then((r) => r.json());
+
+ await page.goto(`/device?user_code=${device.user_code}`);
+ await page.getByRole('button', { name: 'Authorize' }).click();
+
+ await expectScopes(page, ['Email', 'Profile']);
+ await page.getByRole('checkbox', { name: 'Email' }).click();
+ await expect(page.getByRole('checkbox', { name: 'Email' })).not.toBeChecked();
+ await expect(page.getByRole('checkbox', { name: 'Profile' })).toBeChecked();
+
+ await page.getByRole('button', { name: 'Authorize' }).click();
+ await expect(
+ page.getByRole('paragraph').filter({ hasText: 'The device has been authorized.' })
+ ).toBeVisible();
+
+ // The deselected scope must be missing from the token the device receives and therefore from the userinfo claims
+ await page.waitForTimeout(device.interval * 1000 + 100);
+ const token = (await oidcUtil.exchangeCode(page, {
+ ...credentials,
+ grant_type: 'urn:ietf:params:oauth:grant-type:device_code',
+ device_code: device.device_code
+ })) as { access_token?: string; scope?: string };
+ expect(token.scope?.split(' ').sort()).toEqual(['openid', 'profile']);
+
+ const userinfo = await page.request
+ .get('/api/oidc/userinfo', { headers: { Authorization: `Bearer ${token.access_token}` } })
+ .then((r) => r.json());
+ expect(userinfo.sub).toBe(users.tim.id);
+ expect(userinfo.given_name).toBeTruthy();
+ expect(userinfo.email).toBeUndefined();
+});
+
test('Authorize new client with device authorization flow while not signed in', async ({
page
}) => {