From aa31cd8a105071b710a195771a8fd1ac9c3e8c07 Mon Sep 17 00:00:00 2001 From: mrhid6 Date: Wed, 22 Jul 2026 10:26:21 +0100 Subject: [PATCH] fix: validate roles, guard last owner, scope bootstrap status Security review of e70b2f0. The UI gating was correctly backed by RequireRole everywhere; these are the missing validation gaps behind it. - UpdateUserRole and createOrgUser accepted any role string verbatim, so an admin could self-promote to owner, create an owner outright, or set a junk role that silently stripped a user's access. Roles are now whitelisted, only an owner may grant or remove the owner role, and an actor cannot change their own. - Neither demote nor delete guarded the last owner, so an org could reach zero owners. Both now refuse when no owner would remain, returning 409. Self-delete rejected. - CountUsers counted across all orgs, so a locked-out org could never re-bootstrap once another tenant existed, and the unauthenticated bootstrap-status endpoint reported instance-wide state. It now answers per-org on an org host, falling back to global only on the apex. - HandleMe repeats the middleware's host/org check; it sits outside the middleware so it can still return its own 401. - Post-bootstrap now sends the new owner to their org host's login page. The session cookie is deliberately scoped to the exact host, so the old redirect landed them unauthenticated. - AuthProvider renders an error state instead of mounting the shell with a null user when /auth/me fails for a reason other than 401. - api.ts unwraps {"error": ...} so these messages render as text. --- server/internal/api/org.go | 71 +++++++++++++++++++++++++-- server/internal/auth/local.go | 25 +++++++++- server/internal/models/user.go | 16 ++++++ server/internal/services/users.go | 76 ++++++++++++++++++++++++++++- web/app/(app)/settings/org/page.tsx | 30 +++++++++--- web/app/login/page.tsx | 29 +++++++---- web/app/setup/page.tsx | 49 ++++++++++++++++--- web/components/AuthProvider.tsx | 36 +++++++++++++- web/lib/api.ts | 13 ++++- 9 files changed, 312 insertions(+), 33 deletions(-) diff --git a/server/internal/api/org.go b/server/internal/api/org.go index 448e966..bc02476 100644 --- a/server/internal/api/org.go +++ b/server/internal/api/org.go @@ -1,10 +1,12 @@ package api import ( + "errors" "net/http" "github.com/gin-gonic/gin" "github.com/mrhid6/vantage/server/internal/auth" + "github.com/mrhid6/vantage/server/internal/models" "github.com/mrhid6/vantage/server/internal/services" ) @@ -17,6 +19,13 @@ func listOrgUsers(c *gin.Context) { c.JSON(http.StatusOK, users) } +// Granting or removing the owner role is reserved to owners: an admin must +// never be able to mint an owner (and log in as it) or strip the owners above +// them. Everything below derives the actor from the session, never the body. +func actorMayGrantOwner(c *gin.Context) bool { + return auth.Role(c) == models.RoleOwner +} + func createOrgUser(c *gin.Context) { var body struct { Email string `json:"email"` @@ -28,7 +37,15 @@ func createOrgUser(c *gin.Context) { return } if body.Role == "" { - body.Role = "member" + body.Role = models.RoleMember + } + if !models.ValidRole(body.Role) { + c.JSON(http.StatusBadRequest, gin.H{"error": "role must be one of owner, admin, or member"}) + return + } + if body.Role == models.RoleOwner && !actorMayGrantOwner(c) { + c.JSON(http.StatusForbidden, gin.H{"error": "only an owner can create another owner"}) + return } u, err := services.CreateUser(auth.OrgID(c), body.Email, body.Password, body.Role, "local") if err != nil { @@ -46,21 +63,65 @@ func updateOrgUserRole(c *gin.Context) { c.JSON(http.StatusBadRequest, gin.H{"error": "role required"}) return } - if err := services.UpdateUserRole(auth.OrgID(c), c.Param("id"), body.Role); err != nil { - c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + if !models.ValidRole(body.Role) { + c.JSON(http.StatusBadRequest, gin.H{"error": "role must be one of owner, admin, or member"}) + return + } + + orgID, targetID := auth.OrgID(c), c.Param("id") + if targetID == auth.UserID(c) { + c.JSON(http.StatusForbidden, gin.H{"error": "you cannot change your own role"}) + return + } + target, err := services.GetUserInOrg(orgID, targetID) + if err != nil { + c.JSON(http.StatusNotFound, gin.H{"error": "user not found"}) + return + } + if (body.Role == models.RoleOwner || target.Role == models.RoleOwner) && !actorMayGrantOwner(c) { + c.JSON(http.StatusForbidden, gin.H{"error": "only an owner can change owner roles"}) + return + } + + if err := services.UpdateUserRole(orgID, targetID, body.Role); err != nil { + c.JSON(orgUserErrStatus(err), gin.H{"error": err.Error()}) return } c.JSON(http.StatusOK, gin.H{"ok": true}) } func deleteOrgUser(c *gin.Context) { - if err := services.DeleteUser(auth.OrgID(c), c.Param("id")); err != nil { - c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + orgID, targetID := auth.OrgID(c), c.Param("id") + if targetID == auth.UserID(c) { + c.JSON(http.StatusForbidden, gin.H{"error": "you cannot remove your own account"}) + return + } + target, err := services.GetUserInOrg(orgID, targetID) + if err != nil { + c.JSON(http.StatusNotFound, gin.H{"error": "user not found"}) + return + } + if target.Role == models.RoleOwner && !actorMayGrantOwner(c) { + c.JSON(http.StatusForbidden, gin.H{"error": "only an owner can remove another owner"}) + return + } + + if err := services.DeleteUser(orgID, targetID); err != nil { + c.JSON(orgUserErrStatus(err), gin.H{"error": err.Error()}) return } c.JSON(http.StatusOK, gin.H{"deleted": true}) } +// The last-owner guard is a rejected request, not a server fault — surface it +// as 409 so the UI shows the message rather than a generic failure. +func orgUserErrStatus(err error) int { + if errors.Is(err, services.ErrLastOwner) { + return http.StatusConflict + } + return http.StatusInternalServerError +} + func getOrgOIDC(c *gin.Context) { cfg, err := services.GetOrgOIDC(auth.OrgID(c)) if err != nil { diff --git a/server/internal/auth/local.go b/server/internal/auth/local.go index d705716..7a3ec31 100644 --- a/server/internal/auth/local.go +++ b/server/internal/auth/local.go @@ -46,8 +46,20 @@ func HandleLocalLogin(c *gin.Context) { c.JSON(http.StatusOK, gin.H{"ok": true}) } +// HandleBootstrapStatus answers "does the caller need to run setup". It is +// unauthenticated, so it must not report instance-wide state to whoever asks: +// on an org host the answer is scoped to that org, and only the apex — the +// genuine first-run entry point — gets the global "no users anywhere" answer. func HandleBootstrapStatus(c *gin.Context) { - n, err := services.CountUsers() + var ( + n int64 + err error + ) + if org, ok := OrgFromHost(c); ok { + n, err = services.CountOrgUsers(org.OrgID) + } else { + n, err = services.CountUsers() + } if err != nil { c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return @@ -55,6 +67,9 @@ func HandleBootstrapStatus(c *gin.Context) { c.JSON(http.StatusOK, gin.H{"needs_setup": n == 0}) } +// HandleBootstrap creates the very first org and its owner, so its guard stays +// deliberately global: it may run once on an empty instance and never again, +// regardless of which host it is called on. func HandleBootstrap(c *gin.Context) { n, err := services.CountUsers() if err != nil { @@ -106,6 +121,14 @@ func HandleMe(c *gin.Context) { c.JSON(http.StatusUnauthorized, gin.H{"error": "session expired"}) return } + // /auth/me is registered outside Middleware, so it repeats the middleware's + // host/org match itself. Without this, a session for org A presented on org + // B's host would render the shell while every /api call 403s. + if hostOrg, ok := OrgFromHost(c); ok && hostOrg.OrgID != sess.OrgID { + c.JSON(http.StatusForbidden, gin.H{"error": "org host mismatch"}) + return + } + org, _ := services.GetOrg(sess.OrgID) c.JSON(http.StatusOK, gin.H{"user": sess, "org": org}) } diff --git a/server/internal/models/user.go b/server/internal/models/user.go index 59c8e83..e69cefc 100644 --- a/server/internal/models/user.go +++ b/server/internal/models/user.go @@ -6,6 +6,22 @@ import ( "go.mongodb.org/mongo-driver/v2/bson" ) +// Org membership roles. These are the only values ever written to User.Role; +// anything arriving from a client must be checked with ValidRole first. +const ( + RoleOwner = "owner" + RoleAdmin = "admin" + RoleMember = "member" +) + +func ValidRole(role string) bool { + switch role { + case RoleOwner, RoleAdmin, RoleMember: + return true + } + return false +} + type User struct { ID bson.ObjectID `bson:"_id,omitempty" json:"_id,omitempty"` UserID string `bson:"user_id" json:"user_id"` diff --git a/server/internal/services/users.go b/server/internal/services/users.go index 2a18491..e6728b4 100644 --- a/server/internal/services/users.go +++ b/server/internal/services/users.go @@ -2,6 +2,7 @@ package services import ( "context" + "errors" "fmt" "strings" "time" @@ -14,17 +15,56 @@ import ( "golang.org/x/crypto/bcrypt" ) +// ErrLastOwner is returned when an operation would leave an org with no owner, +// which would lock every remaining member out of org administration. +var ErrLastOwner = errors.New("this is the organization's last owner — promote another member to owner first") + +// CountUsers counts users across the whole instance. It answers "is this a +// brand new deployment", so it is deliberately unscoped; anything that asks +// about a single tenant must use CountOrgUsers. func CountUsers() (int64, error) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() return db.Col("users").CountDocuments(ctx, bson.M{}) } +func CountOrgUsers(orgID string) (int64, error) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + return db.Col("users").CountDocuments(ctx, bson.M{"org_id": orgID}) +} + +// countOtherOwners counts owner-role users in the org excluding exceptUserID, +// i.e. how many owners would remain if that user were removed or demoted. +func countOtherOwners(orgID, exceptUserID string) (int64, error) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + return db.Col("users").CountDocuments(ctx, bson.M{ + "org_id": orgID, + "role": models.RoleOwner, + "user_id": bson.M{"$ne": exceptUserID}, + }) +} + +func GetUserInOrg(orgID, userID string) (*models.User, error) { + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + var u models.User + err := db.Col("users").FindOne(ctx, bson.M{"user_id": userID, "org_id": orgID}).Decode(&u) + if err != nil { + return nil, err + } + return &u, nil +} + func CreateUser(orgID, email, password, role, authSource string) (*models.User, error) { email = strings.ToLower(strings.TrimSpace(email)) if email == "" { return nil, fmt.Errorf("email required") } + if !models.ValidRole(role) { + return nil, fmt.Errorf("invalid role %q", role) + } u := &models.User{ UserID: uuid.NewString(), OrgID: orgID, @@ -95,17 +135,49 @@ func ListUsers(orgID string) ([]models.User, error) { } func UpdateUserRole(orgID, userID, role string) error { + if !models.ValidRole(role) { + return fmt.Errorf("invalid role %q", role) + } + target, err := GetUserInOrg(orgID, userID) + if err != nil { + return fmt.Errorf("user not found") + } + // Demoting the final owner would leave nobody able to administer the org. + if target.Role == models.RoleOwner && role != models.RoleOwner { + others, err := countOtherOwners(orgID, userID) + if err != nil { + return err + } + if others == 0 { + return ErrLastOwner + } + } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - _, err := db.Col("users").UpdateOne(ctx, + _, err = db.Col("users").UpdateOne(ctx, bson.M{"user_id": userID, "org_id": orgID}, bson.M{"$set": bson.M{"role": role}}) return err } func DeleteUser(orgID, userID string) error { + target, err := GetUserInOrg(orgID, userID) + if err != nil { + return fmt.Errorf("user not found") + } + if target.Role == models.RoleOwner { + others, err := countOtherOwners(orgID, userID) + if err != nil { + return err + } + if others == 0 { + return ErrLastOwner + } + } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - _, err := db.Col("users").DeleteOne(ctx, bson.M{"user_id": userID, "org_id": orgID}) + _, err = db.Col("users").DeleteOne(ctx, bson.M{"user_id": userID, "org_id": orgID}) return err } diff --git a/web/app/(app)/settings/org/page.tsx b/web/app/(app)/settings/org/page.tsx index 31e87b2..4089392 100644 --- a/web/app/(app)/settings/org/page.tsx +++ b/web/app/(app)/settings/org/page.tsx @@ -50,16 +50,26 @@ function MembersCard() { }, }); - const { mutate: changeRole } = useMutation({ + const { mutate: changeRole, error: roleError } = useMutation({ mutationFn: ({ userId, next }: { userId: string; next: Role }) => api.updateOrgUserRole(userId, next), onSuccess: invalidate, + // A rejected change (last owner, owner-only grant) leaves the select showing + // the value the server refused — refetch so the row snaps back to the truth. + onError: invalidate, }); - const { mutate: removeUser } = useMutation({ + const { mutate: removeUser, error: removeError } = useMutation({ mutationFn: (userId: string) => api.deleteOrgUser(userId), onSuccess: invalidate, }); + const actionError = (roleError ?? removeError) as Error | null; + + // The server lets only an owner grant or change the owner role. Mirror that + // here so admins aren't offered controls that can only 403. + const isOwner = user?.role === "owner"; + const assignableRoles = isOwner ? ROLES : ROLES.filter((r) => r !== "owner"); + return (
@@ -74,6 +84,12 @@ function MembersCard() {
+ {actionError && ( +
+ {actionError.message} +
+ )} + {isLoading ? (
@@ -96,6 +112,8 @@ function MembersCard() { {users.map((u: OrgUser) => { const isSelf = u.user_id === user?.user_id; + // Own row stays read-only, and only owners may act on owners. + const locked = isSelf || (u.role === "owner" && !isOwner); return ( @@ -103,7 +121,7 @@ function MembersCard() { {isSelf && (you)} - {isSelf ? ( + {locked ? ( {u.role} ) : ( setRole(e.target.value as Role)} className={inputClass}> - {ROLES.map((r) => ( + {assignableRoles.map((r) => ( diff --git a/web/app/login/page.tsx b/web/app/login/page.tsx index c669e2e..5eb3aa2 100644 --- a/web/app/login/page.tsx +++ b/web/app/login/page.tsx @@ -9,16 +9,27 @@ export default function LoginPage() { const [email, setEmail] = useState(""); const [password, setPassword] = useState(""); - // If the instance has no users yet, first-run setup is the only way in. + // If the org has no users yet, first-run setup is the only way in. And if the + // visitor already has a valid session on this host, the form is a dead end — + // send them into the app instead. useEffect(() => { - auth - .bootstrapStatus() - .then((s) => { - if (s.needs_setup) window.location.href = "/setup"; - }) - .catch(() => { - // Status unavailable — let the login form stand. - }); + (async () => { + try { + const s = await auth.bootstrapStatus(); + if (s.needs_setup) { + window.location.href = "/setup"; + return; + } + } catch { + // Status unavailable — fall through and let the login form stand. + } + try { + await auth.me(); + window.location.href = "/"; + } catch { + // Not signed in (or session invalid here) — show the form. + } + })(); }, []); const { mutate: signIn, isPending, error } = useMutation({ diff --git a/web/app/setup/page.tsx b/web/app/setup/page.tsx index 36b2809..835b482 100644 --- a/web/app/setup/page.tsx +++ b/web/app/setup/page.tsx @@ -12,20 +12,25 @@ const MIN_PASSWORD_LENGTH = 8; * auth.hostSlug on the server). Build the new org's URL by prepending — or * replacing — the leftmost label. Hosts that don't match that shape (localhost, * bare IPs) have no per-org subdomain, so stay put. + * + * Setup runs on the apex, and the session cookie it sets is scoped to that + * exact host by design — org hosts must not share cookies. So the new owner is + * sent to the org host's *login* page to sign in there, which is what puts a + * session cookie on the host their org actually lives on. */ -function orgUrlForSlug(slug: string): string { - if (typeof window === "undefined") return "/"; +function orgLoginUrlForSlug(slug: string): string { + if (typeof window === "undefined") return "/login"; const { protocol, host } = window.location; const [hostname, port] = host.split(":"); const parts = hostname.split("."); - if (parts.length < 2 || parts[parts.length - 1] === "localhost") return "/"; + if (parts.length < 2 || parts[parts.length - 1] === "localhost") return "/login"; const rest = parts[0] === "vantage" ? parts : parts.slice(1); - if (rest[0] !== "vantage") return "/"; + if (rest[0] !== "vantage") return "/login"; const newHost = [slug, ...rest].join(".") + (port ? `:${port}` : ""); - return `${protocol}//${newHost}/`; + return `${protocol}//${newHost}/login`; } export default function SetupPage() { @@ -34,6 +39,7 @@ export default function SetupPage() { const [password, setPassword] = useState(""); const [confirm, setConfirm] = useState(""); const [validationError, setValidationError] = useState(null); + const [created, setCreated] = useState<{ slug: string; loginUrl: string } | null>(null); // Setup is a one-shot route; once an owner exists it must not be reachable. useEffect(() => { @@ -50,7 +56,7 @@ export default function SetupPage() { const { mutate: bootstrap, isPending, error } = useMutation({ mutationFn: () => auth.bootstrap({ org_name: orgName, email, password }), onSuccess: (res) => { - window.location.href = orgUrlForSlug(res.slug); + setCreated({ slug: res.slug, loginUrl: orgLoginUrlForSlug(res.slug) }); }, }); @@ -74,6 +80,37 @@ export default function SetupPage() { const inputClass = "w-full rounded-lg border border-border bg-surface-2 px-3 py-2 text-sm text-text-primary focus:border-accent/50 focus:outline-none focus:ring-1 focus:ring-accent/30"; + if (created) { + return ( +
+
+
+

Organization created

+

+ Your owner account is ready. One more step to finish signing in. +

+
+ + +

+ {created.slug} has its own address, and sign-in is kept separate per organization. Continue + to your organization's sign-in page and log in with the email and password you just + chose. +

+ + {created.loginUrl} + + + + + +
+
+ ); + } + return (
diff --git a/web/components/AuthProvider.tsx b/web/components/AuthProvider.tsx index af151e6..0edfb9b 100644 --- a/web/components/AuthProvider.tsx +++ b/web/components/AuthProvider.tsx @@ -26,6 +26,7 @@ export function AuthProvider({ children }: { children: ReactNode }) { const [user, setUser] = useState(null); const [org, setOrg] = useState(null); const [loading, setLoading] = useState(true); + const [error, setError] = useState(null); useEffect(() => { let cancelled = false; @@ -50,7 +51,11 @@ export function AuthProvider({ children }: { children: ReactNode }) { window.location.href = "/login"; return; } - // Backend unreachable or unexpected failure — don't trap the user on a spinner. + // Anything else (backend unreachable, org host mismatch) leaves us with + // no session. Rendering children here would mount the whole shell with + // user=null — every page would fire its own doomed API calls and the UI + // would read as a member view. Show the failure instead. + setError((err as Error).message || "Unable to load your session."); setLoading(false); } })(); @@ -68,7 +73,34 @@ export function AuthProvider({ children }: { children: ReactNode }) { ); } - const isAdmin = user?.role === "owner" || user?.role === "admin"; + if (error || !user) { + return ( +
+
+

Can't load your session

+

+ {error ?? "Unable to load your session."} +

+
+ + + Sign in + +
+
+
+ ); + } + + const isAdmin = user.role === "owner" || user.role === "admin"; return ( {children} diff --git a/web/lib/api.ts b/web/lib/api.ts index e7dcf72..e5971c8 100644 --- a/web/lib/api.ts +++ b/web/lib/api.ts @@ -385,8 +385,17 @@ async function request(path: string, options?: RequestInit): Promise { }); if (!res.ok) { - const text = await res.text().catch(() => res.statusText); - throw new ApiError(res.status, text || `HTTP ${res.status}`); + const text = await res.text().catch(() => ""); + // Handlers report failures as {"error": "..."} — unwrap it so the message + // reaching the UI is the sentence the backend wrote, not raw JSON. + let message = text || res.statusText || `HTTP ${res.status}`; + try { + const body = JSON.parse(text); + if (body?.error) message = body.error; + } catch { + // non-JSON body — keep the text as-is + } + throw new ApiError(res.status, message); } if (res.status === 204) {