fix(auth): resolve roles from users.yml instead of the JWT claim (#4983)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

internal/auth/simple.go

@@ -1,20 +1,26 @@
 package auth
 
 import (
+	"context"
 	"crypto/sha256"
 	"errors"
 	"maps"
 	"net/http"
 	"slices"
+	"sync"
 	"time"
 
 	"github.com/go-chi/jwtauth/v5"
+	"github.com/rs/zerolog/log"
 )
 
 type simpleAuthContext struct {
 	UserDatabase UserDatabase
 	tokenAuth    *jwtauth.JWTAuth
 	ttl          time.Duration
+	// UserDatabase.Find reloads users.yml in place, and the middleware now calls it
+	// on every request, so the reload has to be serialized.
+	mu sync.Mutex
 }
 
 var ErrInvalidCredentials = errors.New("invalid credentials")
@@ -40,13 +46,30 @@ func NewSimpleAuth(userDatabase UserDatabase, ttl time.Duration) *simpleAuthCont
 	}
 }
 
-func (a *simpleAuthContext) CreateToken(username, password string) (string, error) {
-	user := a.UserDatabase.FindByPassword(username, password)
+// find returns the user by value. UserDatabase.Find hands back a pointer into
+// UserDatabase.Users and reloads users.yml as it goes, so dereferencing it under
+// the lock keeps a reload from racing whoever is reading the user.
+func (a *simpleAuthContext) find(username string) (User, bool) {
+	a.mu.Lock()
+	defer a.mu.Unlock()
+
+	user := a.UserDatabase.Find(username)
 	if user == nil {
+		return User{}, false
+	}
+
+	return *user, true
+}
+
+func (a *simpleAuthContext) CreateToken(username, password string) (string, error) {
+	user, ok := a.find(username)
+	if !ok || !CompareHashAndPassword(user.Password, password) {
 		return "", ErrInvalidCredentials
 	}
 
-	claims := map[string]any{"username": user.Username, "email": user.Email, "name": user.Name, "filter": user.Filter, "roles": user.Roles}
+	// Identity only. Everything else about the user is read from users.yml per
+	// request, so anything baked in here would just be a copy that goes stale.
+	claims := map[string]any{"username": user.Username}
 	jwtauth.SetIssuedNow(claims)
 
 	if a.ttl > 0 {
@@ -62,5 +85,42 @@ func (a *simpleAuthContext) CreateToken(username, password string) (string, erro
 }
 
 func (a *simpleAuthContext) AuthMiddleware(next http.Handler) http.Handler {
-	return jwtauth.Verifier(a.tokenAuth)(next)
+	return jwtauth.Verifier(a.tokenAuth)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		// The roles claim is a bitmask frozen at login, so it goes stale the moment
+		// the set of roles grows: a session minted before the cloud role existed
+		// carries a mask without that bit and quietly loses the feature until the
+		// user happens to log out. The same is true of a roles or filter edit in
+		// users.yml. Resolve both from the database per request and let the token
+		// prove only who the user is.
+		if user := a.userFromToken(r.Context()); user != nil {
+			r = r.WithContext(WithUser(r.Context(), *user))
+		}
+
+		next.ServeHTTP(w, r)
+	}))
+}
+
+// userFromToken resolves the verified token's subject against users.yml. It returns
+// nil for a missing or invalid token, and for a user who is no longer configured, so
+// the request falls through to RequireAuthentication as unauthenticated.
+func (a *simpleAuthContext) userFromToken(ctx context.Context) *User {
+	_, claims, err := jwtauth.FromContext(ctx)
+	if err != nil {
+		return nil
+	}
+
+	username, ok := claims["username"].(string)
+	if !ok || username == "" {
+		return nil
+	}
+
+	user, ok := a.find(username)
+	if !ok {
+		log.Debug().Str("username", username).Msg("Token is valid but user is no longer in the user database")
+		return nil
+	}
+
+	user.Password = ""
+
+	return &user
 }

internal/auth/simple_test.go

