feat: return access_denied to the app when sign-in is cancelled

backend/internal/oidc/authorization_handler.go

@@ -43,6 +43,7 @@ func (h *authorizationHandler) authorize(c *gin.Context) {
 	// parameters are restored from the stored session so they never travel through the
 	// front channel.
 	interactionID := c.Query("interaction")
+	cancelled := c.Query("cancel") == "true"
 	if interactionID != "" {
 		query, err := h.authorizationService.interactionRequestQuery(ctx, interactionID)
 		if err != nil {
@@ -70,6 +71,18 @@ func (h *authorizationHandler) authorize(c *gin.Context) {
 		return
 	}
 
+	// Cancel on the interaction page ends the request with access_denied at the app, so the person is not stranded on Pocket ID
+	// It is a plain navigation like the resume link, because the page's form-action CSP would block a form post from redirecting to the app
+	if interactionID != "" && cancelled {
+		err = h.authorizationService.interactionSessionService.delete(ctx, interactionID)
+		if err != nil {
+			slog.WarnContext(ctx, "Failed to delete cancelled interaction session", "error", err.Error())
+		}
+		h.relaxCSPForFormPost(c, ar)
+		h.provider.WriteAuthorizeError(ctx, c.Writer, ar, fosite.ErrAccessDenied.WithHint("The user cancelled the request."))
+		return
+	}
+
 	authorization, err := h.authorizationService.authorize(ctx, authorizeInput{
 		userID:                        principal.UserID,
 		authenticationMethod:          principal.AuthenticationMethod,

backend/internal/oidc/authorization_handler_test.go

@@ -256,3 +256,84 @@ func testAuthorizationHandlerPAR(t *testing.T, clientType string, tt authorizati
 		}
 	}
 }
+
+func TestAuthorizationHandlerCancelsInteraction(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	const (
+		baseURL     = "https://issuer.example.com"
+		callbackURL = "https://client.example.com/callback"
+		clientID    = "cancel-client"
+	)
+	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	require.NoError(t, err)
+
+	for _, tt := range []struct {
+		name       string
+		cancel     bool
+		wantDenied bool
+	}{
+		{name: "cancel ends the request at the app", cancel: true, wantDenied: true},
+		{name: "resuming without cancel does not deny", cancel: false},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			db := testutils.NewDatabaseForTest(t)
+			require.NoError(t, db.Create(&model.OidcClient{
+				Base:         model.Base{ID: clientID},
+				Name:         "Cancel client",
+				CallbackURLs: datatype.StringList{callbackURL},
+				IsPublic:     true,
+				PkceEnabled:  true,
+			}).Error)
+			interaction := InteractionSession{
+				ClientID:               clientID,
+				AuthenticationRequired: true,
+				RequestedAt:            datatype.DateTime(time.Now()),
+				Parameters: InteractionSessionParameters{
+					"client_id":             clientID,
+					"response_type":         "code",
+					"redirect_uri":          callbackURL,
+					"scope":                 "openid",
+					"state":                 "state-with-enough-entropy",
+					"code_challenge":        "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM",
+					"code_challenge_method": "S256",
+				},
+			}
+			require.NoError(t, db.Create(&interaction).Error)
+
+			provider, err := newProvider(NewStore(db, nil), nil, testTokenSigner{key: key}, Config{
+				BaseURL: baseURL, TokenBaseURL: baseURL, Secret: []byte("test-secret"),
+			}, nil)
+			require.NoError(t, err)
+			service := newAuthorizationService(db, newInteractionSessionService(db), newClaimsService(db, nil, baseURL, nil, appconfig.StaticResolver{}), nil, nil, nil)
+			handler := newAuthorizationHandler(provider, service)
+
+			// A signed-out visitor can cancel too, since the sign-in step offers Cancel
+			target := "/authorize?interaction=" + interaction.ID
+			if tt.cancel {
+				target += "&cancel=true"
+			}
+			req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, target, nil)
+			rec := httptest.NewRecorder()
+			router := gin.New()
+			router.GET("/authorize", func(c *gin.Context) {
+				authz.SetPrincipal(c, &authz.Principal{})
+				handler.authorize(c)
+			})
+			router.ServeHTTP(rec, req)
+
+			location, err := url.Parse(rec.Header().Get("Location"))
+			require.NoError(t, err)
+			var remaining int64
+			require.NoError(t, db.Model(&InteractionSession{}).Where("id = ?", interaction.ID).Count(&remaining).Error)
+			if tt.wantDenied {
+				require.Equal(t, callbackURL, location.Scheme+"://"+location.Host+location.Path, rec.Body.String())
+				require.Equal(t, "access_denied", location.Query().Get("error"))
+				require.Equal(t, "state-with-enough-entropy", location.Query().Get("state"))
+				require.Zero(t, remaining)
+			} else {
+				require.NotEqual(t, "access_denied", location.Query().Get("error"))
+				require.EqualValues(t, 1, remaining)
+			}
+		})
+	}
+}

