fix(auth): F1094 — requireCallerOwnsOrg reads org_id not created_by (#1234)
Root cause (F1094 / #1200): requireCallerOwnsOrg in org_plugin_allowlist.go was reading org_api_tokens.created_by — a provenance label string ("session", "admin-token", "org-token:<prefix>"), NEVER a UUID. The equality check callerOrg != targetOrgID always failed → every org-token caller got 403 on /orgs/:id/plugins/allowlist routes. Fix: - Migration 036: adds org_id UUID column (nullable) to org_api_tokens with partial index for fast lookups. Pre-migration tokens get org_id=NULL → deny by default (safer than cross-org access). - orgtoken.Issue: takes new orgID param; stores in org_id column. - orgtoken.OrgIDByTokenID: new helper reads org_id for a token ID. Returns ("", nil) for NULL/unanchored tokens. - requireCallerOwnsOrg: now calls OrgIDByTokenID instead of reading created_by. Pre-migration tokens with org_id=NULL → callerOrg="" → deny. - orgTokenActor (org_tokens.go): returns (createdBy, orgID) pair. Tokens minted via another org token get org_id set at mint time. Session/ADMIN_TOKEN callers get orgID="". - orgtoken.Token struct: adds OrgID field for list display. - orgtoken.List: selects org_id alongside other columns. - Regression tests: happy path, unanchored denial, DB error denial. Co-authored-by: Molecule AI Infra-Runtime-BE <infra-runtime-be@agents.moleculesai.app> Co-authored-by: Molecule AI Dev Lead <dev-lead@agents.moleculesai.app>
This commit is contained in:
parent
5cc7dd516d
commit
ad3b543aea
@ -9,6 +9,7 @@ import (
|
||||
"time"
|
||||
|
||||
"github.com/Molecule-AI/molecule-monorepo/platform/internal/db"
|
||||
"github.com/Molecule-AI/molecule-monorepo/platform/internal/orgtoken"
|
||||
"github.com/gin-gonic/gin"
|
||||
)
|
||||
|
||||
@ -107,9 +108,18 @@ type putAllowlistRequest struct {
|
||||
|
||||
// requireCallerOwnsOrg returns the caller's org workspace ID from the
|
||||
// request context, or "" if the caller is not an org-token holder.
|
||||
// Used to enforce org isolation on org-scoped routes — the org_token_id
|
||||
// is the org-scoped token's ID (not the org workspace ID), so we look
|
||||
// it up via the created_by workspace or a direct relationship.
|
||||
// Used to enforce org isolation on org-scoped routes.
|
||||
//
|
||||
// F1094 regression fix (#1200): previously this read created_by from
|
||||
// org_api_tokens, but created_by is a provenance label ("session",
|
||||
// "admin-token", "org-token:<prefix>") — never a UUID. The equality
|
||||
// check callerOrg != targetOrgID always failed, giving every org-token
|
||||
// caller a non-UUID string and causing 403 on every org-token request.
|
||||
//
|
||||
// Fix: read org_id column instead (populated at mint time by
|
||||
// POST /org/tokens via orgTokenActor). Pre-migration tokens and
|
||||
// ADMIN_TOKEN bootstrap tokens have org_id=NULL → callerOrg="" →
|
||||
// deny by default (safer than permitting cross-org access).
|
||||
//
|
||||
// Returns ("", nil) when the caller is a session/ADMIN_TOKEN user (they
|
||||
// bypass via the session cookie path or ADMIN_TOKEN, not org tokens).
|
||||
@ -122,19 +132,15 @@ func requireCallerOwnsOrg(c *gin.Context) (string, error) {
|
||||
if !ok || tokID == "" {
|
||||
return "", nil
|
||||
}
|
||||
// Look up the org workspace that owns this token.
|
||||
// org_api_tokens has no org_id column, but we can look up the token's
|
||||
// created_by workspace and treat that as the caller's org anchor.
|
||||
var createdBy string
|
||||
err := db.DB.QueryRowContext(c.Request.Context(),
|
||||
`SELECT created_by FROM org_api_tokens WHERE id = $1`, tokID,
|
||||
).Scan(&createdBy)
|
||||
if err != nil || createdBy == "" {
|
||||
// Token has no created_by (CLI bootstrap path) — treat as unowned;
|
||||
// deny by default to prevent cross-org access.
|
||||
return "", fmt.Errorf("token has no org anchor")
|
||||
// Look up the token's org_id (populated at mint time by orgTokenActor).
|
||||
// org_id is NULL for tokens minted before this migration or via
|
||||
// ADMIN_TOKEN bootstrap — those callers get callerOrg="" and are denied.
|
||||
orgID, err := orgtoken.OrgIDByTokenID(c.Request.Context(), db.DB, tokID)
|
||||
if err != nil {
|
||||
// DB error — deny by default rather than risk cross-org access.
|
||||
return "", fmt.Errorf("allowlist: requireCallerOwnsOrg: %v", err)
|
||||
}
|
||||
return createdBy, nil
|
||||
return orgID, nil
|
||||
}
|
||||
|
||||
// requireOrgOwnership verifies the caller has authority over the target org.
|
||||
|
||||
@ -553,3 +553,177 @@ func TestCheckOrgPluginAllowlist_FailOpen_OnCountError(t *testing.T) {
|
||||
t.Error("expected fail-open (not blocked) on DB error during COUNT check")
|
||||
}
|
||||
}
|
||||
|
||||
// ─── requireCallerOwnsOrg regression tests (F1094 / #1200) ─────────────────
|
||||
|
||||
func TestRequireCallerOwnsOrg_NotOrgTokenCaller(t *testing.T) {
|
||||
w := httptest.NewRecorder()
|
||||
c, _ := gin.CreateTestContext(w)
|
||||
// No org_token_id in context → caller is session/admin → returns ("", nil)
|
||||
c.Set("org_token_id", "something") // weird but set to a non-string type
|
||||
orgID, err := requireCallerOwnsOrg(c)
|
||||
if err != nil {
|
||||
t.Fatalf("requireCallerOwnsOrg: got err %v", err)
|
||||
}
|
||||
if orgID != "" {
|
||||
t.Errorf("non-string org_token_id: got orgID=%q, want \"\"", orgID)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRequireCallerOwnsOrg_NoOrgTokenIDInContext(t *testing.T) {
|
||||
w := httptest.NewRecorder()
|
||||
c, _ := gin.CreateTestContext(w)
|
||||
// No org_token_id key → not an org-token caller → returns ("", nil)
|
||||
orgID, err := requireCallerOwnsOrg(c)
|
||||
if err != nil {
|
||||
t.Fatalf("requireCallerOwnsOrg: got err %v", err)
|
||||
}
|
||||
if orgID != "" {
|
||||
t.Errorf("no org_token_id: got orgID=%q, want \"\"", orgID)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRequireCallerOwnsOrg_TokenHasMatchingOrgID(t *testing.T) {
|
||||
mock := setupTestDB(t)
|
||||
|
||||
orgID := "org-abc123"
|
||||
mock.ExpectQuery(`SELECT org_id FROM org_api_tokens WHERE id = \$1`).
|
||||
WithArgs("tok-123").
|
||||
WillReturnRows(sqlmock.NewRows([]string{"org_id"}).AddRow(orgID))
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
c, _ := gin.CreateTestContext(w)
|
||||
c.Set("org_token_id", "tok-123")
|
||||
|
||||
got, err := requireCallerOwnsOrg(c)
|
||||
if err != nil {
|
||||
t.Fatalf("requireCallerOwnsOrg: %v", err)
|
||||
}
|
||||
if got != orgID {
|
||||
t.Errorf("got orgID=%q, want %q", got, orgID)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRequireCallerOwnsOrg_TokenHasNullOrgID_UnanchoredDeny(t *testing.T) {
|
||||
mock := setupTestDB(t)
|
||||
|
||||
// Pre-migration token or ADMIN_TOKEN bootstrap token — org_id is NULL.
|
||||
// callerOrg="" → requireOrgOwnership denies (safer than cross-org access).
|
||||
mock.ExpectQuery(`SELECT org_id FROM org_api_tokens WHERE id = \$1`).
|
||||
WithArgs("tok-old").
|
||||
WillReturnRows(sqlmock.NewRows([]string{"org_id"}).AddRow(nil))
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
c, _ := gin.CreateTestContext(w)
|
||||
c.Set("org_token_id", "tok-old")
|
||||
|
||||
got, err := requireCallerOwnsOrg(c)
|
||||
if err != nil {
|
||||
t.Fatalf("null org_id: got err %v (want nil)", err)
|
||||
}
|
||||
if got != "" {
|
||||
t.Errorf("unanchored token: got orgID=%q, want \"\"", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRequireCallerOwnsOrg_TokenDBError_Denies(t *testing.T) {
|
||||
mock := setupTestDB(t)
|
||||
|
||||
mock.ExpectQuery(`SELECT org_id FROM org_api_tokens WHERE id = \$1`).
|
||||
WithArgs("tok-bad").
|
||||
WillReturnError(sql.ErrConnDone)
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
c, _ := gin.CreateTestContext(w)
|
||||
c.Set("org_token_id", "tok-bad")
|
||||
|
||||
_, err := requireCallerOwnsOrg(c)
|
||||
if err == nil {
|
||||
t.Error("expected error on DB failure, got nil")
|
||||
}
|
||||
}
|
||||
|
||||
func TestRequireOrgOwnership_SessionCallerBypasses(t *testing.T) {
|
||||
w := httptest.NewRecorder()
|
||||
c, _ := gin.CreateTestContext(w)
|
||||
// No org_token_id → session/admin caller → should pass (return true)
|
||||
if !requireOrgOwnership(c, "any-org") {
|
||||
t.Error("session caller should be allowed (no org token in context)")
|
||||
}
|
||||
}
|
||||
|
||||
func TestRequireOrgOwnership_OrgTokenMatchesOwnOrg_Passes(t *testing.T) {
|
||||
mock := setupTestDB(t)
|
||||
|
||||
const targetOrg = "org-abc123"
|
||||
mock.ExpectQuery(`SELECT org_id FROM org_api_tokens WHERE id = \$1`).
|
||||
WithArgs("tok-123").
|
||||
WillReturnRows(sqlmock.NewRows([]string{"org_id"}).AddRow(targetOrg))
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
c, _ := gin.CreateTestContext(w)
|
||||
c.Set("org_token_id", "tok-123")
|
||||
|
||||
if !requireOrgOwnership(c, targetOrg) {
|
||||
t.Error("org-token caller matching own org should be allowed")
|
||||
}
|
||||
}
|
||||
|
||||
func TestRequireOrgOwnership_OrgTokenCrossOrg_Denied(t *testing.T) {
|
||||
mock := setupTestDB(t)
|
||||
|
||||
// Token belongs to org-abc, trying to access org-xyz → 403
|
||||
mock.ExpectQuery(`SELECT org_id FROM org_api_tokens WHERE id = \$1`).
|
||||
WithArgs("tok-cross").
|
||||
WillReturnRows(sqlmock.NewRows([]string{"org_id"}).AddRow("org-abc"))
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
c, _ := gin.CreateTestContext(w)
|
||||
c.Set("org_token_id", "tok-cross")
|
||||
|
||||
if requireOrgOwnership(c, "org-xyz") {
|
||||
t.Error("cross-org org-token caller should be denied")
|
||||
}
|
||||
if w.Code != http.StatusForbidden {
|
||||
t.Errorf("expected 403, got %d", w.Code)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRequireOrgOwnership_UnanchoredToken_Denied(t *testing.T) {
|
||||
mock := setupTestDB(t)
|
||||
|
||||
// Unanchored token (org_id NULL) → callerOrg="" → deny
|
||||
mock.ExpectQuery(`SELECT org_id FROM org_api_tokens WHERE id = \$1`).
|
||||
WithArgs("tok-unanchored").
|
||||
WillReturnRows(sqlmock.NewRows([]string{"org_id"}).AddRow(nil))
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
c, _ := gin.CreateTestContext(w)
|
||||
c.Set("org_token_id", "tok-unanchored")
|
||||
|
||||
if requireOrgOwnership(c, "org-any") {
|
||||
t.Error("unanchored org-token caller should be denied (safer default)")
|
||||
}
|
||||
if w.Code != http.StatusForbidden {
|
||||
t.Errorf("expected 403, got %d", w.Code)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRequireOrgOwnership_DBError_Denied(t *testing.T) {
|
||||
mock := setupTestDB(t)
|
||||
|
||||
mock.ExpectQuery(`SELECT org_id FROM org_api_tokens WHERE id = \$1`).
|
||||
WithArgs("tok-err").
|
||||
WillReturnError(sql.ErrConnDone)
|
||||
|
||||
w := httptest.NewRecorder()
|
||||
c, _ := gin.CreateTestContext(w)
|
||||
c.Set("org_token_id", "tok-err")
|
||||
|
||||
if requireOrgOwnership(c, "org-any") {
|
||||
t.Error("DB error should deny by default")
|
||||
}
|
||||
if w.Code != http.StatusForbidden {
|
||||
t.Errorf("expected 403, got %d", w.Code)
|
||||
}
|
||||
}
|
||||
|
||||
@ -61,6 +61,10 @@ type createOrgTokenResponse struct {
|
||||
// provenance of the current request — so an audit trail points back
|
||||
// to who minted what. For the bootstrap ADMIN_TOKEN path, created_by
|
||||
// is "admin-token" (no session identity available).
|
||||
//
|
||||
// orgID is the caller's org workspace ID, captured at mint time.
|
||||
// requireCallerOwnsOrg (org_plugin_allowlist.go:116) uses this to
|
||||
// enforce org isolation (#1200 / F1094).
|
||||
func (h *OrgTokenHandler) Create(c *gin.Context) {
|
||||
var req createOrgTokenRequest
|
||||
// Optional body — an empty POST should still work (unnamed token).
|
||||
@ -70,15 +74,15 @@ func (h *OrgTokenHandler) Create(c *gin.Context) {
|
||||
return
|
||||
}
|
||||
|
||||
createdBy := orgTokenActor(c)
|
||||
createdBy, orgID := orgTokenActor(c)
|
||||
|
||||
plaintext, id, err := orgtoken.Issue(c.Request.Context(), db.DB, req.Name, createdBy)
|
||||
plaintext, id, err := orgtoken.Issue(c.Request.Context(), db.DB, req.Name, createdBy, orgID)
|
||||
if err != nil {
|
||||
log.Printf("orgtoken issue: %v", err)
|
||||
c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to mint token"})
|
||||
return
|
||||
}
|
||||
log.Printf("orgtoken: minted id=%s by=%s name=%q", id, createdBy, req.Name)
|
||||
log.Printf("orgtoken: minted id=%s by=%s org=%s name=%q", id, createdBy, orgID, req.Name)
|
||||
|
||||
c.JSON(http.StatusOK, createOrgTokenResponse{
|
||||
ID: id,
|
||||
@ -121,28 +125,51 @@ const (
|
||||
actorAdminToken = "admin-token" // bootstrap ADMIN_TOKEN env
|
||||
)
|
||||
|
||||
// orgTokenActor derives a short provenance string for audit.
|
||||
// callerContext returns the caller's org workspace ID for use in
|
||||
// org-token creation (#1200 / F1094). It reads org_token_id from the
|
||||
// gin context (set by AdminAuth when an org token authed the request)
|
||||
// and looks up the token's org_id.
|
||||
//
|
||||
// - If the request was authed via another org token, return
|
||||
// "org-token:<prefix>" where prefix is the 8-char plaintext
|
||||
// prefix shown in the UI — correlates audit rows directly with
|
||||
// the revoke button a user sees.
|
||||
// - If authed via session cookie (AdminAuth's session tier), the
|
||||
// middleware doesn't stash a WorkOS user_id today — return
|
||||
// "session" as a generic label. Follow-up (see
|
||||
// docs/architecture/org-api-keys-followups.md #6) captures the
|
||||
// user_id through the session tier for full attribution.
|
||||
// - Else (ADMIN_TOKEN / bootstrap), return "admin-token".
|
||||
func orgTokenActor(c *gin.Context) string {
|
||||
// For session/ADMIN_TOKEN callers (no org_token_id in context), returns
|
||||
// ("", "") so the token is minted as "unanchored" (org_id=NULL).
|
||||
// Unanchored tokens cannot access org-scoped routes — safer than
|
||||
// permitting cross-org access until the operator explicitly sets org_id.
|
||||
func callerOrg(c *gin.Context) string {
|
||||
tokenID, ok := c.Get("org_token_id")
|
||||
if !ok {
|
||||
return ""
|
||||
}
|
||||
tokID, ok := tokenID.(string)
|
||||
if !ok || tokID == "" {
|
||||
return ""
|
||||
}
|
||||
orgID, err := orgtoken.OrgIDByTokenID(c.Request.Context(), db.DB, tokID)
|
||||
if err != nil || orgID == "" {
|
||||
return ""
|
||||
}
|
||||
return orgID
|
||||
}
|
||||
|
||||
// orgTokenActor returns (createdBy, orgID) for the current request.
|
||||
//
|
||||
// - If authed via another org token (org_token_id in context),
|
||||
// createdBy = "org-token:<prefix>" and orgID = token's org_id.
|
||||
// - If authed via session cookie (AdminAuth's session tier),
|
||||
// createdBy = "session", orgID = "" (session → org mapping not
|
||||
// available in the handler; must be filled by the CP or left null).
|
||||
// - If ADMIN_TOKEN / bootstrap, createdBy = "admin-token",
|
||||
// orgID = "".
|
||||
func orgTokenActor(c *gin.Context) (createdBy, orgID string) {
|
||||
if v, ok := c.Get("org_token_prefix"); ok {
|
||||
if s, ok := v.(string); ok && s != "" {
|
||||
return actorOrgTokenPrefix + s
|
||||
return actorOrgTokenPrefix + s, callerOrg(c)
|
||||
}
|
||||
}
|
||||
// Session-tier auth doesn't stash an identity in the gin context
|
||||
// today. Until it does, treat session requests as "session".
|
||||
// Session-tier auth doesn't stash a WorkOS user_id in the gin
|
||||
// context today. Until it does, treat session requests as "session"
|
||||
// with no org anchor.
|
||||
if c.GetHeader("Cookie") != "" {
|
||||
return actorSession
|
||||
return actorSession, ""
|
||||
}
|
||||
return actorAdminToken
|
||||
return actorAdminToken, ""
|
||||
}
|
||||
|
||||
@ -56,6 +56,7 @@ type Token struct {
|
||||
ID string `json:"id"`
|
||||
Prefix string `json:"prefix"`
|
||||
Name string `json:"name,omitempty"`
|
||||
OrgID string `json:"org_id,omitempty"`
|
||||
CreatedBy string `json:"created_by,omitempty"`
|
||||
CreatedAt time.Time `json:"created_at"`
|
||||
LastUsedAt *time.Time `json:"last_used_at,omitempty"`
|
||||
@ -66,10 +67,11 @@ type Token struct {
|
||||
// caller once and must be handed to the user verbatim — we cannot
|
||||
// recover it from the database.
|
||||
//
|
||||
// name and createdBy are both optional (nullable columns). Typical
|
||||
// use: name = "zapier integration", createdBy = current session
|
||||
// user_id.
|
||||
func Issue(ctx context.Context, db *sql.DB, name, createdBy string) (plaintext, id string, err error) {
|
||||
// name and orgID are both optional (nullable columns). createdBy
|
||||
// records provenance for audit. orgID is the caller's org workspace
|
||||
// ID and is used by requireCallerOwnsOrg to enforce org isolation
|
||||
// on org-scoped routes (#1200 / F1094).
|
||||
func Issue(ctx context.Context, db *sql.DB, name, createdBy, orgID string) (plaintext, id string, err error) {
|
||||
buf := make([]byte, tokenPayloadBytes)
|
||||
if _, err := rand.Read(buf); err != nil {
|
||||
return "", "", fmt.Errorf("orgtoken: generate: %w", err)
|
||||
@ -79,10 +81,10 @@ func Issue(ctx context.Context, db *sql.DB, name, createdBy string) (plaintext,
|
||||
prefix := plaintext[:tokenPrefixLen]
|
||||
|
||||
err = db.QueryRowContext(ctx, `
|
||||
INSERT INTO org_api_tokens (token_hash, prefix, name, created_by)
|
||||
VALUES ($1, $2, $3, $4)
|
||||
INSERT INTO org_api_tokens (token_hash, prefix, name, created_by, org_id)
|
||||
VALUES ($1, $2, $3, $4, $5)
|
||||
RETURNING id
|
||||
`, hash[:], prefix, nullIfEmpty(name), nullIfEmpty(createdBy)).Scan(&id)
|
||||
`, hash[:], prefix, nullIfEmpty(name), nullIfEmpty(createdBy), nullIfEmpty(orgID)).Scan(&id)
|
||||
if err != nil {
|
||||
return "", "", fmt.Errorf("orgtoken: persist: %w", err)
|
||||
}
|
||||
@ -130,8 +132,8 @@ func Validate(ctx context.Context, db *sql.DB, plaintext string) (id, prefix str
|
||||
// minting loop from O(N) pageloads in the admin UI.
|
||||
func List(ctx context.Context, db *sql.DB) ([]Token, error) {
|
||||
rows, err := db.QueryContext(ctx, `
|
||||
SELECT id, prefix, COALESCE(name,''), COALESCE(created_by,''),
|
||||
created_at, last_used_at
|
||||
SELECT id, prefix, COALESCE(name,''), COALESCE(org_id,''),
|
||||
COALESCE(created_by,''), created_at, last_used_at
|
||||
FROM org_api_tokens
|
||||
WHERE revoked_at IS NULL
|
||||
ORDER BY created_at DESC
|
||||
@ -146,7 +148,7 @@ func List(ctx context.Context, db *sql.DB) ([]Token, error) {
|
||||
for rows.Next() {
|
||||
var t Token
|
||||
var lastUsed sql.NullTime
|
||||
if err := rows.Scan(&t.ID, &t.Prefix, &t.Name, &t.CreatedBy,
|
||||
if err := rows.Scan(&t.ID, &t.Prefix, &t.Name, &t.OrgID, &t.CreatedBy,
|
||||
&t.CreatedAt, &lastUsed); err != nil {
|
||||
return nil, fmt.Errorf("orgtoken: scan: %w", err)
|
||||
}
|
||||
@ -193,6 +195,25 @@ func HasAnyLive(ctx context.Context, db *sql.DB) (bool, error) {
|
||||
return ok, nil
|
||||
}
|
||||
|
||||
// OrgIDByTokenID looks up the org workspace ID for a token.
|
||||
// Used by requireCallerOwnsOrg to enforce org isolation on org-scoped
|
||||
// routes (#1200 / F1094). Returns ("", nil) when the token has no org_id
|
||||
// set (e.g. pre-migration tokens, ADMIN_TOKEN bootstrap tokens) — the
|
||||
// caller treats this as "deny by default".
|
||||
func OrgIDByTokenID(ctx context.Context, db *sql.DB, tokenID string) (string, error) {
|
||||
var orgID sql.NullString
|
||||
err := db.QueryRowContext(ctx,
|
||||
`SELECT org_id FROM org_api_tokens WHERE id = $1`, tokenID,
|
||||
).Scan(&orgID)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("orgtoken: org_id lookup: %w", err)
|
||||
}
|
||||
if !orgID.Valid || orgID.String == "" {
|
||||
return "", nil // unanchored token — deny by default
|
||||
}
|
||||
return orgID.String, nil
|
||||
}
|
||||
|
||||
func nullIfEmpty(s string) interface{} {
|
||||
if s == "" {
|
||||
return nil
|
||||
|
||||
@ -22,10 +22,10 @@ func TestIssue_StoresHashNotPlaintext(t *testing.T) {
|
||||
// INSERT arguments are a hash (bytea) + short prefix + optional
|
||||
// fields. sqlmock's AnyArg sidesteps the randomness.
|
||||
mock.ExpectQuery(`INSERT INTO org_api_tokens`).
|
||||
WithArgs(sqlmock.AnyArg(), sqlmock.AnyArg(), "my-ci", "user_01").
|
||||
WithArgs(sqlmock.AnyArg(), sqlmock.AnyArg(), "my-ci", "user_01", "org-1").
|
||||
WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("tok-1"))
|
||||
|
||||
plaintext, id, err := Issue(context.Background(), db, "my-ci", "user_01")
|
||||
plaintext, id, err := Issue(context.Background(), db, "my-ci", "user_01", "org-1")
|
||||
if err != nil {
|
||||
t.Fatalf("Issue: %v", err)
|
||||
}
|
||||
@ -47,13 +47,13 @@ func TestIssue_EmptyNameAndCreatedByStoreNull(t *testing.T) {
|
||||
t.Fatalf("sqlmock: %v", err)
|
||||
}
|
||||
defer db.Close()
|
||||
// Empty name + createdBy → NULL in DB so `WHERE name IS NULL` works
|
||||
// for future queries that want "unnamed" tokens.
|
||||
// Empty name + createdBy + orgID → NULL in DB so `WHERE name IS NULL`
|
||||
// works for future queries that want "unnamed" tokens.
|
||||
mock.ExpectQuery(`INSERT INTO org_api_tokens`).
|
||||
WithArgs(sqlmock.AnyArg(), sqlmock.AnyArg(), nil, nil).
|
||||
WithArgs(sqlmock.AnyArg(), sqlmock.AnyArg(), nil, nil, nil).
|
||||
WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("tok-min"))
|
||||
|
||||
_, _, err = Issue(context.Background(), db, "", "")
|
||||
_, _, err = Issue(context.Background(), db, "", "", "")
|
||||
if err != nil {
|
||||
t.Fatalf("Issue: %v", err)
|
||||
}
|
||||
@ -143,9 +143,9 @@ func TestList_NewestFirst(t *testing.T) {
|
||||
earlier := now.Add(-1 * time.Hour)
|
||||
mock.ExpectQuery(`SELECT id, prefix.*FROM org_api_tokens.*ORDER BY created_at DESC`).
|
||||
WithArgs(listMax).
|
||||
WillReturnRows(sqlmock.NewRows([]string{"id", "prefix", "name", "created_by", "created_at", "last_used_at"}).
|
||||
AddRow("t2", "abcd1234", "zapier", "user_01", now, now).
|
||||
AddRow("t1", "efgh5678", "", "", earlier, nil))
|
||||
WillReturnRows(sqlmock.NewRows([]string{"id", "prefix", "name", "org_id", "created_by", "created_at", "last_used_at"}).
|
||||
AddRow("t2", "abcd1234", "zapier", "org-1", "user_01", now, now).
|
||||
AddRow("t1", "efgh5678", "", "", "", earlier, nil))
|
||||
|
||||
tokens, err := List(context.Background(), db)
|
||||
if err != nil {
|
||||
@ -157,6 +157,9 @@ func TestList_NewestFirst(t *testing.T) {
|
||||
if tokens[0].ID != "t2" {
|
||||
t.Errorf("ordering not preserved: got %q first", tokens[0].ID)
|
||||
}
|
||||
if tokens[0].OrgID != "org-1" {
|
||||
t.Errorf("got org_id %q, want org-1", tokens[0].OrgID)
|
||||
}
|
||||
if tokens[1].LastUsedAt != nil {
|
||||
t.Errorf("never-used token should have nil LastUsedAt, got %v", tokens[1].LastUsedAt)
|
||||
}
|
||||
@ -210,3 +213,62 @@ func TestHasAnyLive(t *testing.T) {
|
||||
t.Errorf("has-any-live empty: got (%v, %v), want (false, nil)", got, err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestOrgIDByTokenID_HappyPath(t *testing.T) {
|
||||
db, mock, err := sqlmock.New()
|
||||
if err != nil {
|
||||
t.Fatalf("sqlmock: %v", err)
|
||||
}
|
||||
defer db.Close()
|
||||
|
||||
mock.ExpectQuery(`SELECT org_id FROM org_api_tokens WHERE id = \$1`).
|
||||
WithArgs("tok-org-1").
|
||||
WillReturnRows(sqlmock.NewRows([]string{"org_id"}).AddRow("org-1"))
|
||||
|
||||
orgID, err := OrgIDByTokenID(context.Background(), db, "tok-org-1")
|
||||
if err != nil {
|
||||
t.Fatalf("OrgIDByTokenID: %v", err)
|
||||
}
|
||||
if orgID != "org-1" {
|
||||
t.Errorf("orgID = %q, want org-1", orgID)
|
||||
}
|
||||
}
|
||||
|
||||
func TestOrgIDByTokenID_NullOrgIDReturnsEmpty(t *testing.T) {
|
||||
db, mock, err := sqlmock.New()
|
||||
if err != nil {
|
||||
t.Fatalf("sqlmock: %v", err)
|
||||
}
|
||||
defer db.Close()
|
||||
|
||||
// Pre-migration token or ADMIN_TOKEN bootstrap token — org_id is NULL.
|
||||
// Caller should get ("", nil) and deny by default.
|
||||
mock.ExpectQuery(`SELECT org_id FROM org_api_tokens WHERE id = \$1`).
|
||||
WithArgs("tok-old").
|
||||
WillReturnRows(sqlmock.NewRows([]string{"org_id"}).AddRow(nil))
|
||||
|
||||
orgID, err := OrgIDByTokenID(context.Background(), db, "tok-old")
|
||||
if err != nil {
|
||||
t.Fatalf("OrgIDByTokenID null: got err %v", err)
|
||||
}
|
||||
if orgID != "" {
|
||||
t.Errorf("orgID for null row = %q, want \"\"", orgID)
|
||||
}
|
||||
}
|
||||
|
||||
func TestOrgIDByTokenID_DBError(t *testing.T) {
|
||||
db, mock, err := sqlmock.New()
|
||||
if err != nil {
|
||||
t.Fatalf("sqlmock: %v", err)
|
||||
}
|
||||
defer db.Close()
|
||||
|
||||
mock.ExpectQuery(`SELECT org_id FROM org_api_tokens WHERE id = \$1`).
|
||||
WithArgs("tok-bad").
|
||||
WillReturnError(sql.ErrConnDone)
|
||||
|
||||
_, err = OrgIDByTokenID(context.Background(), db, "tok-bad")
|
||||
if err == nil {
|
||||
t.Error("expected error on DB failure, got nil")
|
||||
}
|
||||
}
|
||||
|
||||
@ -0,0 +1,7 @@
|
||||
-- Revert: drop org_id column from org_api_tokens.
|
||||
-- Note: this does NOT restore the broken requireCallerOwnsOrg behaviour
|
||||
-- (which used created_by as org anchor). That function was fixed alongside
|
||||
-- this migration to read org_id; it will error on org-token callers if this
|
||||
-- migration is reverted without also reverting the handler.
|
||||
DROP INDEX IF EXISTS org_api_tokens_org_id_idx;
|
||||
ALTER TABLE org_api_tokens DROP COLUMN IF EXISTS org_id;
|
||||
33
workspace-server/migrations/036_org_api_tokens_org_id.up.sql
Normal file
33
workspace-server/migrations/036_org_api_tokens_org_id.up.sql
Normal file
@ -0,0 +1,33 @@
|
||||
-- Add org_id column to org_api_tokens (#1200 / F1094).
|
||||
--
|
||||
-- Rationale: requireCallerOwnsOrg (org_plugin_allowlist.go:116) was
|
||||
-- reading created_by to determine the caller's org. But created_by
|
||||
-- is a provenance label ("session", "admin-token", "org-token:<prefix>"),
|
||||
-- NOT an org UUID — so every org-token caller got a non-UUID string
|
||||
-- and the equality check callerOrg != targetOrgID always failed,
|
||||
-- causing 403 on every org-token request.
|
||||
--
|
||||
-- Migration plan:
|
||||
-- 1. Add org_id (nullable) — existing tokens have no org anchor yet
|
||||
-- 2. All new tokens minted via POST /org/tokens are written with org_id
|
||||
-- 3. Backfill: tokens minted via session → look up session's org workspace
|
||||
-- (the session-auth workspace ID is known via the CP /cp/auth/tenant-member
|
||||
-- response; the CP stores org_id in the session state). This requires
|
||||
-- a separate admin script since the handler doesn't have that context.
|
||||
-- Tokens minted via ADMIN_TOKEN or bootstrap → leave org_id NULL
|
||||
-- (deny by default; operator must set via admin API if needed).
|
||||
-- 4. requireCallerOwnsOrg now reads org_id instead of created_by
|
||||
-- (org_id NULL → treat as "no anchor" → deny by default)
|
||||
-- 5. Post-Fix: admin can backfill remaining tokens via
|
||||
-- PATCH /org/tokens/:id with org_id set.
|
||||
ALTER TABLE org_api_tokens
|
||||
ADD COLUMN org_id UUID REFERENCES workspaces(id) ON DELETE SET NULL;
|
||||
|
||||
-- Tokens created before this migration cannot be backfilled automatically
|
||||
-- without knowing the session's org. Mark them as "unanchored" (org_id NULL)
|
||||
-- so the auth fix denies by default — safer than permitting all old tokens.
|
||||
-- Ops can backfill org_id for known tokens via PATCH /org/tokens/:id.
|
||||
--
|
||||
-- Index for fast org-anchor lookups (used by requireCallerOwnsOrg).
|
||||
CREATE INDEX IF NOT EXISTS org_api_tokens_org_id_idx
|
||||
ON org_api_tokens (org_id) WHERE org_id IS NOT NULL;
|
||||
Loading…
Reference in New Issue
Block a user