@@ -1,6 +1,10 @@
 package auth
 
 import (
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -48,3 +52,151 @@ func TestSimpleAuthSigningKeyChangesWhenCredentialsChange(t *testing.T) {
 	_, err = NewSimpleAuth(users, 0).tokenAuth.Decode(token)
 	require.Error(t, err)
 }
+
+// serveWithAuth runs a request carrying token through the simple auth middleware
+// and returns the user the handlers would see.
+func serveWithAuth(t *testing.T, a *simpleAuthContext, token string) *User {
+	t.Helper()
+
+	var user *User
+	handler := a.AuthMiddleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		user = UserFromContext(r.Context())
+	}))
+
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	req.Header.Set("Authorization", "Bearer "+token)
+	handler.ServeHTTP(httptest.NewRecorder(), req)
+
+	return user
+}
+
+// The roles claim is a bitmask frozen at login. Adding a role to the code (cloud
+// was the case that broke) leaves every existing session on the old mask without
+// the new bit, and users.yml is unchanged so the signing key does not roll either.
+// Roles have to come from the database on each request, not from the token.
+func TestSimpleAuthUsesCurrentRolesNotTheTokensRoles(t *testing.T) {
+	users := UserDatabase{
+		Users: map[string]*User{
+			"alice": {Username: "alice", Name: "Alice", Password: "$2a$11$aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", RolesConfigured: "all", Roles: All},
+		},
+	}
+
+	a := NewSimpleAuth(users, 0)
+
+	// A session minted back when All did not include Cloud.
+	stale := All &^ Cloud
+	_, token, err := a.tokenAuth.Encode(map[string]any{"username": "alice", "roles": float64(stale)})
+	require.NoError(t, err)
+
+	user := serveWithAuth(t, a, token)
+	require.NotNil(t, user)
+	require.Equal(t, All, user.Roles)
+	require.True(t, user.Roles.Has(Cloud), "cloud must come back without forcing a re-login")
+}
+
+// A narrowed role in users.yml must apply to live sessions too, not only after
+// the user logs out.
+func TestSimpleAuthAppliesRevokedRolesToExistingSessions(t *testing.T) {
+	users := UserDatabase{
+		Users: map[string]*User{
+			"alice": {Username: "alice", Password: "$2a$11$aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", RolesConfigured: "all,^shell", Roles: ParseRole("all,^shell")},
+		},
+	}
+
+	a := NewSimpleAuth(users, 0)
+	_, token, err := a.tokenAuth.Encode(map[string]any{"username": "alice", "roles": float64(All)})
+	require.NoError(t, err)
+
+	user := serveWithAuth(t, a, token)
+	require.NotNil(t, user)
+	require.False(t, user.Roles.Has(Shell))
+	require.True(t, user.Roles.Has(Actions))
+}
+
+// A token for a user who is gone from users.yml resolves to no user, so the
+// request is unauthenticated rather than running with the token's claims.
+func TestSimpleAuthRejectsTokenForUnknownUser(t *testing.T) {
+	users := UserDatabase{
+		Users: map[string]*User{
+			"alice": {Username: "alice", Password: "$2a$11$aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", RolesConfigured: "all", Roles: All},
+		},
+	}
+
+	a := NewSimpleAuth(users, 0)
+	_, token, err := a.tokenAuth.Encode(map[string]any{"username": "mallory", "roles": float64(All)})
+	require.NoError(t, err)
+
+	require.Nil(t, serveWithAuth(t, a, token))
+}
+
+// A session minted before roles existed at all carries no roles claim. Resolving
+// against users.yml means the session keeps exactly the permissions it is
+// configured for instead of silently losing all of them on upgrade.
+func TestSimpleAuthResolvesTokenWithNoRolesClaim(t *testing.T) {
+	users := UserDatabase{
+		Users: map[string]*User{
+			"alice": {Username: "alice", Password: "$2a$11$aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", RolesConfigured: "all", Roles: All},
+		},
+	}
+
+	a := NewSimpleAuth(users, 0)
+	_, token, err := a.tokenAuth.Encode(map[string]any{"username": "alice"})
+	require.NoError(t, err)
+
+	user := serveWithAuth(t, a, token)
+	require.NotNil(t, user)
+	require.Equal(t, All, user.Roles)
+}
+
+// No token at all stays unauthenticated so the login routes keep working.
+func TestSimpleAuthWithoutTokenHasNoUser(t *testing.T) {
+	users := UserDatabase{
+		Users: map[string]*User{
+			"alice": {Username: "alice", Password: "$2a$11$aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", RolesConfigured: "all", Roles: All},
+		},
+	}
+
+	require.Nil(t, serveWithAuth(t, NewSimpleAuth(users, 0), ""))
+}
+
+// The container filter comes from users.yml as well, parsed into labels when the
+// file is read so a token cannot pin a stale one.
+func TestSimpleAuthResolvesFilterFromDatabase(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "users.yml")
+	require.NoError(t, os.WriteFile(path, []byte(`
+users:
+  alice:
+    name: Alice
+    password: "$2y$11$pTGu6dnTT7Uh3ob7uC6X7OkAamlhHpJ0/mEbsmiPyO85pumillZme"
+    filter: "name=foo"
+`), 0o600))
+
+	users, err := ReadUsersFromFile(path)
+	require.NoError(t, err)
+
+	a := NewSimpleAuth(users, 0)
+	_, token, err := a.tokenAuth.Encode(map[string]any{"username": "alice", "filter": "name=stale"})
+	require.NoError(t, err)
+
+	user := serveWithAuth(t, a, token)
+	require.NotNil(t, user)
+	require.Equal(t, "name=foo", user.Filter)
+	require.Equal(t, []string{"foo"}, user.ContainerLabels["name"])
+	require.Equal(t, All, user.Roles, "no roles key in users.yml means everything")
+	require.Empty(t, user.Password, "the resolved user must not carry the password hash")
+}
+
+// An unparseable filter fails the read instead of silently locking the user out
+// of every container on their next request.
+func TestReadUsersRejectsInvalidFilter(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "users.yml")
+	require.NoError(t, os.WriteFile(path, []byte(`
+users:
+  alice:
+    password: "$2y$11$pTGu6dnTT7Uh3ob7uC6X7OkAamlhHpJ0/mEbsmiPyO85pumillZme"
+    filter: "nope"
+`), 0o600))
+
+	_, err := ReadUsersFromFile(path)
+	require.Error(t, err)
+}

