diff --git a/appview/db/pulls.go b/appview/db/pulls.go index 10f7e47a..d426a7fd 100644 --- a/appview/db/pulls.go +++ b/appview/db/pulls.go @@ -295,11 +295,14 @@ type PullCount struct { func GetPullCount(e Execer, repoAt syntax.ATURI) (PullCount, error) { row := e.QueryRow(` select - count(case when state = 0 then 1 end) as open_count, - count(case when state = 1 then 1 end) as merged_count, - count(case when state = 2 then 1 end) as closed_count + count(case when state = ? then 1 end) as open_count, + count(case when state = ? then 1 end) as merged_count, + count(case when state = ? then 1 end) as closed_count from pulls where repo_at = ?`, + PullOpen, + PullMerged, + PullClosed, repoAt, ) diff --git a/appview/pages/templates/repo/pulls/pull.html b/appview/pages/templates/repo/pulls/pull.html index 5ceaae99..bedc734b 100644 --- a/appview/pages/templates/repo/pulls/pull.html +++ b/appview/pages/templates/repo/pulls/pull.html @@ -35,9 +35,9 @@ > {{ .Pull.State.String }} - + opened by - {{ $owner := didOrHandle .Pull.OwnerDid .PullOwnerHandle }} + {{ $owner := index $.DidHandleMap .Pull.OwnerDid }} {{ $owner }} @@ -80,9 +80,7 @@ id="patch" name="patch" class="font-mono w-full h-full p-4 rounded-b border border-gray-200 text-sm hidden" - > -{{- .Pull.Patch -}} + >{{- .Pull.Patch -}}
- - - {{ if or (eq .LoggedInUser.Did .Pull.OwnerDid) (eq .LoggedInUser.Did .RepoInfo.OwnerDid) }} - {{ $action := "close" }} - {{ $icon := "circle-x" }} - {{ $hoverColor := "red" }} - {{ if .Pull.State.IsClosed }} - {{ $action = "reopen" }} - {{ $icon = "circle-dot" }} - {{ $hoverColor = "green" }} - {{ end }} -
- -
-
-
- {{ end }} -
- {{ end }} - {{ end }} - - {{ end }} + {{ if .Pull.State.IsMerged }} + {{ block "alreadyMergedCard" . }} {{ end }} + {{ else if .MergeCheck }} + {{ if .MergeCheck.IsConflicted }} + {{ block "isConflictedCard" . }} {{ end }} + {{ else }} + {{ block "noConflictsCard" . }} {{ end }} + {{ end }} + {{ end }} - {{ if .LoggedInUser }} -
- - -
-
- {{ end }} + {{ block "newComment" . }} {{ end }} - {{ if and (or (eq .LoggedInUser.Did .Pull.OwnerDid) (eq .LoggedInUser.Did .RepoInfo.OwnerDid)) (not .MergeCheck) (not .Pull.State.IsMerged) }} + {{ if and (or $isPullAuthor $isRepoCollaborator) (not .Pull.State.IsMerged) }} {{ $action := "close" }} {{ $icon := "circle-x" }} {{ $hoverColor := "red" }} @@ -383,22 +179,187 @@ {{ $icon = "circle-dot" }} {{ $hoverColor = "green" }} {{ end }} -
- -
+ {{ end }} -
{{ end }} + +{{ define "comments" }} + {{ range $index, $comment := .Comments }} +
+ {{ if eq $index 0 }} +
+ {{ else }} +
+ {{ end }} +
+ {{ $owner := index $.DidHandleMap .OwnerDid }} + + {{ $owner }} + + + + {{ .Created | timeFmt }} + +
+
+ {{ .Body | markdown }} +
+
+ {{ end }} +{{ end }} + +{{ define "newComment" }} + {{ if .LoggedInUser }} +
+ + +
+
+ {{ else }} +
+ login to join the discussion +
+ {{ end }} +{{ end }} + +{{ define "alreadyMergedCard" }} +
+ {{ if gt (len .Comments) 0 }} +
+ {{ else }} +
+ {{ end }} + + +
+ + Pull request successfully merged +
+ +
+

This pull request has been merged into the base branch.

+
+
+{{ end }} + +{{ define "isConflictedCard" }} +
+ {{ if gt (len .Comments) 0 }} +
+ {{ else }} +
+ {{ end }} + +
+ + merge conflicts detected +
+ +
+
    + {{ range .MergeCheck.Conflicts }} +
  • + + {{ slice .Filename 0 (sub (len .Filename) 2) }} +
    • + {{ end }} +
+
+
+

+ Please resolve these conflicts locally and update + the patch to continue with the merge. +

+
+
+{{ end }} + + +{{ define "noConflictsCard" }} + {{ $isRepoCollaborator := .RepoInfo.Roles.IsCollaborator }} +
+ {{ if gt (len .Comments) 0 }} +
+ {{ else }} +
+ {{ end }} + +
+ + ready to merge +
+ +
+ No conflicts detected with the base branch. This + pull request can be merged safely. +
+ +
+ {{ if $isRepoCollaborator }} + + {{ end }} + +
+
+
+
+{{ end }} diff --git a/appview/state/middleware.go b/appview/state/middleware.go index 88564131..bce4a919 100644 --- a/appview/state/middleware.go +++ b/appview/state/middleware.go @@ -4,6 +4,7 @@ import ( "context" "log" "net/http" + "strconv" "strings" "time" @@ -98,7 +99,7 @@ func AuthMiddleware(s *State) Middleware { } } -func RoleMiddleware(s *State, group string) Middleware { +func knotRoleMiddleware(s *State, group string) Middleware { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // requires auth also @@ -128,6 +129,10 @@ func RoleMiddleware(s *State, group string) Middleware { } } +func KnotOwner(s *State) Middleware { + return knotRoleMiddleware(s, "server:owner") +} + func RepoPermissionMiddleware(s *State, requiredPerm string) Middleware { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -188,7 +193,7 @@ func ResolveIdent(s *State) Middleware { } } -func ResolveRepoKnot(s *State) Middleware { +func ResolveRepo(s *State) Middleware { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { repoName := chi.URLParam(req, "repo") @@ -215,3 +220,36 @@ func ResolveRepoKnot(s *State) Middleware { }) } } + +// middleware that is tacked on top of /{user}/{repo}/pulls/{pull} +func ResolvePull(s *State) Middleware { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + f, err := fullyResolvedRepo(r) + if err != nil { + log.Println("failed to fully resolve repo", err) + http.Error(w, "invalid repo url", http.StatusNotFound) + return + } + + prId := chi.URLParam(r, "pull") + prIdInt, err := strconv.Atoi(prId) + if err != nil { + http.Error(w, "bad pr id", http.StatusBadRequest) + log.Println("failed to parse pr id", err) + return + } + + pr, comments, err := db.GetPullWithComments(s.db, f.RepoAt, prIdInt) + if err != nil { + log.Println("failed to get pull and comments", err) + return + } + + ctx := context.WithValue(r.Context(), "pull", pr) + ctx = context.WithValue(ctx, "pull_comments", comments) + + next.ServeHTTP(w, r.WithContext(ctx)) + }) + } +} diff --git a/appview/state/repo.go b/appview/state/repo.go index faac75ae..fd4b64f8 100644 --- a/appview/state/repo.go +++ b/appview/state/repo.go @@ -232,31 +232,29 @@ func (s *State) RepoDescription(w http.ResponseWriter, r *http.Request) { func (s *State) EditPatch(w http.ResponseWriter, r *http.Request) { user := s.auth.GetUser(r) - f, err := fullyResolvedRepo(r) - if err != nil { - log.Println("failed to get repo and knot", err) - s.pages.Notice(w, "pull-error", "Failed to edit patch. Try again later.") + + patch := r.FormValue("patch") + if patch == "" { + s.pages.Notice(w, "pull-error", "Patch is required.") return } - prId := chi.URLParam(r, "pull") - prIdInt, err := strconv.Atoi(prId) - if err != nil { - http.Error(w, "bad pr id", http.StatusBadRequest) - log.Println("failed to parse pr id", err) + pull, ok := r.Context().Value("pull").(*db.Pull) + if !ok { + log.Println("failed to get pull") + s.pages.Notice(w, "pull-error", "Failed to edit patch. Try again later.") return } - patch := r.FormValue("patch") - if patch == "" { - s.pages.Notice(w, "pull-error", "Patch is required.") + if pull.OwnerDid != user.Did { + log.Println("failed to edit pull information") + s.pages.Notice(w, "pull-error", "Unauthorized") return } - // Get pull information before updating to get the atproto record URI - pull, _, err := db.GetPullWithComments(s.db, f.RepoAt, prIdInt) + f, err := fullyResolvedRepo(r) if err != nil { - log.Println("failed to get pull information", err) + log.Println("failed to get repo and knot", err) s.pages.Notice(w, "pull-error", "Failed to edit patch. Try again later.") return } @@ -273,7 +271,7 @@ func (s *State) EditPatch(w http.ResponseWriter, r *http.Request) { defer tx.Rollback() // Update patch in the database within transaction - err = db.EditPatch(tx, f.RepoAt, prIdInt, patch) + err = db.EditPatch(tx, f.RepoAt, pull.PullId, patch) if err != nil { log.Println("failed to update patch", err) s.pages.Notice(w, "pull-error", "Failed to edit patch. Try again later.") @@ -362,7 +360,7 @@ func (s *State) EditPatch(w http.ResponseWriter, r *http.Request) { return } - s.pages.HxLocation(w, fmt.Sprintf("/@%s/%s/pulls/%d", f.OwnerHandle(), f.RepoName, prIdInt)) + s.pages.HxLocation(w, fmt.Sprintf("/@%s/%s/pulls/%d", f.OwnerHandle(), f.RepoName, pull.PullId)) return } @@ -489,30 +487,20 @@ func (s *State) RepoSinglePull(w http.ResponseWriter, r *http.Request) { return } - prId := chi.URLParam(r, "pull") - prIdInt, err := strconv.Atoi(prId) - if err != nil { - http.Error(w, "bad pr id", http.StatusBadRequest) - log.Println("failed to parse pr id", err) - return - } - - pr, comments, err := db.GetPullWithComments(s.db, f.RepoAt, prIdInt) - if err != nil { - log.Println("failed to get pr and comments", err) - s.pages.Notice(w, "pull", "Failed to load pull request. Try again later.") + pull, ok1 := r.Context().Value("pull").(*db.Pull) + comments, ok2 := r.Context().Value("pull_comments").([]db.PullComment) + if !ok1 || !ok2 { + log.Println("failed to get pull") + s.pages.Notice(w, "pull-error", "Failed to edit patch. Try again later.") return } - pullOwnerIdent, err := s.resolver.ResolveIdent(r.Context(), pr.OwnerDid) - if err != nil { - log.Println("failed to resolve pull owner", err) - } - identsToResolve := make([]string, len(comments)) for i, comment := range comments { identsToResolve[i] = comment.OwnerDid } + identsToResolve = append(identsToResolve, pull.OwnerDid) + resolvedIds := s.resolver.ResolveIdents(r.Context(), identsToResolve) didHandleMap := make(map[string]string) for _, identity := range resolvedIds { @@ -526,7 +514,7 @@ func (s *State) RepoSinglePull(w http.ResponseWriter, r *http.Request) { var mergeCheckResponse types.MergeCheckResponse // Only perform merge check if the pull request is not already merged - if pr.State != db.PullMerged { + if pull.State != db.PullMerged { secret, err := db.GetRegistrationKey(s.db, f.Knot) if err != nil { log.Printf("failed to get registration key for %s", f.Knot) @@ -536,7 +524,7 @@ func (s *State) RepoSinglePull(w http.ResponseWriter, r *http.Request) { ksClient, err := NewSignedClient(f.Knot, secret, s.config.Dev) if err == nil { - resp, err := ksClient.MergeCheck([]byte(pr.Patch), pr.OwnerDid, f.RepoName, pr.TargetBranch) + resp, err := ksClient.MergeCheck([]byte(pull.Patch), pull.OwnerDid, f.RepoName, pull.TargetBranch) if err != nil { log.Println("failed to check for mergeability:", err) } else { @@ -556,13 +544,12 @@ func (s *State) RepoSinglePull(w http.ResponseWriter, r *http.Request) { } s.pages.RepoSinglePull(w, pages.RepoSinglePullParams{ - LoggedInUser: user, - RepoInfo: f.RepoInfo(s, user), - Pull: *pr, - Comments: comments, - PullOwnerHandle: pullOwnerIdent.Handle.String(), - DidHandleMap: didHandleMap, - MergeCheck: mergeCheckResponse, + LoggedInUser: user, + RepoInfo: f.RepoInfo(s, user), + Pull: *pull, + Comments: comments, + DidHandleMap: didHandleMap, + MergeCheck: mergeCheckResponse, }) } @@ -1012,7 +999,7 @@ func (f *FullyResolvedRepo) RepoInfo(s *State, u *auth.User) pages.RepoInfo { Description: f.Description, IsStarred: isStarred, Knot: knot, - Roles: rolesInRepo(s, u, f), + Roles: RolesInRepo(s, u, f), Stats: db.RepoStats{ StarCount: starCount, IssueCount: issueCount, @@ -1464,19 +1451,13 @@ func (s *State) MergePull(w http.ResponseWriter, r *http.Request) { return } - // Get the pull request ID from the request URL - pullId := chi.URLParam(r, "pull") - pullIdInt, err := strconv.Atoi(pullId) - if err != nil { - log.Println("failed to parse pull ID:", err) - s.pages.Notice(w, "pull-merge-error", "Failed to merge pull request. Try again later.") + pull, ok := r.Context().Value("pull").(*db.Pull) + if !ok { + log.Println("failed to get pull") + s.pages.Notice(w, "pull-error", "Failed to edit patch. Try again later.") return } - // Get the patch data from the request body - patch := r.FormValue("patch") - branch := r.FormValue("targetBranch") - secret, err := db.GetRegistrationKey(s.db, f.Knot) if err != nil { log.Printf("no registration key found for domain %s: %s\n", f.Knot, err) @@ -1492,7 +1473,7 @@ func (s *State) MergePull(w http.ResponseWriter, r *http.Request) { } // Merge the pull request - resp, err := ksClient.Merge([]byte(patch), user.Did, f.RepoName, branch) + resp, err := ksClient.Merge([]byte(pull.Patch), user.Did, f.RepoName, pull.TargetBranch) if err != nil { log.Printf("failed to merge pull request: %s", err) s.pages.Notice(w, "pull-merge-error", "Failed to merge pull request. Try again later.") @@ -1500,13 +1481,13 @@ func (s *State) MergePull(w http.ResponseWriter, r *http.Request) { } if resp.StatusCode == http.StatusOK { - err := db.MergePull(s.db, f.RepoAt, pullIdInt) + err := db.MergePull(s.db, f.RepoAt, pull.PullId) if err != nil { log.Printf("failed to update pull request status in database: %s", err) s.pages.Notice(w, "pull-merge-error", "Failed to merge pull request. Try again later.") return } - s.pages.HxLocation(w, fmt.Sprintf("/@%s/%s/pulls/%d", f.OwnerHandle(), f.RepoName, pullIdInt)) + s.pages.HxLocation(w, fmt.Sprintf("/@%s/%s/pulls/%d", f.OwnerHandle(), f.RepoName, pull.PullId)) } else { log.Printf("knotserver returned non-OK status code for merge: %d", resp.StatusCode) s.pages.Notice(w, "pull-merge-error", "Failed to merge pull request. Try again later.") @@ -1609,16 +1590,29 @@ func (s *State) PullComment(w http.ResponseWriter, r *http.Request) { } func (s *State) ClosePull(w http.ResponseWriter, r *http.Request) { + user := s.auth.GetUser(r) + f, err := fullyResolvedRepo(r) if err != nil { log.Println("malformed middleware") return } - pullId := chi.URLParam(r, "pull") - pullIdInt, err := strconv.Atoi(pullId) - if err != nil { - log.Println("malformed middleware") + pull, ok := r.Context().Value("pull").(*db.Pull) + if !ok { + log.Println("failed to get pull") + s.pages.Notice(w, "pull-error", "Failed to edit patch. Try again later.") + return + } + + // auth filter: only owner or collaborators can close + roles := RolesInRepo(s, user, f) + isCollaborator := roles.IsCollaborator() + isPullAuthor := user.Did == pull.OwnerDid + isCloseAllowed := isCollaborator || isPullAuthor + if !isCloseAllowed { + log.Println("failed to close pull") + s.pages.Notice(w, "pull-close", "You are unauthorized to close this pull.") return } @@ -1631,7 +1625,7 @@ func (s *State) ClosePull(w http.ResponseWriter, r *http.Request) { } // Close the pull in the database - err = db.ClosePull(tx, f.RepoAt, pullIdInt) + err = db.ClosePull(tx, f.RepoAt, pull.PullId) if err != nil { log.Println("failed to close pull", err) s.pages.Notice(w, "pull-close", "Failed to close pull.") @@ -1645,11 +1639,13 @@ func (s *State) ClosePull(w http.ResponseWriter, r *http.Request) { return } - s.pages.HxLocation(w, fmt.Sprintf("/%s/pulls/%d", f.OwnerSlashRepo(), pullIdInt)) + s.pages.HxLocation(w, fmt.Sprintf("/%s/pulls/%d", f.OwnerSlashRepo(), pull.PullId)) return } func (s *State) ReopenPull(w http.ResponseWriter, r *http.Request) { + user := s.auth.GetUser(r) + f, err := fullyResolvedRepo(r) if err != nil { log.Println("failed to resolve repo", err) @@ -1657,24 +1653,34 @@ func (s *State) ReopenPull(w http.ResponseWriter, r *http.Request) { return } - // Start a transaction - tx, err := s.db.BeginTx(r.Context(), nil) - if err != nil { - log.Println("failed to start transaction", err) - s.pages.Notice(w, "pull-reopen", "Failed to reopen pull.") + pull, ok := r.Context().Value("pull").(*db.Pull) + if !ok { + log.Println("failed to get pull") + s.pages.Notice(w, "pull-error", "Failed to edit patch. Try again later.") return } - pullId := chi.URLParam(r, "pull") - pullIdInt, err := strconv.Atoi(pullId) + // auth filter: only owner or collaborators can close + roles := RolesInRepo(s, user, f) + isCollaborator := roles.IsCollaborator() + isPullAuthor := user.Did == pull.OwnerDid + isCloseAllowed := isCollaborator || isPullAuthor + if !isCloseAllowed { + log.Println("failed to close pull") + s.pages.Notice(w, "pull-close", "You are unauthorized to close this pull.") + return + } + + // Start a transaction + tx, err := s.db.BeginTx(r.Context(), nil) if err != nil { - log.Println("failed to parse pull id", err) + log.Println("failed to start transaction", err) s.pages.Notice(w, "pull-reopen", "Failed to reopen pull.") return } // Reopen the pull in the database - err = db.ReopenPull(tx, f.RepoAt, pullIdInt) + err = db.ReopenPull(tx, f.RepoAt, pull.PullId) if err != nil { log.Println("failed to reopen pull", err) s.pages.Notice(w, "pull-reopen", "Failed to reopen pull.") @@ -1688,7 +1694,7 @@ func (s *State) ReopenPull(w http.ResponseWriter, r *http.Request) { return } - s.pages.HxLocation(w, fmt.Sprintf("/%s/pulls/%d", f.OwnerSlashRepo(), pullIdInt)) + s.pages.HxLocation(w, fmt.Sprintf("/%s/pulls/%d", f.OwnerSlashRepo(), pull.PullId)) return } @@ -1731,7 +1737,7 @@ func fullyResolvedRepo(r *http.Request) (*FullyResolvedRepo, error) { }, nil } -func rolesInRepo(s *State, u *auth.User, f *FullyResolvedRepo) pages.RolesInRepo { +func RolesInRepo(s *State, u *auth.User, f *FullyResolvedRepo) pages.RolesInRepo { if u != nil { r := s.enforcer.GetPermissionsInRepo(u.Did, f.Knot, f.OwnerSlashRepo()) return pages.RolesInRepo{r} diff --git a/appview/state/router.go b/appview/state/router.go index bdeb59bd..4a7a442e 100644 --- a/appview/state/router.go +++ b/appview/state/router.go @@ -30,7 +30,7 @@ func (s *State) UserRouter() http.Handler { r.With(ResolveIdent(s)).Route("/{user}", func(r chi.Router) { r.Get("/", s.ProfilePage) - r.With(ResolveRepoKnot(s)).Route("/{repo}", func(r chi.Router) { + r.With(ResolveRepo(s)).Route("/{repo}", func(r chi.Router) { r.Get("/", s.RepoIndex) r.Get("/commits/{ref}", s.RepoLog) r.Route("/tree/{ref}", func(r chi.Router) { @@ -58,17 +58,29 @@ func (s *State) UserRouter() http.Handler { r.Route("/pulls", func(r chi.Router) { r.Get("/", s.RepoPulls) - r.Get("/{pull}", s.RepoSinglePull) + r.With(AuthMiddleware(s)).Route("/new", func(r chi.Router) { + r.Get("/", s.NewPull) + r.Post("/", s.NewPull) + }) - r.Group(func(r chi.Router) { - r.Use(AuthMiddleware(s)) - r.Get("/new", s.NewPull) - r.Post("/new", s.NewPull) - r.Patch("/{pull}/patch", s.EditPatch) - r.Post("/{pull}/comment", s.PullComment) - r.Post("/{pull}/close", s.ClosePull) - r.Post("/{pull}/reopen", s.ReopenPull) - r.Post("/{pull}/merge", s.MergePull) + r.Route("/{pull}", func(r chi.Router) { + r.Use(ResolvePull(s)) + r.Get("/", s.RepoSinglePull) + + // authorized requests below this point + r.Group(func(r chi.Router) { + r.Use(AuthMiddleware(s)) + r.Patch("/patch", s.EditPatch) + r.Post("/comment", s.PullComment) + r.Post("/close", s.ClosePull) + r.Post("/reopen", s.ReopenPull) + // collaborators only + r.Group(func(r chi.Router) { + r.Use(RepoPermissionMiddleware(s, "repo:collaborator")) + r.Post("/merge", s.MergePull) + // maybe lock, etc. + }) + }) }) }) @@ -123,7 +135,7 @@ func (s *State) StandardRouter() http.Handler { r.Post("/init", s.InitKnotServer) r.Get("/", s.KnotServerInfo) r.Route("/member", func(r chi.Router) { - r.Use(RoleMiddleware(s, "server:owner")) + r.Use(KnotOwner(s)) r.Get("/", s.ListMembers) r.Put("/", s.AddMember) r.Delete("/", s.RemoveMember) diff --git a/flake.nix b/flake.nix index 0d71e314..807358ee 100644 --- a/flake.nix +++ b/flake.nix @@ -44,7 +44,7 @@ inherit (gitignore.lib) gitignoreSource; in { overlays.default = final: prev: let - goModHash = "sha256-k+WeNx9jZ5YGgskCJYiU2mwyz25E0bhFgSg2GDWZXFw="; + goModHash = "sha256-zJKjcxd+gr+9Kx2e1lUv+0hlXlxJm5YbWeIGUo0eIiE="; buildCmdPackage = name: final.buildGoModule { pname = name;