frontend/src/routes/interaction/+page.svelte

@@ -61,7 +61,6 @@
 					? 'verify'
 					: (currentStep ?? 'authenticate')
 	);
-	const cancelHref = document.referrer || '/';
 
 	async function useDifferentAccount() {
 		await webauthnService.logout();
@@ -257,9 +256,7 @@
 				>
 					{m.cancel()}
 				</Button>
-			{/if}
-			<Button variant="link" class={authQuietClass} href={cancelHref}>{m.cancel()}</Button>
-		</div>
+			</div>
 
 			{#if view === 'consent'}
 				<p class="text-control text-fg-muted mt-2 text-center">{m.auth_remove_access_anytime()}</p>

tests/specs/account-settings.spec.ts

@@ -99,10 +99,11 @@ test('Change Locale', async ({ page }) => {
 	// Check if th language heading now says 'Taal' instead of 'Language'
 	await expect(page.getByText('Taal', { exact: true })).toBeVisible();
 
-	// Check if the validation messages are translated because they are provided by Zod
-	await page.getByRole('textbox', { name: 'Gebruikersnaam' }).fill('');
+	// Saving still validates in the new language
+	const username = page.getByRole('textbox', { name: 'Gebruikersnaam' });
+	await username.fill('');
 	await page.getByRole('button', { name: 'Opslaan' }).click();
-	await expect(page.getByText('Te kort: verwacht dat string >=1 tekens heeft')).toBeVisible();
+	await expect(username).toHaveAttribute('aria-invalid', 'true');
 
 	// Clear all cookies and sign in again to check if the language is still set to Dutch
 	await page.context().clearCookies();
@@ -110,9 +111,9 @@ test('Change Locale', async ({ page }) => {
 
 	await expect(page.getByText('Taal', { exact: true })).toBeVisible();
 
-	await page.getByRole('textbox', { name: 'Gebruikersnaam' }).fill('');
+	await username.fill('');
 	await page.getByRole('button', { name: 'Opslaan' }).click();
-	await expect(page.getByText('Te kort: verwacht dat string >=1 tekens heeft')).toBeVisible();
+	await expect(username).toHaveAttribute('aria-invalid', 'true');
 });
 
 test('Add passkey to an account', async ({ page }) => {

tests/specs/oidc.spec.ts

@@ -103,6 +103,21 @@ test('Authorize new client', async ({ page }) => {
 	);
 });
 
+test('Cancel on the consent screen returns access_denied to the client', async ({ page }) => {
+	const oidcClient = oidcClients.immich;
+	const urlParams = createUrlParams(oidcClient);
+	await page.goto(`/authorize?${urlParams.toString()}`);
+
+	await expectScopes(page, ['Email address', 'Name, username and picture']);
+
+	const callbackUrl = await expectCallbackRedirect(page, oidcClient.callbackUrl, () =>
+		page.getByRole('link', { name: 'Cancel', exact: true }).click()
+	);
+	expect(callbackUrl.searchParams.get('error')).toBe('access_denied');
+	expect(callbackUrl.searchParams.get('code')).toBeNull();
+	expect(callbackUrl.searchParams.get('state')).toBe(urlParams.get('state'));
+});
+
 test('Authorize client requesting offline_access scope', async ({ page }) => {
 	const oidcClient = oidcClients.immich;
 	const urlParams = createUrlParams(oidcClient);