internal/auth/users.go

@@ -11,7 +11,6 @@ import (
 	"time"
 
 	"github.com/amir20/dozzle/internal/container"
-	"github.com/go-chi/jwtauth/v5"
 	"github.com/rs/zerolog/log"
 	"golang.org/x/crypto/bcrypt"
 	"gopkg.in/yaml.v3"
@@ -117,6 +116,12 @@ func decodeUsersFromFile(path string) (UserDatabase, error) {
 		}
 
 		user.Roles = ParseRole(user.RolesConfigured)
+
+		labels, err := container.ParseContainerFilter(user.Filter)
+		if err != nil {
+			return users, fmt.Errorf("user %s has an invalid filter %q: %w", username, user.Filter, err)
+		}
+		user.ContainerLabels = labels
 	}
 
 	return users, nil
@@ -156,20 +161,6 @@ func (u *UserDatabase) Find(username string) *User {
 	return user
 }
 
-func (u *UserDatabase) FindByPassword(username, password string) *User {
-	user := u.Find(username)
-
-	if user == nil {
-		return nil
-	}
-
-	if !CompareHashAndPassword(user.Password, password) {
-		return nil
-	}
-
-	return user
-}
-
 func CompareHashAndPassword(hash, password string) bool {
 	if len(hash) == 64 {
 		log.Fatal().Msg("sha256 passwords are no longer supported. Please use bcrypt. See https://github.com/amir20/dozzle/security/advisories/GHSA-w7qr-q9fh-fj35 for more details.")
@@ -185,47 +176,17 @@ func CompareHashAndPassword(hash, password string) bool {
 	return false
 }
 
+// UserFromContext returns the user an authentication middleware resolved for this
+// request. Both providers resolve the user themselves: proxy auth from the request
+// headers, simple auth from users.yml keyed by the verified token's username. Roles
+// deliberately are not read back out of the JWT, because a bitmask frozen at login
+// goes stale the moment the role set grows or users.yml changes.
 func UserFromContext(ctx context.Context) *User {
 	if user, ok := ctx.Value(remoteUser).(User); ok {
 		return &user
-	} else {
-		if _, claims, err := jwtauth.FromContext(ctx); err == nil {
-			username, ok := claims["username"].(string)
-			if !ok {
-				return nil
-			}
-			if username == "" {
-				return nil
-			}
-			email := claims["email"].(string)
-			name := claims["name"].(string)
-			containerFilter := container.ContainerLabels{}
-
-			if filter, ok := claims["filter"].(string); ok {
-				containerFilter, err = container.ParseContainerFilter(filter)
-				if err != nil {
-					log.Warn().Err(err).Str("filter", filter).Msg("Failed to parse container filter")
-					return nil
-				}
-			}
-			// A token minted before roles existed carries no roles claim at all.
-			// Defaulting to None silently stripped every permission from a session
-			// that is otherwise still valid, so an upgrade quietly hid notifications,
-			// cloud and downloads until the user happened to log out. Absent means
-			// full access here, matching an absent `roles` in users.yml and an absent
-			// roles header in proxy auth.
-			roles := All
-			if r, ok := claims["roles"].(float64); ok {
-				roles = Role(r)
-			} else if claims["roles"] != nil {
-				log.Warn().Interface("roles", claims["roles"]).Msg("Failed to parse roles from JWT claims")
-			}
-
-			user := newUser(username, email, name, containerFilter, roles)
-			return &user
-		}
-		return nil
 	}
+
+	return nil
 }
 
 func RequireAuthentication(next http.Handler) http.Handler {

internal/auth/users_test.go

@@ -1,89 +1,11 @@
 package auth
 
 import (
-	"net/http"
-	"net/http/httptest"
 	"testing"
 
-	"github.com/go-chi/jwtauth/v5"
 	"github.com/stretchr/testify/require"
 )
 
-// userFromToken runs claims through the real Verifier so the test exercises the
-// same path a browser's session cookie takes.
-func userFromToken(t *testing.T, claims map[string]any) *User {
-	t.Helper()
-
-	tokenAuth := jwtauth.New("HS256", []byte("secret"), nil)
-	_, tokenString, err := tokenAuth.Encode(claims)
-	require.NoError(t, err)
-
-	var user *User
-	handler := jwtauth.Verifier(tokenAuth)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		user = UserFromContext(r.Context())
-	}))
-
-	req := httptest.NewRequest(http.MethodGet, "/", nil)
-	req.Header.Set("Authorization", "Bearer "+tokenString)
-	handler.ServeHTTP(httptest.NewRecorder(), req)
-
-	return user
-}
-
-// Tokens minted before roles existed carry no roles claim. Reading that as None
-// silently stripped every permission from sessions that were otherwise still
-// valid, so upgrading Dozzle hid notifications, cloud and downloads until the
-// user happened to log out. Absent means full access, the same as an absent
-// `roles` in users.yml and an absent roles header in proxy auth.
-func TestUserFromContextMissingRolesClaimGrantsAll(t *testing.T) {
-	user := userFromToken(t, map[string]any{
-		"username": "alice",
-		"email":    "alice@example.com",
-		"name":     "Alice",
-	})
-
-	require.NotNil(t, user)
-	require.Equal(t, All, user.Roles)
-	for name, role := range map[string]Role{
-		"shell":         Shell,
-		"actions":       Actions,
-		"download":      Download,
-		"notifications": Notifications,
-		"cloud":         Cloud,
-	} {
-		require.Truef(t, user.Roles.Has(role), "expected %s to be granted", name)
-	}
-}
-
-// An explicit claim still wins, including one that grants nothing.
-func TestUserFromContextHonoursExplicitRolesClaim(t *testing.T) {
-	user := userFromToken(t, map[string]any{
-		"username": "alice",
-		"email":    "alice@example.com",
-		"name":     "Alice",
-		"roles":    float64(Shell | Actions),
-	})
-
-	require.NotNil(t, user)
-	require.True(t, user.Roles.Has(Shell))
-	require.True(t, user.Roles.Has(Actions))
-	require.False(t, user.Roles.Has(Cloud))
-	require.False(t, user.Roles.Has(Download))
-}
-
-func TestUserFromContextExplicitNoneGrantsNothing(t *testing.T) {
-	user := userFromToken(t, map[string]any{
-		"username": "alice",
-		"email":    "alice@example.com",
-		"name":     "Alice",
-		"roles":    float64(None),
-	})
-
-	require.NotNil(t, user)
-	require.Equal(t, None, user.Roles)
-	require.False(t, user.Roles.Has(Cloud))
-}
-
 // Every role bit must be covered by All. Adding a new role without adding it to
 // All is what let Cloud fall outside the default in the first place.
 func TestAllCoversEveryRole(t *testing.T) {
@@ -98,8 +20,7 @@ func TestAllCoversEveryRole(t *testing.T) {
 	}
 }
 
-// A user in users.yml with no roles key gets everything, which is the behaviour
-// the JWT default above mirrors.
+// A user in users.yml with no roles key gets everything.
 func TestUserWithoutConfiguredRolesGetsAll(t *testing.T) {
 	require.Equal(t, All, ParseRole("all"))
 }