coderabbit review stuff

backend/internal/common/errors.go

@@ -87,6 +87,14 @@ type NotSignedInError struct{}
 
 func (e NotSignedInError) Error() string       { return "You are not signed in" }
 func (e NotSignedInError) HttpStatusCode() int { return http.StatusUnauthorized }
+func (e NotSignedInError) Is(target error) bool {
+	switch target.(type) {
+	case NotSignedInError, *NotSignedInError:
+		return true
+	default:
+		return false
+	}
+}
 
 type MissingPermissionError struct{}
 

backend/internal/dto/scim_dto.go

@@ -1,8 +1,10 @@
 package dto
 
 import (
+	"net/url"
 	"time"
 
+	"github.com/danielgtaylor/huma/v2"
 	datatype "github.com/pocket-id/pocket-id/backend/internal/model/types"
 )
 
@@ -21,6 +23,14 @@ type ScimServiceProviderCreateDTO struct {
 	OidcClientID string `json:"oidcClientId" required:"true"`
 }
 
+func (d *ScimServiceProviderCreateDTO) Resolve(huma.Context) []error {
+	endpoint, err := url.Parse(d.Endpoint)
+	if err != nil || !endpoint.IsAbs() {
+		return []error{&huma.ErrorDetail{Location: "body.endpoint", Message: "Endpoint must be an absolute URI"}}
+	}
+	return nil
+}
+
 type ScimUser struct {
 	ScimResourceData
 	UserName string      `json:"userName"`

backend/internal/dto/user_dto.go

@@ -4,6 +4,8 @@ import (
 	"errors"
 	"net/mail"
 	"unicode/utf8"
+
+	"github.com/danielgtaylor/huma/v2"
 )
 
 type UserDto struct {
@@ -36,6 +38,17 @@ type UserCreateDto struct {
 	LdapID        string   `json:"-"`
 }
 
+func (u UserCreateDto) Resolve(huma.Context) []error {
+	if u.Email == nil {
+		return nil
+	}
+	address, err := mail.ParseAddress(*u.Email)
+	if err != nil || address.Address != *u.Email {
+		return []error{&huma.ErrorDetail{Location: "body.email", Message: "Field validation for 'Email' failed on the 'email' tag"}}
+	}
+	return nil
+}
+
 //nolint:staticcheck // LDAP callers and their tests rely on the existing capitalized validation text
 func (u UserCreateDto) Validate() error {
 	if u.Username == "" {
@@ -47,12 +60,6 @@ func (u UserCreateDto) Validate() error {
 	if utf8.RuneCountInString(u.Username) > 50 {
 		return errors.New("Field validation for 'Username' failed on the 'max' tag")
 	}
-	if u.Email != nil {
-		address, err := mail.ParseAddress(*u.Email)
-		if err != nil || address.Address != *u.Email {
-			return errors.New("Field validation for 'Email' failed on the 'email' tag")
-		}
-	}
 	if utf8.RuneCountInString(u.FirstName) > 50 {
 		return errors.New("Field validation for 'FirstName' failed on the 'max' tag")
 	}

backend/internal/dto/user_dto_test.go

@@ -63,17 +63,6 @@ func TestUserCreateDto_Validate(t *testing.T) {
 			},
 			wantErr: "Field validation for 'Username' failed on the 'username' tag",
 		},
-		{
-			name: "invalid email",
-			input: UserCreateDto{
-				Username:    "testuser",
-				Email:       new("not-an-email"),
-				FirstName:   "John",
-				LastName:    "Doe",
-				DisplayName: "John Doe",
-			},
-			wantErr: "Field validation for 'Email' failed on the 'email' tag",
-		},
 		{
 			name: "first name too short",
 			input: UserCreateDto{
@@ -112,3 +101,28 @@ func TestUserCreateDto_Validate(t *testing.T) {
 		})
 	}
 }
+
+func TestUserCreateDtoResolveEmail(t *testing.T) {
+	testCases := []struct {
+		name    string
+		email   *string
+		wantErr bool
+	}{
+		{name: "empty email", email: nil},
+		{name: "exact address", email: new("test@example.com")},
+		{name: "invalid address", email: new("not-an-email"), wantErr: true},
+		{name: "display name is not an exact address", email: new("Test User <test@example.com>"), wantErr: true},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			errs := (&UserCreateDto{Email: testCase.email}).Resolve(nil)
+			if testCase.wantErr {
+				require.Len(t, errs, 1)
+				require.ErrorContains(t, errs[0], "Field validation for 'Email' failed on the 'email' tag")
+				return
+			}
+			require.Empty(t, errs)
+		})
+	}
+}

backend/internal/middleware/auth_middleware.go

@@ -131,14 +131,18 @@ type authenticationResult struct {
 }
 
 func (m *AuthMiddleware) authenticate(c *gin.Context) (authenticationResult, error) {
+	// Return immediately after successful JWT authentication
 	userID, isAdmin, authenticationMethod, authenticationTime, err := m.jwtMiddleware.Verify(c, m.options.AdminRequired)
 	if err == nil {
 		return authenticationResult{userID, isAdmin, authenticationMethod, authenticationTime}, nil
 	}
+
+	// Fall back only when JWT verification reports that the request is not signed in
 	if !errors.Is(err, &common.NotSignedInError{}) {
 		return authenticationResult{}, err
 	}
 
+	// Handle API-key-disabled routes before considering API-key authentication
 	if !m.options.AllowApiKeyAuth {
 		if m.options.SuccessOptional {
 			return authenticationResult{}, nil
@@ -149,6 +153,7 @@ func (m *AuthMiddleware) authenticate(c *gin.Context) (authenticationResult, err
 		return authenticationResult{}, err
 	}
 
+	// Attempt API-key authentication after JWT reports that the request is not signed in
 	userID, isAdmin, err = m.apiKeyMiddleware.Verify(c, m.options.AdminRequired)
 	if err == nil {
 		return authenticationResult{UserID: userID, IsAdmin: isAdmin}, nil

backend/internal/oidc/end_session_handler.go

@@ -35,7 +35,7 @@ func (h *endSessionHandler) endSession(c *gin.Context) {
 		return
 	}
 
-	http.SetCookie(c.Writer, cookie.NewAccessTokenCookie(0, ""))
+	http.SetCookie(c.Writer, cookie.NewAccessTokenCookie(-1, ""))
 	if callbackURL == "" {
 		c.Redirect(http.StatusFound, h.baseURL+"/logout")
 		return

backend/internal/webauthn/handler.go

@@ -143,20 +143,20 @@ func (h *handler) updateCredential(ctx context.Context, input *credentialUpdateI
 }
 
 func (h *handler) logout(_ context.Context, _ *httpapi.EmptyInput) (*emptyOutput, error) {
-	return &emptyOutput{SetCookie: []http.Cookie{*cookie.NewAccessTokenCookie(0, "")}}, nil
+	return &emptyOutput{SetCookie: []http.Cookie{*cookie.NewAccessTokenCookie(-1, "")}}, nil
 }
 
 func (h *handler) reauthenticate(ctx context.Context, input *optionalCredentialBodyInput) (*emptyOutput, error) {
-	sessionID, err := sessionID(ctx)
-	if err != nil {
-		return nil, err
-	}
-
 	var token string
+	var err error
 	if input.Body != nil {
 		assertion, parseErr := protocol.ParseCredentialRequestResponseBody(bytes.NewReader(*input.Body))
 		if parseErr == nil {
-			token, err = h.service.CreateReauthenticationTokenWithWebauthn(ctx, sessionID, assertion)
+			sessionCookieID, sessionErr := sessionID(ctx)
+			if sessionErr != nil {
+				return nil, sessionErr
+			}
+			token, err = h.service.CreateReauthenticationTokenWithWebauthn(ctx, sessionCookieID, assertion)
 		} else {
 			token, err = h.reauthenticateWithAccessToken(ctx)
 		}

backend/internal/webauthn/handler_test.go

@@ -13,7 +13,10 @@ import (
 	"github.com/gin-gonic/gin"
 	"github.com/stretchr/testify/require"
 
+	"github.com/pocket-id/pocket-id/backend/internal/model"
+	"github.com/pocket-id/pocket-id/backend/internal/utils/cookie"
 	httpapi "github.com/pocket-id/pocket-id/backend/internal/utils/huma"
+	testutils "github.com/pocket-id/pocket-id/backend/internal/utils/testing"
 )
 
 func TestRequestWithBodyReconstructsUnderlyingRequest(t *testing.T) {
@@ -51,3 +54,56 @@ func TestRequestWithBodyReconstructsUnderlyingRequest(t *testing.T) {
 	router.ServeHTTP(response, request)
 	require.Equal(t, http.StatusOK, response.Code)
 }
+
+func TestLogoutClearsAccessTokenCookie(t *testing.T) {
+	output, err := (&handler{}).logout(t.Context(), &httpapi.EmptyInput{})
+	require.NoError(t, err)
+	require.Len(t, output.SetCookie, 1)
+	require.Equal(t, -1, output.SetCookie[0].MaxAge)
+	require.Contains(t, output.SetCookie[0].String(), "Max-Age=0")
+}
+
+func TestReauthenticateFallsBackWithoutSessionCookie(t *testing.T) {
+	db := testutils.NewDatabaseForTest(t)
+	user := model.User{Base: model.Base{ID: "handler-reauth-user"}, Username: "handler-reauth-user"}
+	require.NoError(t, db.Create(&user).Error)
+
+	signer := newFakeSigner()
+	accessToken, err := signer.GenerateAccessToken(user, authenticationMethodPhishingResistant)
+	require.NoError(t, err)
+
+	gin.SetMode(gin.TestMode)
+	router := gin.New()
+	api := httpapi.New(router, router.Group("/"))
+	h := &handler{service: &Service{db: db, signer: signer}}
+	httpapi.Register(api, huma.Operation{
+		OperationID:   "test-reauthenticate-fallback",
+		Method:        http.MethodPost,
+		Path:          "/api/test-reauthenticate-fallback",
+		DefaultStatus: http.StatusNoContent,
+	}, h.reauthenticate)
+
+	testCases := []struct {
+		name string
+		body io.Reader
+	}{
+		{name: "empty body"},
+		{name: "invalid assertion", body: strings.NewReader(`{"invalid":true}`)},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			request := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/api/test-reauthenticate-fallback", testCase.body)
+			if testCase.body != nil {
+				request.Header.Set("Content-Type", "application/json")
+			}
+			request.AddCookie(cookie.NewAccessTokenCookie(60, accessToken))
+			response := httptest.NewRecorder()
+
+			router.ServeHTTP(response, request)
+
+			require.Equal(t, http.StatusNoContent, response.Code)
+			require.Contains(t, response.Header().Get("Set-Cookie"), cookie.ReauthenticationTokenCookieName+"=")
+		})
+	}
+}