diff --git a/CHANGELOG.md b/CHANGELOG.md index c1d0e6b1..55b6ff33 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,8 @@ written while it was being built. See [RELEASING.md](RELEASING.md). ### Fixed +- A server problem while joining a hackathon no longer claims that your sign-in + has expired. A rejected login and a real fault are told apart again. - Invitation links no longer fail with "This invitation is no longer valid" for people who have never used Hackagon before. The link was always fine — their account had simply never been created. diff --git a/components/backend/go.mod b/components/backend/go.mod index 7ae1c426..90974ab6 100644 --- a/components/backend/go.mod +++ b/components/backend/go.mod @@ -19,7 +19,6 @@ require ( github.com/MicahParks/keyfunc/v3 v3.8.0 github.com/casbin/casbin/v3 v3.8.1 github.com/casbin/ent-adapter v1.4.0 - github.com/golang-jwt/jwt/v4 v4.4.2 github.com/golang-jwt/jwt/v5 v5.3.1 github.com/grpc-ecosystem/go-grpc-middleware v1.4.0 github.com/grpc-ecosystem/go-grpc-middleware/v2 v2.3.2 diff --git a/components/backend/go.sum b/components/backend/go.sum index 7c3dd192..f72c7d62 100644 --- a/components/backend/go.sum +++ b/components/backend/go.sum @@ -80,8 +80,6 @@ github.com/go-viper/mapstructure/v2 v2.4.0/go.mod h1:oJDH3BJKyqBA2TXFhDsKDGDTlnd github.com/goccy/go-yaml v1.18.0 h1:8W7wMFS12Pcas7KU+VVkaiCng+kG8QiFeFwzFb+rwuw= github.com/goccy/go-yaml v1.18.0/go.mod h1:XBurs7gK8ATbW4ZPGKgcbrY1Br56PdM69F7LkFRi1kA= github.com/gogo/protobuf v1.3.2/go.mod h1:P1XiOD3dCwIKUDQYPy72D8LYyHL2YPYrpS2s69NZV8Q= -github.com/golang-jwt/jwt/v4 v4.4.2 h1:rcc4lwaZgFMCZ5jxF9ABolDcIHdBytAFgqFPbSJQAYs= -github.com/golang-jwt/jwt/v4 v4.4.2/go.mod h1:m21LjoU+eqJr34lmDMbreY2eSTRJ1cv77w39/MY0Ch0= github.com/golang-jwt/jwt/v5 v5.3.1 h1:kYf81DTWFe7t+1VvL7eS+jKFVWaUnK9cB1qbwn63YCY= github.com/golang-jwt/jwt/v5 v5.3.1/go.mod h1:fxCRLWMO43lRc8nhHWY6LGqRcf+1gQWArsqaEUEa5bE= github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfUlaOkMMuAQtPIUF8ecWP5IEl/CR7VP2Q= diff --git a/components/backend/internal/middleware/auth_test.go b/components/backend/internal/middleware/auth_test.go index f403f820..e9c741b5 100644 --- a/components/backend/internal/middleware/auth_test.go +++ b/components/backend/internal/middleware/auth_test.go @@ -10,7 +10,9 @@ import ( "github.com/golang-jwt/jwt/v5" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" + "google.golang.org/grpc/status" "github.com/swissdatasciencecenter/hackagon/components/backend/internal/config" "github.com/swissdatasciencecenter/hackagon/components/backend/internal/middleware" @@ -84,6 +86,7 @@ var _ = Describe("Auth Middleware", func() { _, err := validator.AuthFunc()(ctx) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("token is malformed")) + Expect(status.Code(err)).To(Equal(codes.Unauthenticated)) }) It("rejects expired tokens", func() { @@ -96,6 +99,7 @@ var _ = Describe("Auth Middleware", func() { _, err := validator.AuthFunc()(ctx) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("token is expired")) + Expect(status.Code(err)).To(Equal(codes.Unauthenticated)) }) It("rejects tokens with future not-before", func() { @@ -119,6 +123,21 @@ var _ = Describe("Auth Middleware", func() { _, err = validator.AuthFunc()(ctx) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("token is not valid yet")) + Expect(status.Code(err)).To(Equal(codes.Unauthenticated)) + }) + + It("rejects a token from the wrong issuer", func() { + tokenString := testutils.GenerateTestToken( + "wrong-issuer-user", 24*time.Hour, "https://not-our-issuer.example", + ) + ctx := metadata.NewIncomingContext(context.Background(), metadata.Pairs( + "authorization", "Bearer "+tokenString, + )) + + validator := middleware.NewTestJWTValidator(cfg, keyfunc) + _, err := validator.AuthFunc()(ctx) + Expect(err).To(HaveOccurred()) + Expect(status.Code(err)).To(Equal(codes.Unauthenticated)) }) }) @@ -158,6 +177,7 @@ var _ = Describe("Auth Middleware", func() { _, err := validator.AuthFunc()(ctx) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("token is malformed")) + Expect(status.Code(err)).To(Equal(codes.Unauthenticated)) }) }) diff --git a/components/backend/internal/middleware/errors.go b/components/backend/internal/middleware/errors.go index 5f16e05a..c4bfe740 100644 --- a/components/backend/internal/middleware/errors.go +++ b/components/backend/internal/middleware/errors.go @@ -4,7 +4,7 @@ import ( "errors" "strings" - "github.com/golang-jwt/jwt/v4" + "github.com/golang-jwt/jwt/v5" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -35,25 +35,40 @@ func handleError(err error, setErrorCodes bool) error { return err } +// handleJwtError translates a failure from the jwt library into one of this +// package's own errors, which `setGrpcErrorCodes` then turns into +// `Unauthenticated`. +// +// Matched with `errors.Is` against the library's sentinel values. v5 reports a +// bad token by wrapping those sentinels; it has no error *type* to match on — +// the `*jwt.ValidationError` of v4 was removed. This file went on matching the +// v4 type while `auth.go` parsed with v5, so nothing here ever matched and +// every expired, malformed or badly signed token fell through to `Internal`, +// reported to callers as a server fault rather than a rejected login. +// +// Anything unrecognised is returned untouched, so an error raised by `auth.go` +// itself (a missing header, an unknown key id) reaches `setGrpcErrorCodes` as +// the sentinel it already is. func handleJwtError(errIn error) error { - var err *jwt.ValidationError - if errors.As(errIn, &err) { - switch { - case err.Is(jwt.ErrTokenExpired): - return ErrTokenExpired - case err.Is(jwt.ErrTokenMalformed): - return ErrTokenMalformed - case err.Is(jwt.ErrTokenNotValidYet): - return ErrTokenNotValidYet - case err.Is(jwt.ErrTokenUsedBeforeIssued): - return ErrTokenUsedBeforeIssued - case err.Is(jwt.ErrTokenSignatureInvalid): - if strings.Contains(err.Error(), "signing method") { - return ErrBadAlgorithm - } - - return ErrSignatureInvalid + switch { + case errors.Is(errIn, jwt.ErrTokenExpired): + return ErrTokenExpired + case errors.Is(errIn, jwt.ErrTokenMalformed): + return ErrTokenMalformed + case errors.Is(errIn, jwt.ErrTokenNotValidYet): + return ErrTokenNotValidYet + case errors.Is(errIn, jwt.ErrTokenUsedBeforeIssued): + return ErrTokenUsedBeforeIssued + case errors.Is(errIn, jwt.ErrTokenInvalidIssuer): + return ErrTokenInvalidIssuer + case errors.Is(errIn, jwt.ErrTokenSignatureInvalid): + // `WithValidMethods` rejects a wrong algorithm through the same + // sentinel, and only the message tells the two apart. + if strings.Contains(errIn.Error(), "signing method") { + return ErrBadAlgorithm } + + return ErrSignatureInvalid } return errIn diff --git a/components/frontend/src/routes/(public)/invite/[token]/+page.server.ts b/components/frontend/src/routes/(public)/invite/[token]/+page.server.ts index 6d3bf2f0..e7ab79b3 100644 --- a/components/frontend/src/routes/(public)/invite/[token]/+page.server.ts +++ b/components/frontend/src/routes/(public)/invite/[token]/+page.server.ts @@ -87,17 +87,13 @@ function authorizedFor(session: CustomSession | null) { * refusal falls back to the anonymous call: everything but that one field is * identical, and the page's whole point is being readable before signing in. * `usableSession` already screens out the refusal Auth.js reports, but a token - * can also lapse between refreshes, and the backend answers that with INTERNAL - * rather than UNAUTHENTICATED — see `TODO(backend: jwt-error-codes)` below. + * can also lapse between refreshes, and that arrives here as UNAUTHENTICATED. */ function askPreview(token: string, grpc?: AuthorizedGrpc) { if (!grpc) return publicHackathonClient().previewInvite({ token }) return grpc.hackathon.previewInvite({ token }).catch((e) => { - if ( - e instanceof ClientError && - (e.code === Status.UNAUTHENTICATED || e.code === Status.INTERNAL) - ) { + if (e instanceof ClientError && e.code === Status.UNAUTHENTICATED) { return publicHackathonClient().previewInvite({ token }) } throw e @@ -263,22 +259,22 @@ export const actions: Actions = { return fail(400, { message: e.details || "Some answers are not valid.", }) - // The backend refusing the token itself. UNAUTHENTICATED is what the - // middleware means to send. - // - // TODO(backend: jwt-error-codes): INTERNAL is in this branch because it - // is what actually arrives. `errors.go` matches jwt/**v4**'s - // `*ValidationError` while `auth.go` parses with **v5**, which removed - // that type — so `errors.As` never matches and every auth failure falls - // past the `unauthenticatedErrors` list to `codes.Internal`. Drop - // INTERNAL from here once that is fixed; until then a genuine server - // fault during a join is reported to the user as an expired session, - // which is the lesser of the two wrong answers available. - if (e.code === Status.UNAUTHENTICATED || e.code === Status.INTERNAL) + // The backend refusing the token itself. + if (e.code === Status.UNAUTHENTICATED) return fail(401, { message: "Your sign-in has expired. Sign in again — this invitation still works.", }) + // A real server fault, and now distinguishable from one: INTERNAL used + // to arrive for every rejected token too, so this page had to read it + // as an expired session. It no longer does, so a fault can say so and + // the invitation can be described as still good — which it is. + if (e.code === Status.INTERNAL) + return fail(500, { + message: + "Something went wrong on our side. Your invitation is still " + + "valid — please try again.", + }) } throw e }