diff --git a/golink.go b/golink.go index 04eed71..61dc5b1 100644 --- a/golink.go +++ b/golink.go @@ -605,8 +605,8 @@ func serveDelete(w http.ResponseWriter, r *http.Request) { return } - if link.Owner != login { - http.Error(w, "cannot delete link owned by another user", http.StatusForbidden) + if err := checkLinkOwnership(r.Context(), link, login); err != nil { + http.Error(w, fmt.Sprintf("cannot delete link: %v", err), http.StatusForbidden) return } @@ -653,17 +653,9 @@ func serveSave(w http.ResponseWriter, r *http.Request) { http.Error(w, err.Error(), http.StatusInternalServerError) } - if link != nil && link.Owner != "" && link.Owner != login { - exists, err := userExists(r.Context(), link.Owner) - if err != nil { - log.Printf("looking up tailnet user %q: %v", link.Owner, err) - } - // Don't allow taking over links if the owner account still exists - // or if we're unsure because an error occurred. - if exists || err != nil { - http.Error(w, "not your link; owned by "+link.Owner, http.StatusForbidden) - return - } + if err := checkLinkOwnership(r.Context(), link, login); err != nil { + http.Error(w, fmt.Sprintf("cannot update link: %v", err), http.StatusForbidden) + return } // allow transferring ownership to valid users. If empty, set owner to current user. @@ -705,6 +697,23 @@ func serveSave(w http.ResponseWriter, r *http.Request) { } } +func checkLinkOwnership(ctx context.Context, link *Link, login string) error { + if link == nil || link.Owner == "" { + return nil + } + + linkOwnerExists, err := userExists(ctx, link.Owner) + if err != nil { + log.Printf("looking up tailnet user %q: %v", link.Owner, err) + } + // Don't allow deleting or updating links if the owner account still exists + // or if we're unsure because an error occurred. + if (linkOwnerExists && link.Owner != login) || err != nil { + return fmt.Errorf("link owned by user %q", link.Owner) + } + return nil +} + // serveExport prints a snapshot of the link database. Links are JSON encoded // and printed one per line. This format is used to restore link snapshots on // startup. diff --git a/golink_test.go b/golink_test.go index 94c7823..14a6006 100644 --- a/golink_test.go +++ b/golink_test.go @@ -124,6 +124,8 @@ func TestServeSave(t *testing.T) { t.Fatal(err) } + db.Save(&Link{Short: "link-owned-by-tagged-devices", Long: "/before", Owner: "tagged-devices"}) + tests := []struct { name string short string @@ -157,6 +159,13 @@ func TestServeSave(t *testing.T) { currentUser: func(*http.Request) (string, error) { return "bar@example.com", nil }, wantStatus: http.StatusForbidden, }, + { + name: "allow editing link owned by tagged-devices", + short: "link-owned-by-tagged-devices", + long: "/after", + currentUser: func(*http.Request) (string, error) { return "bar@example.com", nil }, + wantStatus: http.StatusOK, + }, { name: "disallow unknown users", short: "who2", @@ -211,6 +220,7 @@ func TestServeDelete(t *testing.T) { } db.Save(&Link{Short: "a", Owner: "a@example.com"}) db.Save(&Link{Short: "foo", Owner: "foo@example.com"}) + db.Save(&Link{Short: "link-owned-by-tagged-devices", Long: "/before", Owner: "tagged-devices"}) xsrf := func(short string) string { return xsrftoken.Generate(xsrfKey, "foo@example.com", short) @@ -238,6 +248,12 @@ func TestServeDelete(t *testing.T) { short: "a", wantStatus: http.StatusForbidden, }, + { + name: "allow deleting link owned by tagged-devices", + short: "link-owned-by-tagged-devices", + xsrf: xsrf("link-owned-by-tagged-devices"), + wantStatus: http.StatusOK, + }, { name: "invalid xsrf", short: "foo",