diff --git a/apps/admin/package.json b/apps/admin/package.json index e3be06b7..e2361a51 100644 --- a/apps/admin/package.json +++ b/apps/admin/package.json @@ -1,6 +1,6 @@ { "name": "admin", - "version": "1.4.38", + "version": "1.4.39", "private": true, "description": "Admin UI for Pace", "license": "AGPL-3.0", diff --git a/apps/api/internal/access/membership.go b/apps/api/internal/access/membership.go new file mode 100644 index 00000000..c0ce9454 --- /dev/null +++ b/apps/api/internal/access/membership.go @@ -0,0 +1,57 @@ +// Package access holds the project membership predicate every application's queries are written around, because all three of them ask the same question and the answer is an authorization decision. +// +// Two columns on project_members look like two spellings of one thing and are not. `is_active = FALSE` means this person was removed from the project: it is written synchronously, by project member removal, by workspace member removal and by account deactivation, and none of those three touches deleted_at. `deleted_at IS NOT NULL` means the project or workspace the membership belonged to was soft-deleted: it is written by one path only, the worker's cascade over the relation graph, which runs after the delete has already answered and after the parent row's own deleted_at is set. +// +// So the membership test is is_active, and it is unconditional. The membership's own soft delete is a lagging copy of a parent-liveness signal, and whether a query carries it is a question about which queryset the query is written from: Django's default manager filters the model's own queryset and never a join traversed through it, so a predicate written from ProjectMember's manager carries deleted_at IS NULL and a relation traversal from another model's queryset does not. That is why the option below is an option rather than the default — the sites that leave it out are reproducing the port's rule, and the base row's own deleted_at, stamped by the same cascade, is what excludes a cascaded row from those queries anyway. +package access + +import ( + "context" + + "gorm.io/gorm" +) + +// MemberJoin is the membership join a project-scoped queryset carries: the caller's own active membership of the project the base row belongs to. It is an inner join, so a row whose reader is not a member of its project does not survive it. +// +// alias and projectColumn say where the project id is read from, which is not always .project_id — a query over projects joins on p.id, and a page reaches its project through the link table, so it joins on pp.project_id. The member id stays a bind argument of the call site, which reads Joins(access.MemberJoin("i", "project_id"), user.ID). +func MemberJoin(alias, projectColumn string, options ...Option) string { + var settings joinSettings + for _, option := range options { + option(&settings) + } + join := "JOIN project_members pm ON pm.project_id = " + alias + "." + projectColumn + + " AND pm.member_id = ? AND pm.is_active = TRUE" + if settings.membershipSoftDelete { + join += " AND pm.deleted_at IS NULL" + } + return join +} + +// Option narrows what MemberJoin writes. There is one, and the reason it exists is in this package's doc comment. +type Option func(*joinSettings) + +// joinSettings is unexported so that the only options are the ones named here. +type joinSettings struct { + membershipSoftDelete bool +} + +// WithMembershipSoftDelete adds the membership row's own soft-delete check, for the queries whose predicate is written from project_members' own manager rather than traversed into from another model's queryset. It narrows nothing the parent's deleted_at does not narrow first, and it narrows it later, because the cascade that writes the one has already written the other. +func WithMembershipSoftDelete() Option { + return func(settings *joinSettings) { settings.membershipSoftDelete = true } +} + +// ProjectRole reads one person's role in one project and reports whether they are an active member of it at all. It is the permission gate itself rather than a predicate joined into a larger query — the membership is the whole subject — so it filters the membership's own soft delete, as MemberJoin does only when asked to. +// +// It answers with the role rather than a count of rows matching a role, which the callers that used to count rely on: project_members carries a unique index on (project_id, member_id) where deleted_at is null (migrate/testdata/schema.tsv:2697), and the workspace is joined on its primary key, so at most one row can match and "no row with this role" and "the one row has another role" are the same answer. +func ProjectRole(ctx context.Context, db *gorm.DB, slug, projectID, memberID string) (int, bool, error) { + var roles []int + err := db.WithContext(ctx).Table("project_members pm"). + Joins("JOIN workspaces w ON w.id = pm.workspace_id"). + Where("w.slug = ? AND pm.project_id = ? AND pm.member_id = ? AND pm.is_active = TRUE AND pm.deleted_at IS NULL", + slug, projectID, memberID). + Limit(1).Pluck("pm.role", &roles).Error + if err != nil || len(roles) == 0 { + return 0, false, err + } + return roles[0], true, nil +} diff --git a/apps/api/internal/access/membership_test.go b/apps/api/internal/access/membership_test.go new file mode 100644 index 00000000..3cf4ad66 --- /dev/null +++ b/apps/api/internal/access/membership_test.go @@ -0,0 +1,52 @@ +package access + +import ( + "strings" + "testing" +) + +// The expected strings here are the literals the call sites carried before they were replaced, copied out of them character for character. That is the point of the test: the join this package writes has to be the same SQL those queries have always sent, for each of the three shapes of project column the tree uses. +func TestTheMembershipJoinIsWhatTheCallSitesWrote(t *testing.T) { + for _, test := range []struct { + name string + got string + want string + }{ + { + name: "the base row's project", + got: MemberJoin("i", "project_id"), + want: "JOIN project_members pm ON pm.project_id = i.project_id AND pm.member_id = ? AND pm.is_active = TRUE", + }, + { + name: "a query over projects themselves", + got: MemberJoin("p", "id"), + want: "JOIN project_members pm ON pm.project_id = p.id AND pm.member_id = ? AND pm.is_active = TRUE", + }, + { + name: "a page, which reaches its project through the link table", + got: MemberJoin("pp", "project_id"), + want: "JOIN project_members pm ON pm.project_id = pp.project_id AND pm.member_id = ? AND pm.is_active = TRUE", + }, + { + name: "the membership's own soft delete, where the queryset is project_members'", + got: MemberJoin("l", "project_id", WithMembershipSoftDelete()), + want: "JOIN project_members pm ON pm.project_id = l.project_id AND pm.member_id = ? AND pm.is_active = TRUE AND pm.deleted_at IS NULL", + }, + } { + t.Run(test.name, func(t *testing.T) { + if test.got != test.want { + t.Errorf("the join renders\n %s\nwant\n %s", test.got, test.want) + } + }) + } +} + +// The option is the only thing that adds the membership's soft delete, so a site that does not pass it cannot acquire the filter by accident. +func TestTheMembershipSoftDeleteIsOptIn(t *testing.T) { + if got := MemberJoin("i", "project_id"); strings.Contains(got, "pm.deleted_at") { + t.Errorf("the plain join filters the membership's soft delete:\n %s", got) + } + if got := MemberJoin("i", "project_id", WithMembershipSoftDelete()); !strings.Contains(got, "AND pm.deleted_at IS NULL") { + t.Errorf("the option did not add the membership's soft delete:\n %s", got) + } +} diff --git a/apps/api/internal/externalapi/cycle_crud_handler.go b/apps/api/internal/externalapi/cycle_crud_handler.go index 4ee5e65c..f14c898d 100644 --- a/apps/api/internal/externalapi/cycle_crud_handler.go +++ b/apps/api/internal/externalapi/cycle_crud_handler.go @@ -6,6 +6,7 @@ import ( "time" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/cycles" "github.com/yldm-tech/pace/apps/api/internal/drf" @@ -102,7 +103,7 @@ func (handler *Handler) cycleRetrieve(c *gin.Context, user *auth.User, _ APIToke func (handler *Handler) cycleScope(c *gin.Context, user *auth.User) *gorm.DB { return handler.db.WithContext(c.Request.Context()).Table("cycles c"). Joins("JOIN workspaces w ON w.id = c.workspace_id"). - Joins("JOIN project_members pm ON pm.project_id = c.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("c", "project_id"), user.ID). Joins("LEFT JOIN cycle_issues ci ON ci.cycle_id = c.id"). Joins("LEFT JOIN issues ii ON ii.id = ci.issue_id"). Where("w.slug = ? AND c.project_id = ? AND c.deleted_at IS NULL", c.Param("slug"), c.Param("project")) diff --git a/apps/api/internal/externalapi/cycle_handler.go b/apps/api/internal/externalapi/cycle_handler.go index ca9c5667..c3e70e2e 100644 --- a/apps/api/internal/externalapi/cycle_handler.go +++ b/apps/api/internal/externalapi/cycle_handler.go @@ -5,6 +5,7 @@ import ( "time" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "gorm.io/gorm" ) @@ -98,7 +99,7 @@ func (handler *Handler) archivedCycleList(c *gin.Context, user *auth.User, _ API err := handler.db.WithContext(c.Request.Context()).Table("cycles c"). Select(externalCycleAnnotations()). Joins("JOIN workspaces w ON w.id = c.workspace_id"). - Joins("JOIN project_members pm ON pm.project_id = c.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("c", "project_id"), user.ID). Joins("LEFT JOIN cycle_issues ci ON ci.cycle_id = c.id"). Joins("LEFT JOIN issues ii ON ii.id = ci.issue_id"). Joins("LEFT JOIN estimate_points ep ON ep.id = ii.estimate_point_id"). diff --git a/apps/api/internal/externalapi/cycle_issue_handler.go b/apps/api/internal/externalapi/cycle_issue_handler.go index 60f2b791..6dc65a7b 100644 --- a/apps/api/internal/externalapi/cycle_issue_handler.go +++ b/apps/api/internal/externalapi/cycle_issue_handler.go @@ -6,6 +6,7 @@ import ( "time" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "gorm.io/gorm" @@ -285,7 +286,7 @@ func (handler *Handler) cycleIssueLinks(c *gin.Context, user *auth.User) ([]cycl err := handler.db.WithContext(c.Request.Context()).Table("cycle_issues ci"). Select(cycleIssueSelection()). Joins("JOIN workspaces w ON w.id = ci.workspace_id"). - Joins("JOIN project_members pm ON pm.project_id = ci.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("ci", "project_id"), user.ID). Where("w.slug = ? AND ci.project_id = ? AND ci.cycle_id = ? AND ci.deleted_at IS NULL", c.Param("slug"), c.Param("project"), c.Param("cycle")). Order("ci.created_at DESC").Scan(&rows).Error diff --git a/apps/api/internal/externalapi/intake_handler.go b/apps/api/internal/externalapi/intake_handler.go index 0a35b6a0..6af17581 100644 --- a/apps/api/internal/externalapi/intake_handler.go +++ b/apps/api/internal/externalapi/intake_handler.go @@ -6,6 +6,7 @@ import ( "time" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "github.com/yldm-tech/pace/apps/api/internal/htmlsanitizer" @@ -490,17 +491,9 @@ func (handler *Handler) requireProjectLite(c *gin.Context, user *auth.User) bool return true } +// projectRole reads the caller's role in the project named in the url, and reports whether they are a member of it at all. It is what ProjectLitePermission asks, and what requireProjectBase asks for a write that is neither a create nor a safe method. func (handler *Handler) projectRole(c *gin.Context, user *auth.User) (int, bool, error) { - var roles []int - err := handler.db.WithContext(c.Request.Context()).Table("project_members pm"). - Joins("JOIN workspaces w ON w.id = pm.workspace_id"). - Where("w.slug = ? AND pm.project_id = ? AND pm.member_id = ? AND pm.is_active = TRUE", - c.Param("slug"), c.Param("project"), user.ID). - Limit(1).Pluck("pm.role", &roles).Error - if err != nil || len(roles) == 0 { - return 0, false, err - } - return roles[0], true, nil + return access.ProjectRole(c.Request.Context(), handler.db, c.Param("slug"), c.Param("project"), user.ID) } // projectIntakeIfEnabled reports the project's intake and whether the list should read it at all. diff --git a/apps/api/internal/externalapi/issue_activity_handler.go b/apps/api/internal/externalapi/issue_activity_handler.go index 9e7b3602..0a69b3d4 100644 --- a/apps/api/internal/externalapi/issue_activity_handler.go +++ b/apps/api/internal/externalapi/issue_activity_handler.go @@ -5,6 +5,7 @@ import ( "time" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "gorm.io/gorm" @@ -117,7 +118,7 @@ func (handler *Handler) issueActivityScope(c *gin.Context, user *auth.User) *gor return handler.db.WithContext(c.Request.Context()).Table("issue_activities a").Select("a.*"). Joins("JOIN workspaces w ON w.id = a.workspace_id"). Joins("JOIN projects p ON p.id = a.project_id AND p.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = a.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("a", "project_id"), user.ID). Where("w.slug = ? AND a.project_id = ? AND a.issue_id = ? AND a.deleted_at IS NULL", c.Param("slug"), c.Param("project"), c.Param("issue")). // The exclusion is a NOT IN, so an entry whose field is null survives it: a null is not in any list. diff --git a/apps/api/internal/externalapi/issue_comment_handler.go b/apps/api/internal/externalapi/issue_comment_handler.go index 1d14a0bf..ad3e85c5 100644 --- a/apps/api/internal/externalapi/issue_comment_handler.go +++ b/apps/api/internal/externalapi/issue_comment_handler.go @@ -6,6 +6,7 @@ import ( "time" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "github.com/yldm-tech/pace/apps/api/internal/htmlsanitizer" @@ -346,7 +347,7 @@ func (handler *Handler) issueCommentScope(c *gin.Context, user *auth.User) *gorm c.Param("project"), user.ID, c.Param("slug")). Joins("JOIN workspaces w ON w.id = cm.workspace_id"). Joins("JOIN projects p ON p.id = cm.project_id AND p.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = cm.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("cm", "project_id"), user.ID). Where("w.slug = ? AND cm.project_id = ? AND cm.issue_id = ? AND cm.deleted_at IS NULL", c.Param("slug"), c.Param("project"), c.Param("issue")) } diff --git a/apps/api/internal/externalapi/issue_link_handler.go b/apps/api/internal/externalapi/issue_link_handler.go index 7a790c3a..84e23bc7 100644 --- a/apps/api/internal/externalapi/issue_link_handler.go +++ b/apps/api/internal/externalapi/issue_link_handler.go @@ -8,6 +8,7 @@ import ( "time" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "gorm.io/gorm" @@ -261,7 +262,7 @@ func (handler *Handler) issueLinkScope(c *gin.Context, user *auth.User) *gorm.DB return handler.db.WithContext(c.Request.Context()).Table("issue_links l").Select("l.*"). Joins("JOIN workspaces w ON w.id = l.workspace_id"). Joins("JOIN projects p ON p.id = l.project_id AND p.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = l.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("l", "project_id"), user.ID). Where("w.slug = ? AND l.project_id = ? AND l.issue_id = ? AND l.deleted_at IS NULL", c.Param("slug"), c.Param("project"), c.Param("issue")) } diff --git a/apps/api/internal/externalapi/issue_search_handler.go b/apps/api/internal/externalapi/issue_search_handler.go index aa18e1aa..02c0128b 100644 --- a/apps/api/internal/externalapi/issue_search_handler.go +++ b/apps/api/internal/externalapi/issue_search_handler.go @@ -9,6 +9,7 @@ import ( "github.com/gin-gonic/gin" "github.com/lib/pq" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" ) @@ -56,7 +57,7 @@ func (handler *Handler) externalIssueSearch(c *gin.Context, user *auth.User, _ A query := handler.db.WithContext(c.Request.Context()).Table("issues i"). Joins("JOIN workspaces w ON w.id = i.workspace_id"). Joins("JOIN projects p ON p.id = i.project_id AND p.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = i.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("i", "project_id"), user.ID). Joins("LEFT JOIN states s ON s.id = i.state_id"). Where("w.slug = ?", c.Param("slug")). Where(`i.deleted_at IS NULL AND i.archived_at IS NULL AND i.is_draft = FALSE diff --git a/apps/api/internal/externalapi/label_handler.go b/apps/api/internal/externalapi/label_handler.go index dec6c7d1..64912316 100644 --- a/apps/api/internal/externalapi/label_handler.go +++ b/apps/api/internal/externalapi/label_handler.go @@ -6,6 +6,7 @@ import ( "time" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "gorm.io/gorm" @@ -236,7 +237,7 @@ func (handler *Handler) labelScope(c *gin.Context, user *auth.User) *gorm.DB { return handler.db.WithContext(c.Request.Context()).Table("labels l").Select("l.*"). Joins("JOIN workspaces w ON w.id = l.workspace_id"). Joins("JOIN projects p ON p.id = l.project_id AND p.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = l.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("l", "project_id"), user.ID). Where("w.slug = ? AND l.project_id = ? AND l.deleted_at IS NULL", c.Param("slug"), c.Param("project")) } diff --git a/apps/api/internal/externalapi/member_handler.go b/apps/api/internal/externalapi/member_handler.go index 968b6678..afa4a72d 100644 --- a/apps/api/internal/externalapi/member_handler.go +++ b/apps/api/internal/externalapi/member_handler.go @@ -4,6 +4,7 @@ import ( "net/http" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "gorm.io/gorm" @@ -353,17 +354,12 @@ func validMemberRole(role int) bool { // requireProjectAdmin is ProjectAdminPermission, which the three write routes swap in for the read one. func (handler *Handler) requireProjectAdmin(c *gin.Context, user *auth.User) bool { - var admins int64 - err := handler.db.WithContext(c.Request.Context()).Table("project_members pm"). - Joins("JOIN workspaces w ON w.id = pm.workspace_id"). - Where("w.slug = ? AND pm.project_id = ? AND pm.member_id = ? AND pm.role = ? AND pm.is_active = TRUE", - c.Param("slug"), c.Param("project"), user.ID, roleAdmin). - Count(&admins).Error + role, member, err := access.ProjectRole(c.Request.Context(), handler.db, c.Param("slug"), c.Param("project"), user.ID) if err != nil { handler.serverError(c, err) return false } - if admins == 0 { + if !member || role != roleAdmin { c.AbortWithStatusJSON(http.StatusForbidden, gin.H{ "detail": "You do not have permission to perform this action.", }) diff --git a/apps/api/internal/externalapi/membership_predicate_test.go b/apps/api/internal/externalapi/membership_predicate_test.go new file mode 100644 index 00000000..c204b486 --- /dev/null +++ b/apps/api/internal/externalapi/membership_predicate_test.go @@ -0,0 +1,101 @@ +package externalapi + +import ( + "net/http" + "net/http/httptest" + "regexp" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/auth" + "gorm.io/driver/postgres" + "gorm.io/gorm" +) + +// These tests pin the membership join every external-API queryset carries. The join is the whole gate on these routes — the API-key middleware at auth.go checks the key and the user, not the project — and it lives in string literals nothing else reads, so this renders the statement and reads the fragment back out of it. +// +// As in the session API, the membership row's own soft delete is deliberately absent: it is set only by the worker cascade when the parent project or workspace is soft-deleted, and a join traversed through another model's queryset does not apply the joined model's manager. The base row's `deleted_at IS NULL` in the same statement is what excludes a cascaded row, and it is stamped by the same cascade, so the filter left out here would change no row. + +// placeholderNumbers matches the bind markers the postgres driver numbers, which are put back as question marks so a fragment reads the way the call site wrote it. +var placeholderNumbers = regexp.MustCompile(`\$\d+`) + +// dryRunHandler is a Handler whose queries render to SQL instead of reaching a database. DisableAutomaticPing is what keeps gorm.Open from dialling the DSN, which is never connected to. +func dryRunHandler(t *testing.T) *Handler { + t.Helper() + database, err := gorm.Open( + postgres.New(postgres.Config{DSN: "postgres://render@127.0.0.1:1/render"}), + &gorm.Config{DryRun: true, DisableAutomaticPing: true}, + ) + if err != nil { + t.Fatalf("opening a dry-run database: %v", err) + } + return &Handler{db: database} +} + +// renderedSQL is the statement a scope becomes. +func renderedSQL(t *testing.T, scope *gorm.DB) string { + t.Helper() + var rows []map[string]any + statement := scope.Find(&rows).Statement + if statement.SQL.Len() == 0 { + t.Fatal("the scope rendered no SQL") + } + return placeholderNumbers.ReplaceAllString(statement.SQL.String(), "?") +} + +// dryRunContext is a request the scope builders can read their route parameters off. +func dryRunContext(t *testing.T) *gin.Context { + t.Helper() + context, _ := gin.CreateTestContext(httptest.NewRecorder()) + context.Request = httptest.NewRequest(http.MethodGet, "/", nil) + context.Params = gin.Params{ + {Key: "slug", Value: "acme"}, + {Key: "project", Value: "00000000-0000-0000-0000-0000000000p1"}, + {Key: "issue", Value: "00000000-0000-0000-0000-0000000000i1"}, + } + return context +} + +// Every queryset the external API reads through joins the caller's membership of the project in the url, on its own base table's project column. +func TestTheExternalScopesJoinTheCallersMembership(t *testing.T) { + handler := dryRunHandler(t) + context := dryRunContext(t) + user := &auth.User{ID: "00000000-0000-0000-0000-0000000000u1"} + + for _, test := range []struct { + name string + scope *gorm.DB + want string + }{ + {"stateScope", handler.stateScope(context, user), + "JOIN project_members pm ON pm.project_id = s.project_id AND pm.member_id = ? AND pm.is_active = TRUE"}, + {"labelScope", handler.labelScope(context, user), + "JOIN project_members pm ON pm.project_id = l.project_id AND pm.member_id = ? AND pm.is_active = TRUE"}, + {"issueLinkScope", handler.issueLinkScope(context, user), + "JOIN project_members pm ON pm.project_id = l.project_id AND pm.member_id = ? AND pm.is_active = TRUE"}, + {"issueActivityScope", handler.issueActivityScope(context, user), + "JOIN project_members pm ON pm.project_id = a.project_id AND pm.member_id = ? AND pm.is_active = TRUE"}, + {"issueCommentScope", handler.issueCommentScope(context, user), + "JOIN project_members pm ON pm.project_id = cm.project_id AND pm.member_id = ? AND pm.is_active = TRUE"}, + {"cycleScope", handler.cycleScope(context, user), + "JOIN project_members pm ON pm.project_id = c.project_id AND pm.member_id = ? AND pm.is_active = TRUE"}, + } { + t.Run(test.name, func(t *testing.T) { + rendered := renderedSQL(t, test.scope) + if !strings.Contains(rendered, test.want) { + t.Errorf("%s does not carry\n %s\nit renders\n %s", test.name, test.want, rendered) + } + if strings.Contains(rendered, "pm.deleted_at") { + t.Errorf("%s checks the membership's soft delete, and Django's join does not", test.name) + } + if strings.Contains(rendered, "LEFT JOIN project_members") { + t.Errorf("%s joins the membership outer, which lets a non-member's row through", test.name) + } + // Whatever a cascaded membership would have hidden, the base row's own soft delete hides first: the two are stamped by the same cascade. + if !strings.Contains(rendered, "deleted_at IS NULL") { + t.Errorf("%s filters no soft-deleted row at all:\n %s", test.name, rendered) + } + }) + } +} diff --git a/apps/api/internal/externalapi/module_issue_handler.go b/apps/api/internal/externalapi/module_issue_handler.go index c2af23fd..293a42f8 100644 --- a/apps/api/internal/externalapi/module_issue_handler.go +++ b/apps/api/internal/externalapi/module_issue_handler.go @@ -7,6 +7,7 @@ import ( "time" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "gorm.io/gorm" @@ -251,7 +252,7 @@ func (handler *Handler) moduleIssueLinks(c *gin.Context, user *auth.User) ([]mod Select(moduleIssueSelection()). Joins("JOIN workspaces w ON w.id = mi.workspace_id"). Joins("JOIN projects p ON p.id = mi.project_id AND p.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = mi.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("mi", "project_id"), user.ID). Where("w.slug = ? AND mi.project_id = ? AND mi.module_id = ? AND mi.deleted_at IS NULL", c.Param("slug"), c.Param("project"), c.Param("module")). Order("mi.created_at DESC").Scan(&rows).Error diff --git a/apps/api/internal/externalapi/permissions.go b/apps/api/internal/externalapi/permissions.go index 0bd73e9b..c242f821 100644 --- a/apps/api/internal/externalapi/permissions.go +++ b/apps/api/internal/externalapi/permissions.go @@ -7,6 +7,7 @@ import ( "github.com/gin-gonic/gin" "github.com/google/uuid" "github.com/jackc/pgx/v5/pgconn" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "gorm.io/gorm/clause" @@ -21,17 +22,12 @@ const ( // requireProjectMember is ProjectEntityPermission: a safe method wants any active member, and a write wants an admin or a member. func (handler *Handler) requireProjectMember(c *gin.Context, user *auth.User, method string) bool { - var roles []int - err := handler.db.WithContext(c.Request.Context()).Table("project_members pm"). - Joins("JOIN workspaces w ON w.id = pm.workspace_id"). - Where("w.slug = ? AND pm.project_id = ? AND pm.member_id = ? AND pm.is_active = TRUE", - c.Param("slug"), c.Param("project"), user.ID). - Limit(1).Pluck("pm.role", &roles).Error + role, member, err := access.ProjectRole(c.Request.Context(), handler.db, c.Param("slug"), c.Param("project"), user.ID) if err != nil { handler.serverError(c, err) return false } - if len(roles) == 0 { + if !member { c.AbortWithStatusJSON(http.StatusForbidden, gin.H{ "detail": "You do not have permission to perform this action.", }) @@ -40,7 +36,7 @@ func (handler *Handler) requireProjectMember(c *gin.Context, user *auth.User, me if method == http.MethodGet || method == http.MethodHead || method == http.MethodOptions { return true } - if roles[0] == roleAdmin || roles[0] == roleMember { + if role == roleAdmin || role == roleMember { return true } c.AbortWithStatusJSON(http.StatusForbidden, gin.H{ diff --git a/apps/api/internal/externalapi/permissions_test.go b/apps/api/internal/externalapi/permissions_test.go new file mode 100644 index 00000000..fa4ed907 --- /dev/null +++ b/apps/api/internal/externalapi/permissions_test.go @@ -0,0 +1,147 @@ +package externalapi + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/auth" + "gorm.io/driver/postgres" + "gorm.io/gorm" + "gorm.io/gorm/logger" +) + +// The external API's permission gates are the whole of its authorization: the API-key middleware in auth.go authenticates the key and the user and says nothing about the workspace or the project, so whatever the gate's query does not filter, the route does not filter either. There is no database-backed test in this package to check them against, and these gates are not pure functions, so what these tests do is run the gate against a session that records the statement instead of sending it, and read the predicate back out of the record. +// +// That gives two assertions per gate, and both matter. The recorded SQL says which rows the gate would have accepted. The gate's own answer, against a session where no query matches anything, says which way it fails when it finds nothing — and the answer has to be no. + +// recordedStatements is a gorm logger that keeps the SQL a session would have sent. Under DryRun nothing is sent, and the trace still carries the statement with its bind values inlined. +type recordedStatements struct { + statements []string +} + +func (recorded *recordedStatements) LogMode(logger.LogLevel) logger.Interface { return recorded } + +func (recorded *recordedStatements) Info(context.Context, string, ...any) {} + +func (recorded *recordedStatements) Warn(context.Context, string, ...any) {} + +func (recorded *recordedStatements) Error(context.Context, string, ...any) {} + +func (recorded *recordedStatements) Trace(_ context.Context, _ time.Time, sql func() (string, int64), _ error) { + statement, _ := sql() + recorded.statements = append(recorded.statements, statement) +} + +// contains reports whether any recorded statement carries the fragment. +func (recorded *recordedStatements) contains(fragment string) bool { + for _, statement := range recorded.statements { + if strings.Contains(statement, fragment) { + return true + } + } + return false +} + +func (recorded *recordedStatements) String() string { + return strings.Join(recorded.statements, "\n ") +} + +// recordingHandler is a Handler whose queries are built and recorded rather than run. DisableAutomaticPing is what keeps gorm.Open from dialling the DSN, which is never connected to. +func recordingHandler(t *testing.T) (*Handler, *recordedStatements) { + t.Helper() + recorded := &recordedStatements{} + database, err := gorm.Open( + postgres.New(postgres.Config{DSN: "postgres://render@127.0.0.1:1/render"}), + &gorm.Config{DryRun: true, DisableAutomaticPing: true, Logger: recorded}, + ) + if err != nil { + t.Fatalf("opening a dry-run database: %v", err) + } + return &Handler{db: database}, recorded +} + +// gateRequest is a request carrying the two route parameters every project gate reads. +func gateRequest(t *testing.T, method string) (*gin.Context, *httptest.ResponseRecorder) { + t.Helper() + recorder := httptest.NewRecorder() + context, _ := gin.CreateTestContext(recorder) + context.Request = httptest.NewRequest(method, "/", nil) + context.Params = gin.Params{ + {Key: "slug", Value: "acme"}, + {Key: "project", Value: "00000000-0000-0000-0000-0000000000p1"}, + } + return context, recorder +} + +// gateUser is the caller every gate below is asked about. +func gateUser() *auth.User { + return &auth.User{ID: "00000000-0000-0000-0000-0000000000u1"} +} + +// ProjectBasePermission reads the caller's workspace role, and a soft-deleted workspace has no members to read: the workspace's own deleted_at is written in the same transaction as the delete, so this is the filter that closes the window rather than narrowing it eventually. +// +// This gate used to filter neither the workspace's soft delete nor the membership's, alone among the five copies of the lookup in the tree, and it is the one that admits the external API's project list, create, update and delete. +func TestTheProjectBaseGateWillNotReadASoftDeletedWorkspace(t *testing.T) { + handler, recorded := recordingHandler(t) + context, recorder := gateRequest(t, http.MethodGet) + + if handler.requireProjectBase(context, gateUser(), http.MethodGet) { + t.Error("the gate admitted a caller whose membership lookup matched nothing") + } + if recorder.Code != http.StatusForbidden { + t.Errorf("the gate answered %d, want %d", recorder.Code, http.StatusForbidden) + } + for _, fragment := range []string{ + "w.deleted_at IS NULL", + "wm.is_active = TRUE", + "wm.deleted_at IS NULL", + } { + if !recorded.contains(fragment) { + t.Errorf("the workspace role lookup does not filter %s:\n %s", fragment, recorded) + } + } +} + +// The three gates over project_members are the only queries in this app whose whole subject is the membership: they read it through its own manager and carry no other liveness check, so the membership's own soft delete is the one thing standing between a soft-deleted project and a caller who used to be in it. Every other project-scoped query here traverses the membership as a join from a base row it already filters, and leaves the filter out to match Django. +// +// This takes effect when the worker drains the cascade that stamps the membership, not when the project is deleted. The gates do not test the project row itself, so the window between the two stays open; closing it takes a join onto projects, which is a departure from the predicate Django writes for these permissions and is deliberately not made here. +func TestTheProjectGatesRefuseACascadedMembership(t *testing.T) { + for _, test := range []struct { + name string + method string + gate func(*Handler, *gin.Context) bool + }{ + {"requireProjectMember", http.MethodGet, func(handler *Handler, context *gin.Context) bool { + return handler.requireProjectMember(context, gateUser(), http.MethodGet) + }}, + {"requireProjectAdmin", http.MethodPost, func(handler *Handler, context *gin.Context) bool { + return handler.requireProjectAdmin(context, gateUser()) + }}, + {"projectRole", http.MethodPatch, func(handler *Handler, context *gin.Context) bool { + _, found, err := handler.projectRole(context, gateUser()) + if err != nil { + t.Fatalf("the role lookup failed: %v", err) + } + return found + }}, + } { + t.Run(test.name, func(t *testing.T) { + handler, recorded := recordingHandler(t) + context, _ := gateRequest(t, test.method) + + if test.gate(handler, context) { + t.Error("the gate admitted a caller whose membership lookup matched nothing") + } + for _, fragment := range []string{"pm.is_active = TRUE", "pm.deleted_at IS NULL"} { + if !recorded.contains(fragment) { + t.Errorf("the gate does not filter %s:\n %s", fragment, recorded) + } + } + }) + } +} diff --git a/apps/api/internal/externalapi/project_detail_handler.go b/apps/api/internal/externalapi/project_detail_handler.go index e893712e..473ca74c 100644 --- a/apps/api/internal/externalapi/project_detail_handler.go +++ b/apps/api/internal/externalapi/project_detail_handler.go @@ -324,11 +324,14 @@ func (handler *Handler) requireProjectBase(c *gin.Context, user *auth.User, meth return false } +// workspaceRole reads the caller's role in the workspace, and reports whether they are a member of it at all. +// +// The predicate is internal/workspace's, which is the strictest of the copies in the tree, and this one used to be the loosest: it filtered neither the workspace's own soft delete nor the membership's. A soft-deleted workspace is renamed to `__` in the same transaction that stamps its deleted_at, so reaching one takes knowing the mangled slug — but it was reachable, and this lookup is the whole of ProjectBasePermission, which gates the project list, create, update and delete. The workspace filter is the one that matters: deleted_at on the workspace is written synchronously, while the membership's arrives later with the worker's cascade. func (handler *Handler) workspaceRole(c *gin.Context, user *auth.User, slug string) (int, bool, error) { var roles []int err := handler.db.WithContext(c.Request.Context()).Table("workspace_members wm"). Joins("JOIN workspaces w ON w.id = wm.workspace_id"). - Where("w.slug = ? AND wm.member_id = ? AND wm.is_active = TRUE", slug, user.ID). + Where("w.slug = ? AND w.deleted_at IS NULL AND wm.member_id = ? AND wm.is_active = TRUE AND wm.deleted_at IS NULL", slug, user.ID). Limit(1).Pluck("wm.role", &roles).Error if err != nil || len(roles) == 0 { return 0, false, err diff --git a/apps/api/internal/externalapi/state_handler.go b/apps/api/internal/externalapi/state_handler.go index 6ad0ce38..ad813387 100644 --- a/apps/api/internal/externalapi/state_handler.go +++ b/apps/api/internal/externalapi/state_handler.go @@ -7,6 +7,7 @@ import ( "time" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "github.com/yldm-tech/pace/apps/api/internal/pagination" @@ -325,7 +326,7 @@ func (handler *Handler) stateScope(c *gin.Context, user *auth.User) *gorm.DB { return handler.db.WithContext(c.Request.Context()).Table("states s").Select("s.*"). Joins("JOIN workspaces w ON w.id = s.workspace_id"). Joins("JOIN projects p ON p.id = s.project_id AND p.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = s.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("s", "project_id"), user.ID). Where("w.slug = ? AND s.project_id = ? AND s.is_triage = FALSE AND s.deleted_at IS NULL", c.Param("slug"), c.Param("project")) } diff --git a/apps/api/internal/project/archived_cycle_detail_handler.go b/apps/api/internal/project/archived_cycle_detail_handler.go index c2d833e0..28131a23 100644 --- a/apps/api/internal/project/archived_cycle_detail_handler.go +++ b/apps/api/internal/project/archived_cycle_detail_handler.go @@ -6,6 +6,7 @@ import ( "time" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" ) @@ -105,7 +106,7 @@ func (handler *Handler) archivedCycleRowByID(c *gin.Context, slug, projectID, us Select(selection, userID, projectID, slug, now, now, now, now, projectID). Joins("JOIN workspaces w ON w.id = c.workspace_id"). Joins("JOIN projects p ON p.id = c.project_id AND p.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = c.project_id AND pm.member_id = ? AND pm.is_active = TRUE", userID). + Joins(access.MemberJoin("c", "project_id"), userID). Where("w.slug = ? AND c.project_id = ? AND c.id = ? AND c.deleted_at IS NULL AND c.archived_at IS NOT NULL", slug, projectID, cycleID). Group("c.id").Limit(1).Scan(&rows).Error diff --git a/apps/api/internal/project/archived_cycle_handler.go b/apps/api/internal/project/archived_cycle_handler.go index e1b682e1..40cf6b51 100644 --- a/apps/api/internal/project/archived_cycle_handler.go +++ b/apps/api/internal/project/archived_cycle_handler.go @@ -4,6 +4,7 @@ import ( "net/http" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" ) @@ -27,7 +28,7 @@ func (handler *Handler) archivedCycleList(c *gin.Context, user *auth.User) { Select(archivedCycleAnnotations(), user.ID, projectID, slug, now, now, now, now). Joins("JOIN workspaces w ON w.id = c.workspace_id"). Joins("JOIN projects p ON p.id = c.project_id AND p.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = c.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("c", "project_id"), user.ID). Where("w.slug = ? AND c.project_id = ? AND c.deleted_at IS NULL AND c.archived_at IS NOT NULL", slug, projectID). Group("c.id").Order("is_favorite DESC, c.created_at DESC").Scan(&rows).Error if err != nil { diff --git a/apps/api/internal/project/cycle_list_handler.go b/apps/api/internal/project/cycle_list_handler.go index dc4f9719..bafe600e 100644 --- a/apps/api/internal/project/cycle_list_handler.go +++ b/apps/api/internal/project/cycle_list_handler.go @@ -7,6 +7,7 @@ import ( "github.com/gin-gonic/gin" "github.com/lib/pq" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "gorm.io/gorm" @@ -89,7 +90,7 @@ func (handler *Handler) cycleRows(c *gin.Context, slug, projectID, userID string Select(cycleAnnotations(), userID, projectID, slug, now, now, now, now). Joins("JOIN workspaces w ON w.id = c.workspace_id"). Joins("JOIN projects p ON p.id = c.project_id AND p.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = c.project_id AND pm.member_id = ? AND pm.is_active = TRUE", userID). + Joins(access.MemberJoin("c", "project_id"), userID). Where("w.slug = ? AND c.project_id = ? AND c.deleted_at IS NULL AND c.archived_at IS NULL", slug, projectID) if currentOnly { query = query.Where("c.start_date <= ? AND c.end_date >= ?", now, now) @@ -116,7 +117,7 @@ func (handler *Handler) cycleRowsByID(c *gin.Context, slug, projectID, userID, c Select(selection, arguments...). Joins("JOIN workspaces w ON w.id = c.workspace_id"). Joins("JOIN projects p ON p.id = c.project_id AND p.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = c.project_id AND pm.member_id = ? AND pm.is_active = TRUE", userID). + Joins(access.MemberJoin("c", "project_id"), userID). Where("w.slug = ? AND c.project_id = ? AND c.id = ? AND c.deleted_at IS NULL", slug, projectID, cycleID). Group("c.id").Limit(1).Scan(&rows).Error return rows, err diff --git a/apps/api/internal/project/entity_search_handler.go b/apps/api/internal/project/entity_search_handler.go index 67163321..bff04dc2 100644 --- a/apps/api/internal/project/entity_search_handler.go +++ b/apps/api/internal/project/entity_search_handler.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "gorm.io/gorm" @@ -200,7 +201,7 @@ func (handler *Handler) memberIssueScopeAnyProject(c *gin.Context, user *auth.Us return handler.db.WithContext(c.Request.Context()).Table("issues i"). Joins("JOIN workspaces w ON w.id = i.workspace_id"). Joins("JOIN projects p ON p.id = i.project_id"). - Joins("JOIN project_members pm ON pm.project_id = i.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("i", "project_id"), user.ID). Where("w.slug = ?", slug) } @@ -276,7 +277,7 @@ func (handler *Handler) projectScopedTable(c *gin.Context, user *auth.User, tabl database := handler.db.WithContext(c.Request.Context()).Table(table+" t"). Joins("JOIN workspaces w ON w.id = t.workspace_id"). Joins("JOIN projects p ON p.id = t.project_id"). - Joins("JOIN project_members pm ON pm.project_id = t.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("t", "project_id"), user.ID). Where("w.slug = ? AND t.deleted_at IS NULL", slug) if projectID != "" { database = database.Where("t.project_id = ?", projectID) @@ -290,7 +291,7 @@ func (handler *Handler) searchMentionablePages(c *gin.Context, user *auth.User, Joins("JOIN workspaces w ON w.id = pg.workspace_id"). Joins("JOIN project_pages pp ON pp.page_id = pg.id"). Joins("JOIN projects p ON p.id = pp.project_id"). - Joins("JOIN project_members pm ON pm.project_id = pp.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("pp", "project_id"), user.ID). Where("w.slug = ? AND pg.deleted_at IS NULL AND pg.access = ?", slug, pagePublicAccess) if projectID != "" { database = database.Where("pp.project_id = ?", projectID) diff --git a/apps/api/internal/project/exporter_handler.go b/apps/api/internal/project/exporter_handler.go index e8d88f8e..c06a54de 100644 --- a/apps/api/internal/project/exporter_handler.go +++ b/apps/api/internal/project/exporter_handler.go @@ -10,6 +10,7 @@ import ( "github.com/gin-gonic/gin" "github.com/google/uuid" "github.com/lib/pq" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "github.com/yldm-tech/pace/apps/api/internal/pagination" @@ -90,7 +91,7 @@ func (handler *Handler) exporterCreate(c *gin.Context, user *auth.User) { if len(projectIDs) == 0 { // Nobody named a project, so it is every project in the workspace this person is still a member of and which has not been archived. err = handler.db.WithContext(c.Request.Context()).Table("projects p"). - Joins("JOIN project_members pm ON pm.project_id = p.id AND pm.member_id = ? AND pm.is_active = TRUE AND pm.deleted_at IS NULL", user.ID). + Joins(access.MemberJoin("p", "id", access.WithMembershipSoftDelete()), user.ID). Where("p.workspace_id = ? AND p.archived_at IS NULL AND p.deleted_at IS NULL", workspaceID). Distinct().Pluck("p.id", &projectIDs).Error if err != nil { diff --git a/apps/api/internal/project/global_search_handler.go b/apps/api/internal/project/global_search_handler.go index 5036e93e..85d87782 100644 --- a/apps/api/internal/project/global_search_handler.go +++ b/apps/api/internal/project/global_search_handler.go @@ -7,6 +7,7 @@ import ( "github.com/gin-gonic/gin" "github.com/lib/pq" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "gorm.io/gorm" @@ -135,7 +136,7 @@ func (handler *Handler) searchWorkspaces(c *gin.Context, user *auth.User, search func (handler *Handler) searchProjects(c *gin.Context, user *auth.User, search, slug string) ([]gin.H, error) { query := handler.db.WithContext(c.Request.Context()).Table("projects p"). Joins("JOIN workspaces w ON w.id = p.workspace_id"). - Joins("JOIN project_members pm ON pm.project_id = p.id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("p", "id"), user.ID). Where("w.slug = ? AND p.archived_at IS NULL AND p.deleted_at IS NULL", slug) if search != "" { query = query.Where("p.name ILIKE ? OR p.identifier ILIKE ?", "%"+search+"%", "%"+search+"%") @@ -190,7 +191,7 @@ func (handler *Handler) memberIssueScope(c *gin.Context, user *auth.User, slug s return handler.db.WithContext(c.Request.Context()).Table("issues i"). Joins("JOIN workspaces w ON w.id = i.workspace_id"). Joins("JOIN projects p ON p.id = i.project_id AND p.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = i.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("i", "project_id"), user.ID). Where("w.slug = ?", slug) } @@ -225,7 +226,7 @@ func (handler *Handler) searchNamed(c *gin.Context, user *auth.User, search, slu query := handler.db.WithContext(c.Request.Context()).Table(table+" t"). Joins("JOIN workspaces w ON w.id = t.workspace_id"). Joins("JOIN projects p ON p.id = t.project_id AND p.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = t.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("t", "project_id"), user.ID). Where("w.slug = ? AND t.deleted_at IS NULL", slug) if search != "" { query = query.Where("t.name ILIKE ?", "%"+search+"%") @@ -261,7 +262,7 @@ func (handler *Handler) searchPages(c *gin.Context, user *auth.User, search, slu Joins("JOIN workspaces w ON w.id = pg.workspace_id"). Joins("JOIN project_pages pp ON pp.page_id = pg.id"). Joins("JOIN projects p ON p.id = pp.project_id AND p.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = pp.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("pp", "project_id"), user.ID). Where("w.slug = ? AND pg.deleted_at IS NULL", slug) if search != "" { query = query.Where("pg.name ILIKE ?", "%"+search+"%") diff --git a/apps/api/internal/project/handler.go b/apps/api/internal/project/handler.go index c287da0f..b3c665ff 100644 --- a/apps/api/internal/project/handler.go +++ b/apps/api/internal/project/handler.go @@ -10,6 +10,7 @@ import ( "time" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "github.com/yldm-tech/pace/apps/api/internal/projects" @@ -633,19 +634,10 @@ func (handler *Handler) createDefaultStates(tx *gorm.DB, project Project, actorI return projects.CreateDefaultStates(tx, project.ID, project.WorkspaceID, actorID, now, newUUID) } +// workspaceRole is the role lookup for the callers that treat "not a member" and "no matching role" alike: a miss is role 0, which is not one of the three roles, so every allowlist refuses it. The callers that have to tell a missing membership from a role they do not want read workspaceMemberRole, which is the one query behind both. func (handler *Handler) workspaceRole(ctx context.Context, slug, userID string) (int, error) { - var role *int - err := handler.db.WithContext(ctx).Table("workspace_members wm"). - Joins("JOIN workspaces w ON w.id = wm.workspace_id"). - Where("w.slug = ? AND wm.member_id = ? AND wm.is_active = TRUE AND wm.deleted_at IS NULL", slug, userID). - Select("wm.role").Limit(1).Scan(&role).Error - if err != nil { - return 0, err - } - if role == nil { - return 0, nil - } - return *role, nil + role, _, err := handler.workspaceMemberRole(ctx, slug, userID) + return role, err } func (handler *Handler) requireWorkspaceRole(c *gin.Context, user *auth.User, allowed ...int) bool { @@ -675,12 +667,8 @@ func (handler *Handler) workspaceOrProjectAdmin(ctx context.Context, slug, proje if workspaceAdmins > 0 { return true, nil } - var projectAdmins int64 - err = handler.db.WithContext(ctx).Table("project_members pm"). - Joins("JOIN workspaces w ON w.id = pm.workspace_id"). - Where("w.slug = ? AND pm.member_id = ? AND pm.project_id = ? AND pm.role = ? AND pm.is_active = TRUE AND pm.deleted_at IS NULL", slug, userID, projectID, roleAdmin). - Count(&projectAdmins).Error - return projectAdmins > 0, err + role, member, err := access.ProjectRole(ctx, handler.db, slug, projectID, userID) + return member && role == roleAdmin, err } // projectMemberIDsIncludingBots backs the membership check in retrieve, which diff --git a/apps/api/internal/project/issue_activity_handler.go b/apps/api/internal/project/issue_activity_handler.go index e1346a8b..aedddcd6 100644 --- a/apps/api/internal/project/issue_activity_handler.go +++ b/apps/api/internal/project/issue_activity_handler.go @@ -8,6 +8,7 @@ import ( "github.com/gin-gonic/gin" "github.com/lib/pq" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "gorm.io/gorm" @@ -93,7 +94,7 @@ func (handler *Handler) issueActivityRows(ctx context.Context, slug, projectID, query := handler.db.WithContext(ctx).Table("issue_activities ia"). Joins("JOIN workspaces w ON w.id = ia.workspace_id"). Joins("JOIN projects p ON p.id = ia.project_id"). - Joins("JOIN project_members pm ON pm.project_id = ia.project_id AND pm.member_id = ? AND pm.is_active = TRUE", userID). + Joins(access.MemberJoin("ia", "project_id"), userID). Where(`ia.issue_id = ? AND ia.deleted_at IS NULL AND w.slug = ? AND p.archived_at IS NULL AND (ia.field IS NULL OR ia.field NOT IN ?)`, issueID, slug, hiddenActivityFields) if since != nil { @@ -270,7 +271,7 @@ func (handler *Handler) issueActivityComments(ctx context.Context, slug, project query := handler.db.WithContext(ctx).Table("issue_comments ic"). Joins("JOIN workspaces w ON w.id = ic.workspace_id"). Joins("JOIN projects p ON p.id = ic.project_id"). - Joins("JOIN project_members pm ON pm.project_id = ic.project_id AND pm.member_id = ? AND pm.is_active = TRUE", userID). + Joins(access.MemberJoin("ic", "project_id"), userID). Where("ic.issue_id = ? AND ic.deleted_at IS NULL AND w.slug = ? AND p.archived_at IS NULL", issueID, slug) if since != nil { query = query.Where("ic.created_at > ?", *since) diff --git a/apps/api/internal/project/issue_search_handler.go b/apps/api/internal/project/issue_search_handler.go index 179d49a8..d6856bf0 100644 --- a/apps/api/internal/project/issue_search_handler.go +++ b/apps/api/internal/project/issue_search_handler.go @@ -6,6 +6,7 @@ import ( "time" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "gorm.io/gorm" @@ -24,7 +25,7 @@ func (handler *Handler) issueSearch(c *gin.Context, user *auth.User) { query := handler.db.WithContext(c.Request.Context()).Table("issues i"). Joins("JOIN workspaces w ON w.id = i.workspace_id"). Joins("JOIN projects p ON p.id = i.project_id AND p.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = i.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("i", "project_id"), user.ID). Where("w.slug = ?", slug). Where(issueObjectsPredicate("i")) diff --git a/apps/api/internal/project/membership_predicate_test.go b/apps/api/internal/project/membership_predicate_test.go new file mode 100644 index 00000000..54571414 --- /dev/null +++ b/apps/api/internal/project/membership_predicate_test.go @@ -0,0 +1,128 @@ +package project + +import ( + "net/http" + "net/http/httptest" + "regexp" + "strings" + "testing" + + "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/auth" + "gorm.io/driver/postgres" + "gorm.io/gorm" +) + +// These tests pin the membership join every project-scoped queryset carries, one alias at a time, because the join is held in string literals that nothing else reads: go build says nothing about a string, and the queries themselves only run under a database this suite skips by default. +// +// What they pin is a split that looks like drift and is not. `pm.is_active = TRUE` is the membership test, and it is written everywhere. `pm.deleted_at IS NULL` is the *membership row's* soft delete, which is set only when the parent project or workspace is soft-deleted and only by the worker cascade, and it is written at the sites whose queryset reads project_members through its own manager. A join traversed through another model's queryset carries no such filter — the rule stated at issue_ordering.go:28 and asserted for the analytics scope at advance_analytics_test.go:100 — so every scope below leaves it out on purpose. Adding it here would be a sweep that changes no row (the same cascade stamps the base row, which each of these queries already filters) while breaking parity with Django. +// +// The SQL is rendered rather than reproduced: a dry-run session builds the statement without connecting to anything, so the assertion is over what the database would have been sent. + +// placeholderNumbers matches the bind markers the postgres driver numbers, which are put back as question marks so a fragment reads the way the call site wrote it. +var placeholderNumbers = regexp.MustCompile(`\$\d+`) + +// dryRunHandler is a Handler whose queries render to SQL instead of reaching a database. DisableAutomaticPing is what keeps gorm.Open from dialling the DSN, which is never connected to. +func dryRunHandler(t *testing.T) *Handler { + t.Helper() + database, err := gorm.Open( + postgres.New(postgres.Config{DSN: "postgres://render@127.0.0.1:1/render"}), + &gorm.Config{DryRun: true, DisableAutomaticPing: true}, + ) + if err != nil { + t.Fatalf("opening a dry-run database: %v", err) + } + return &Handler{db: database} +} + +// renderedSQL is the statement a scope becomes. +func renderedSQL(t *testing.T, scope *gorm.DB) string { + t.Helper() + var rows []map[string]any + statement := scope.Find(&rows).Statement + if statement.SQL.Len() == 0 { + t.Fatal("the scope rendered no SQL") + } + return placeholderNumbers.ReplaceAllString(statement.SQL.String(), "?") +} + +// dryRunContext is a request the scope builders can read their route parameters off. +func dryRunContext(t *testing.T) *gin.Context { + t.Helper() + context, _ := gin.CreateTestContext(httptest.NewRecorder()) + context.Request = httptest.NewRequest(http.MethodGet, "/", nil) + context.Params = gin.Params{ + {Key: "slug", Value: "acme"}, + {Key: "id", Value: "00000000-0000-0000-0000-0000000000p1"}, + {Key: "module", Value: "00000000-0000-0000-0000-0000000000m1"}, + } + return context +} + +// Every queryset the session API's project routes read through joins the caller's membership, and the alias it joins on is the one its own base table names. +func TestTheProjectScopesJoinTheCallersMembership(t *testing.T) { + handler := dryRunHandler(t) + context := dryRunContext(t) + user := &auth.User{ID: "00000000-0000-0000-0000-0000000000u1"} + const project = "00000000-0000-0000-0000-0000000000p1" + + for _, test := range []struct { + name string + scope *gorm.DB + want string + }{ + {"memberIssueScope", handler.memberIssueScope(context, user, "acme"), + "JOIN project_members pm ON pm.project_id = i.project_id AND pm.member_id = ? AND pm.is_active = TRUE"}, + {"memberIssueScopeAnyProject", handler.memberIssueScopeAnyProject(context, user, "acme"), + "JOIN project_members pm ON pm.project_id = i.project_id AND pm.member_id = ? AND pm.is_active = TRUE"}, + {"projectScopedTable", handler.projectScopedTable(context, user, "cycles", "acme", project), + "JOIN project_members pm ON pm.project_id = t.project_id AND pm.member_id = ? AND pm.is_active = TRUE"}, + {"viewScope", handler.viewScope(context, user, "acme", project), + "JOIN project_members pm ON pm.project_id = v.project_id AND pm.member_id = ? AND pm.is_active = TRUE"}, + // A page reaches its project through the link table, so the membership is joined on the link's project rather than the page's. + {"pageScope", handler.pageScope(context, user, "acme", project), + "JOIN project_members pm ON pm.project_id = pp.project_id AND pm.member_id = ? AND pm.is_active = TRUE"}, + {"moduleLinkScope", handler.moduleLinkScope(context, user), + "JOIN project_members pm ON pm.project_id = l.project_id AND pm.member_id = ? AND pm.is_active = TRUE"}, + } { + t.Run(test.name, func(t *testing.T) { + rendered := renderedSQL(t, test.scope) + if !strings.Contains(rendered, test.want) { + t.Errorf("%s does not carry\n %s\nit renders\n %s", test.name, test.want, rendered) + } + // The membership's own soft delete belongs to project_members' manager, and a join traversed through another queryset does not apply it. + if strings.Contains(rendered, "pm.deleted_at") { + t.Errorf("%s checks the membership's soft delete, and Django's join does not", test.name) + } + }) + } +} + +// The membership join narrows rather than widens: none of these scopes can reach a row through a LEFT JOIN of it, and each keeps the project-liveness filter that is what actually closes a deleted project. +func TestTheProjectScopesJoinTheMembershipInner(t *testing.T) { + handler := dryRunHandler(t) + context := dryRunContext(t) + user := &auth.User{ID: "00000000-0000-0000-0000-0000000000u1"} + const project = "00000000-0000-0000-0000-0000000000p1" + + for _, test := range []struct { + name string + scope *gorm.DB + want string + }{ + {"memberIssueScope", handler.memberIssueScope(context, user, "acme"), "JOIN projects p ON p.id = i.project_id AND p.archived_at IS NULL"}, + {"viewScope", handler.viewScope(context, user, "acme", project), "JOIN projects p ON p.id = v.project_id AND p.archived_at IS NULL"}, + {"pageScope", handler.pageScope(context, user, "acme", project), "JOIN project_pages pp ON pp.page_id = p.id AND pp.deleted_at IS NULL"}, + {"moduleLinkScope", handler.moduleLinkScope(context, user), "JOIN projects p ON p.id = l.project_id AND p.archived_at IS NULL"}, + } { + t.Run(test.name, func(t *testing.T) { + rendered := renderedSQL(t, test.scope) + if strings.Contains(rendered, "LEFT JOIN project_members") { + t.Errorf("%s joins the membership outer, which lets a non-member's row through", test.name) + } + if !strings.Contains(rendered, test.want) { + t.Errorf("%s does not carry\n %s\nit renders\n %s", test.name, test.want, rendered) + } + }) + } +} diff --git a/apps/api/internal/project/module_handler.go b/apps/api/internal/project/module_handler.go index a22f914d..6e8fdfbc 100644 --- a/apps/api/internal/project/module_handler.go +++ b/apps/api/internal/project/module_handler.go @@ -6,6 +6,7 @@ import ( "net/http" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "github.com/yldm-tech/pace/apps/api/internal/validate" @@ -330,7 +331,7 @@ func (handler *Handler) moduleLinkScope(c *gin.Context, user *auth.User) *gorm.D return handler.db.WithContext(c.Request.Context()).Table("module_links l"). Joins("JOIN workspaces w ON w.id = l.workspace_id"). Joins("JOIN projects p ON p.id = l.project_id AND p.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = l.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("l", "project_id"), user.ID). Where("w.slug = ? AND l.project_id = ? AND l.module_id = ? AND l.deleted_at IS NULL", c.Param("slug"), c.Param("id"), c.Param("module")) } diff --git a/apps/api/internal/project/module_list_handler.go b/apps/api/internal/project/module_list_handler.go index 5a4d0f48..4e92d5c5 100644 --- a/apps/api/internal/project/module_list_handler.go +++ b/apps/api/internal/project/module_list_handler.go @@ -6,6 +6,7 @@ import ( "github.com/gin-gonic/gin" "github.com/lib/pq" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" ) @@ -65,7 +66,7 @@ func (handler *Handler) moduleRows(c *gin.Context, userID, moduleID string) ([]m Select(moduleAnnotations(), userID, c.Param("id"), c.Param("slug")). Joins("JOIN workspaces w ON w.id = m.workspace_id"). Joins("JOIN projects p ON p.id = m.project_id AND p.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = m.project_id AND pm.member_id = ? AND pm.is_active = TRUE", userID). + Joins(access.MemberJoin("m", "project_id"), userID). Where("w.slug = ? AND m.project_id = ? AND m.deleted_at IS NULL AND m.archived_at IS NULL", c.Param("slug"), c.Param("id")) if moduleID != "" { diff --git a/apps/api/internal/project/page_handler.go b/apps/api/internal/project/page_handler.go index cb14db8a..6b56289c 100644 --- a/apps/api/internal/project/page_handler.go +++ b/apps/api/internal/project/page_handler.go @@ -8,6 +8,7 @@ import ( "github.com/gin-gonic/gin" "github.com/lib/pq" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "gorm.io/gorm" @@ -150,7 +151,7 @@ func (handler *Handler) pageScope(c *gin.Context, user *auth.User, slug, project Joins("JOIN workspaces w ON w.id = p.workspace_id"). Joins("JOIN project_pages pp ON pp.page_id = p.id AND pp.deleted_at IS NULL"). Joins("JOIN projects pr ON pr.id = pp.project_id AND pr.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = pp.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("pp", "project_id"), user.ID). Where("w.slug = ? AND p.deleted_at IS NULL AND p.parent_id IS NULL", slug). Where("p.owned_by_id = ? OR p.access = ?", user.ID, pagePublicAccess). // The project filter is an annotation that is then filtered on, so the page has to be linked to *this* project while still being reachable through any of them. @@ -587,7 +588,7 @@ func (handler *Handler) pageSummary(c *gin.Context, user *auth.User) { Joins("JOIN workspaces w ON w.id = p.workspace_id"). Joins("JOIN project_pages pp ON pp.page_id = p.id"). Joins("JOIN projects pr ON pr.id = pp.project_id AND pr.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = pp.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("pp", "project_id"), user.ID). Where("w.slug = ? AND p.deleted_at IS NULL AND p.parent_id IS NULL", slug). Where("p.owned_by_id = ? OR p.access = ?", user.ID, pagePublicAccess). Where(`EXISTS (SELECT 1 FROM project_pages fp WHERE fp.page_id = p.id AND fp.project_id = ?)`, projectID) diff --git a/apps/api/internal/project/state_handler.go b/apps/api/internal/project/state_handler.go index d21deed0..9ea8c744 100644 --- a/apps/api/internal/project/state_handler.go +++ b/apps/api/internal/project/state_handler.go @@ -7,6 +7,7 @@ import ( "sort" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "gorm.io/gorm" @@ -92,7 +93,7 @@ func (handler *Handler) projectStates(c *gin.Context, user *auth.User) ([]State, err := handler.db.WithContext(c.Request.Context()).Table("states s").Select("s.*"). Joins("JOIN workspaces w ON w.id = s.workspace_id"). Joins("JOIN projects p ON p.id = s.project_id AND p.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = s.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("s", "project_id"), user.ID). Where("w.slug = ? AND s.project_id = ? AND s.is_triage = FALSE AND s.deleted_at IS NULL", c.Param("slug"), c.Param("id")). Order("s.sequence").Scan(&states).Error diff --git a/apps/api/internal/project/view_handler.go b/apps/api/internal/project/view_handler.go index 59f80f27..5303d275 100644 --- a/apps/api/internal/project/view_handler.go +++ b/apps/api/internal/project/view_handler.go @@ -9,6 +9,7 @@ import ( "time" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "gorm.io/gorm" @@ -113,7 +114,7 @@ func (handler *Handler) viewScope(c *gin.Context, user *auth.User, slug, project AND uf.project_id = ? AND uw.slug = ? AND uf.deleted_at IS NULL) AS is_favorite`, user.ID, projectID, slug). Joins("JOIN workspaces w ON w.id = v.workspace_id"). Joins("JOIN projects p ON p.id = v.project_id AND p.archived_at IS NULL"). - Joins("JOIN project_members pm ON pm.project_id = v.project_id AND pm.member_id = ? AND pm.is_active = TRUE", user.ID). + Joins(access.MemberJoin("v", "project_id"), user.ID). Where("w.slug = ? AND v.project_id = ? AND v.deleted_at IS NULL", slug, projectID). // Access 1 is public and 0 is private, and a private view is visible to the person who made it. Where("v.owned_by_id = ? OR v.access = 1", user.ID) diff --git a/apps/api/internal/project/workspace_aggregate_handler.go b/apps/api/internal/project/workspace_aggregate_handler.go index a13fdb95..65a74cc7 100644 --- a/apps/api/internal/project/workspace_aggregate_handler.go +++ b/apps/api/internal/project/workspace_aggregate_handler.go @@ -4,6 +4,7 @@ import ( "net/http" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" ) @@ -26,8 +27,7 @@ func (handler *Handler) workspaceLabels(c *gin.Context, user *auth.User) { err := handler.db.WithContext(c.Request.Context()).Table("labels l").Select("l.*"). Joins("JOIN workspaces w ON w.id = l.workspace_id"). Joins("JOIN projects p ON p.id = l.project_id AND p.archived_at IS NULL"). - Joins(`JOIN project_members pm ON pm.project_id = l.project_id - AND pm.member_id = ? AND pm.is_active = TRUE AND pm.deleted_at IS NULL`, user.ID). + Joins(access.MemberJoin("l", "project_id", access.WithMembershipSoftDelete()), user.ID). Where("w.slug = ? AND l.deleted_at IS NULL", c.Param("slug")). Order("l.created_at").Scan(&labels).Error if err != nil { @@ -52,8 +52,7 @@ func (handler *Handler) workspaceStates(c *gin.Context, user *auth.User) { err := handler.db.WithContext(c.Request.Context()).Table("states s").Select("s.*"). Joins("JOIN workspaces w ON w.id = s.workspace_id"). Joins("JOIN projects p ON p.id = s.project_id AND p.archived_at IS NULL"). - Joins(`JOIN project_members pm ON pm.project_id = s.project_id - AND pm.member_id = ? AND pm.is_active = TRUE AND pm.deleted_at IS NULL`, user.ID). + Joins(access.MemberJoin("s", "project_id", access.WithMembershipSoftDelete()), user.ID). Where("w.slug = ? AND s.is_triage = FALSE AND s.deleted_at IS NULL", c.Param("slug")). Order("s.sequence").Scan(&states).Error if err != nil { @@ -93,8 +92,7 @@ func (handler *Handler) workspaceCycles(c *gin.Context, user *auth.User) { Select(cycleAnnotations(), user.ID, nil, c.Param("slug"), now, now, now, now). Joins("JOIN workspaces w ON w.id = c.workspace_id"). Joins("JOIN projects p ON p.id = c.project_id AND p.archived_at IS NULL"). - Joins(`JOIN project_members pm ON pm.project_id = c.project_id - AND pm.member_id = ? AND pm.is_active = TRUE AND pm.deleted_at IS NULL`, user.ID). + Joins(access.MemberJoin("c", "project_id", access.WithMembershipSoftDelete()), user.ID). Where("w.slug = ? AND c.archived_at IS NULL AND c.deleted_at IS NULL", c.Param("slug")). Group("c.id").Order("c.created_at DESC").Scan(&rows).Error if err != nil { @@ -128,8 +126,7 @@ func (handler *Handler) workspaceModules(c *gin.Context, user *auth.User) { Select(moduleAnnotations(), user.ID, nil, c.Param("slug")). Joins("JOIN workspaces w ON w.id = m.workspace_id"). Joins("JOIN projects p ON p.id = m.project_id AND p.archived_at IS NULL"). - Joins(`JOIN project_members pm ON pm.project_id = m.project_id - AND pm.member_id = ? AND pm.is_active = TRUE AND pm.deleted_at IS NULL`, user.ID). + Joins(access.MemberJoin("m", "project_id", access.WithMembershipSoftDelete()), user.ID). Where("w.slug = ? AND m.archived_at IS NULL AND m.deleted_at IS NULL", c.Param("slug")). Group("m.id").Order("m.created_at DESC").Scan(&rows).Error if err != nil { diff --git a/apps/api/internal/project/workspace_user_handler.go b/apps/api/internal/project/workspace_user_handler.go index ec3b39a7..22f02261 100644 --- a/apps/api/internal/project/workspace_user_handler.go +++ b/apps/api/internal/project/workspace_user_handler.go @@ -38,7 +38,7 @@ func (handler *Handler) registerWorkspaceUserRoutes(router gin.IRouter) { // It guards nothing but the session. What stands in for a permission is the pair of lookups it opens with: the caller has to be an active member of the workspace and so does the person being asked about, and either one missing is a 404 rather than a 403. func (handler *Handler) workspaceUserProfile(c *gin.Context, user *auth.User) { slug, target := c.Param("slug"), c.Param("user") - callerRole, found, err := handler.activeWorkspaceRole(c.Request.Context(), slug, user.ID) + callerRole, found, err := handler.workspaceMemberRole(c.Request.Context(), slug, user.ID) if err != nil { handler.internalError(c, err) return @@ -725,19 +725,6 @@ func (handler *Handler) workspaceProjectMembers(c *gin.Context, user *auth.User) drf.Respond(c, http.StatusOK, grouped) } -// activeWorkspaceRole reads the caller's role, and reports whether they are a member at all. -func (handler *Handler) activeWorkspaceRole(ctx context.Context, slug, userID string) (int, bool, error) { - var roles []int - err := handler.db.WithContext(ctx).Table("workspace_members wm"). - Joins("JOIN workspaces w ON w.id = wm.workspace_id"). - Where("w.slug = ? AND wm.member_id = ? AND wm.is_active = TRUE AND wm.deleted_at IS NULL", slug, userID). - Limit(1).Pluck("wm.role", &roles).Error - if err != nil || len(roles) == 0 { - return 0, false, err - } - return roles[0], true, nil -} - // workspaceMemberProfile is the eight fields the profile reports about a person, which stop short of anything an account needs. func (handler *Handler) workspaceMemberProfile(ctx context.Context, slug, memberID string) (gin.H, bool, error) { var rows []struct { diff --git a/apps/api/internal/server/membership_join_test.go b/apps/api/internal/server/membership_join_test.go new file mode 100644 index 00000000..c3e82fb5 --- /dev/null +++ b/apps/api/internal/server/membership_join_test.go @@ -0,0 +1,19 @@ +package server + +import ( + "regexp" + "strings" + "testing" +) + +// TestTheMembershipJoinIsNotWrittenOutByHand keeps the join that gates every project-scoped queryset in one place. +// +// It was written out as a literal at thirty-six call sites in three packages, which is how the five that carry the membership's own soft delete came to be indistinguishable from the thirty-one that leave it out on purpose: the difference was a few characters inside otherwise identical strings. access.MemberJoin writes it now, and the exception is an argument you can see. A new query that spells the join out again is not wrong SQL, but it puts the semantics back where nobody can find them, so it fails here instead. +// +// The pattern is the shape that joins the project id first and the member id second, which is the authorization join. The assignee joins in the work-item annotations (pm2.member_id = ia.assignee_id) decide which assignees to render rather than who may read, and they are deliberately not this. +func TestTheMembershipJoinIsNotWrittenOutByHand(t *testing.T) { + handWritten := regexp.MustCompile(`JOIN project_members \w+ ON \w+\.project_id = \w+\.\w+ AND \w+\.member_id = \?`) + if offenders := walk(t, handWritten); len(offenders) > 0 { + t.Errorf("the membership join belongs to access.MemberJoin, which is where its two soft-delete rules are written down:\n %s", strings.Join(offenders, "\n ")) + } +} diff --git a/apps/api/internal/worker/export_rows.go b/apps/api/internal/worker/export_rows.go index be9d3417..d0fa29ac 100644 --- a/apps/api/internal/worker/export_rows.go +++ b/apps/api/internal/worker/export_rows.go @@ -6,6 +6,7 @@ import ( "strings" "time" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/drf" "gorm.io/gorm" ) @@ -53,7 +54,7 @@ func loadExportIssues(ctx context.Context, db *gorm.DB, workspaceID string, proj } err := db.WithContext(ctx).Table("issues i"). Joins("JOIN projects p ON p.id = i.project_id"). - Joins("JOIN project_members pm ON pm.project_id = i.project_id AND pm.member_id = ? AND pm.is_active = TRUE", memberID). + Joins(access.MemberJoin("i", "project_id"), memberID). Joins("LEFT JOIN states s ON s.id = i.state_id"). Joins("LEFT JOIN users cu ON cu.id = i.created_by_id"). Joins("LEFT JOIN estimate_points ep ON ep.id = i.estimate_point_id"). diff --git a/apps/api/internal/workspace/project_asset_handler.go b/apps/api/internal/workspace/project_asset_handler.go index ae7ce285..49f9b937 100644 --- a/apps/api/internal/workspace/project_asset_handler.go +++ b/apps/api/internal/workspace/project_asset_handler.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/access" "github.com/yldm-tech/pace/apps/api/internal/auth" "github.com/yldm-tech/pace/apps/api/internal/drf" "github.com/yldm-tech/pace/apps/api/internal/uploads" @@ -435,18 +436,14 @@ func (handler *Handler) projectAsset(c *gin.Context, user *auth.User, uploadedOn return assets[0], true } -// requireProjectMember is the project branch of allow_permission: an active membership of the project named in the url, at any role. +// requireProjectMember is the project branch of allow_permission: an active membership of the project named in the url, at any role. The project is the :id parameter on these routes rather than the :project the external API uses. func (handler *Handler) requireProjectMember(c *gin.Context, user *auth.User) bool { - var count int64 - err := handler.db.WithContext(c.Request.Context()).Table("project_members pm"). - Joins("JOIN workspaces w ON w.id = pm.workspace_id"). - Where("w.slug = ? AND pm.project_id = ? AND pm.member_id = ? AND pm.is_active = TRUE", - c.Param("slug"), c.Param("id"), user.ID).Count(&count).Error + _, member, err := access.ProjectRole(c.Request.Context(), handler.db, c.Param("slug"), c.Param("id"), user.ID) if err != nil { handler.internalError(c, err) return false } - if count == 0 { + if !member { forbidden(c) return false } diff --git a/apps/api/internal/workspace/project_asset_permission_test.go b/apps/api/internal/workspace/project_asset_permission_test.go new file mode 100644 index 00000000..87e5b982 --- /dev/null +++ b/apps/api/internal/workspace/project_asset_permission_test.go @@ -0,0 +1,87 @@ +package workspace + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/gin-gonic/gin" + "github.com/yldm-tech/pace/apps/api/internal/auth" + "gorm.io/driver/postgres" + "gorm.io/gorm" + "gorm.io/gorm/logger" +) + +// requireProjectMember is the whole gate on the project asset routes, and its query is the only thing in them that reads the caller's membership. It is not a pure function and this package's database-backed tests are skipped unless WORKSPACE_TEST_DATABASE_URL is set, so what this does is run the gate against a session that records the statement instead of sending it, and read the predicate back out of the record. + +// recordedStatements is a gorm logger that keeps the SQL a session would have sent. Under DryRun nothing is sent, and the trace still carries the statement with its bind values inlined. +type recordedStatements struct { + statements []string +} + +func (recorded *recordedStatements) LogMode(logger.LogLevel) logger.Interface { return recorded } + +func (recorded *recordedStatements) Info(context.Context, string, ...any) {} + +func (recorded *recordedStatements) Warn(context.Context, string, ...any) {} + +func (recorded *recordedStatements) Error(context.Context, string, ...any) {} + +func (recorded *recordedStatements) Trace(_ context.Context, _ time.Time, sql func() (string, int64), _ error) { + statement, _ := sql() + recorded.statements = append(recorded.statements, statement) +} + +func (recorded *recordedStatements) contains(fragment string) bool { + for _, statement := range recorded.statements { + if strings.Contains(statement, fragment) { + return true + } + } + return false +} + +func (recorded *recordedStatements) String() string { + return strings.Join(recorded.statements, "\n ") +} + +// recordingHandler is a Handler whose queries are built and recorded rather than run. DisableAutomaticPing is what keeps gorm.Open from dialling the DSN, which is never connected to. +func recordingHandler(t *testing.T) (*Handler, *recordedStatements) { + t.Helper() + recorded := &recordedStatements{} + database, err := gorm.Open( + postgres.New(postgres.Config{DSN: "postgres://render@127.0.0.1:1/render"}), + &gorm.Config{DryRun: true, DisableAutomaticPing: true, Logger: recorded}, + ) + if err != nil { + t.Fatalf("opening a dry-run database: %v", err) + } + return &Handler{db: database}, recorded +} + +// The gate asks project_members through its own manager and has no other liveness condition in it, so the membership's own soft delete is what stops a member of a soft-deleted project from reading and writing its assets. It arrives with the worker's cascade rather than with the delete, which is a lag and not a hole: the project row itself is stamped first, and no route behind this gate is reachable without the project. +func TestTheProjectAssetGateRefusesACascadedMembership(t *testing.T) { + handler, recorded := recordingHandler(t) + recorder := httptest.NewRecorder() + context, _ := gin.CreateTestContext(recorder) + context.Request = httptest.NewRequest(http.MethodGet, "/", nil) + context.Params = gin.Params{ + {Key: "slug", Value: "acme"}, + {Key: "id", Value: "00000000-0000-0000-0000-0000000000p1"}, + } + + if handler.requireProjectMember(context, &auth.User{ID: "00000000-0000-0000-0000-0000000000u1"}) { + t.Error("the gate admitted a caller whose membership lookup matched nothing") + } + if recorder.Code != http.StatusForbidden { + t.Errorf("the gate answered %d, want %d", recorder.Code, http.StatusForbidden) + } + for _, fragment := range []string{"pm.is_active = TRUE", "pm.deleted_at IS NULL"} { + if !recorded.contains(fragment) { + t.Errorf("the gate does not filter %s:\n %s", fragment, recorded) + } + } +} diff --git a/apps/space/package.json b/apps/space/package.json index f773d059..7fe586f1 100644 --- a/apps/space/package.json +++ b/apps/space/package.json @@ -1,6 +1,6 @@ { "name": "space", - "version": "1.4.38", + "version": "1.4.39", "private": true, "license": "AGPL-3.0", "type": "module", diff --git a/apps/web/package.json b/apps/web/package.json index 08ee07aa..cd55c38a 100644 --- a/apps/web/package.json +++ b/apps/web/package.json @@ -1,6 +1,6 @@ { "name": "web", - "version": "1.4.38", + "version": "1.4.39", "private": true, "license": "AGPL-3.0", "type": "module", diff --git a/package.json b/package.json index fa3dbd24..6291a891 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "pace", - "version": "1.4.38", + "version": "1.4.39", "private": true, "description": "Project management that keeps pace with the work", "license": "AGPL-3.0", diff --git a/packages/codemods/package.json b/packages/codemods/package.json index eff21b10..e20d849a 100644 --- a/packages/codemods/package.json +++ b/packages/codemods/package.json @@ -1,6 +1,6 @@ { "name": "@pace/codemods", - "version": "1.4.38", + "version": "1.4.39", "private": true, "scripts": { "check:format": "oxfmt --check .", diff --git a/packages/constants/package.json b/packages/constants/package.json index 6b7e5f02..b58b4f76 100644 --- a/packages/constants/package.json +++ b/packages/constants/package.json @@ -1,6 +1,6 @@ { "name": "@pace/constants", - "version": "1.4.38", + "version": "1.4.39", "private": true, "license": "AGPL-3.0", "type": "module", diff --git a/packages/editor/package.json b/packages/editor/package.json index e3aab465..cc755645 100644 --- a/packages/editor/package.json +++ b/packages/editor/package.json @@ -1,6 +1,6 @@ { "name": "@pace/editor", - "version": "1.4.38", + "version": "1.4.39", "private": true, "description": "Core Editor that powers Pace", "keywords": [ diff --git a/packages/hooks/package.json b/packages/hooks/package.json index 7db38f89..a465e16a 100644 --- a/packages/hooks/package.json +++ b/packages/hooks/package.json @@ -1,6 +1,6 @@ { "name": "@pace/hooks", - "version": "1.4.38", + "version": "1.4.39", "private": true, "description": "React hooks that are shared across multiple apps internally", "license": "AGPL-3.0", diff --git a/packages/i18n/package.json b/packages/i18n/package.json index 5c179910..afd8f563 100644 --- a/packages/i18n/package.json +++ b/packages/i18n/package.json @@ -1,6 +1,6 @@ { "name": "@pace/i18n", - "version": "1.4.38", + "version": "1.4.39", "private": true, "description": "I18n shared across multiple apps internally", "license": "AGPL-3.0", diff --git a/packages/propel/package.json b/packages/propel/package.json index 5cf9843f..1a404e58 100644 --- a/packages/propel/package.json +++ b/packages/propel/package.json @@ -1,6 +1,6 @@ { "name": "@pace/propel", - "version": "1.4.38", + "version": "1.4.39", "private": true, "license": "AGPL-3.0", "type": "module", diff --git a/packages/services/package.json b/packages/services/package.json index f0af3409..b2d62b85 100644 --- a/packages/services/package.json +++ b/packages/services/package.json @@ -1,6 +1,6 @@ { "name": "@pace/services", - "version": "1.4.38", + "version": "1.4.39", "private": true, "license": "AGPL-3.0", "type": "module", diff --git a/packages/shared-state/package.json b/packages/shared-state/package.json index 98cd1e7c..a1f21d63 100644 --- a/packages/shared-state/package.json +++ b/packages/shared-state/package.json @@ -1,6 +1,6 @@ { "name": "@pace/shared-state", - "version": "1.4.38", + "version": "1.4.39", "private": true, "description": "Shared state shared across multiple apps internally", "license": "AGPL-3.0", diff --git a/packages/tailwind-config/package.json b/packages/tailwind-config/package.json index 35406e35..4fef73fa 100644 --- a/packages/tailwind-config/package.json +++ b/packages/tailwind-config/package.json @@ -1,6 +1,6 @@ { "name": "@pace/tailwind-config", - "version": "1.4.38", + "version": "1.4.39", "private": true, "description": "common tailwind configuration across monorepo", "license": "AGPL-3.0", diff --git a/packages/types/package.json b/packages/types/package.json index 4f252210..f16a1dfd 100644 --- a/packages/types/package.json +++ b/packages/types/package.json @@ -1,6 +1,6 @@ { "name": "@pace/types", - "version": "1.4.38", + "version": "1.4.39", "private": true, "license": "AGPL-3.0", "type": "module", diff --git a/packages/typescript-config/package.json b/packages/typescript-config/package.json index 1df10826..a65311ee 100644 --- a/packages/typescript-config/package.json +++ b/packages/typescript-config/package.json @@ -1,6 +1,6 @@ { "name": "@pace/typescript-config", - "version": "1.4.38", + "version": "1.4.39", "private": true, "license": "AGPL-3.0", "files": [ diff --git a/packages/ui/package.json b/packages/ui/package.json index 8f128831..d3f99689 100644 --- a/packages/ui/package.json +++ b/packages/ui/package.json @@ -1,6 +1,6 @@ { "name": "@pace/ui", - "version": "1.4.38", + "version": "1.4.39", "private": true, "description": "UI components shared across multiple apps internally", "license": "AGPL-3.0", diff --git a/packages/utils/package.json b/packages/utils/package.json index 274f4cc1..84fd1eca 100644 --- a/packages/utils/package.json +++ b/packages/utils/package.json @@ -1,6 +1,6 @@ { "name": "@pace/utils", - "version": "1.4.38", + "version": "1.4.39", "private": true, "description": "Helper functions shared across multiple apps internally", "license": "AGPL-3.0",