amadan

oficina / docs Public

Keep transient team uncertainty retryable through response classification

Paul Campbell pushed by paul@keymail.dev acbb192c0c5b27adc3de71c73c87d86013e4d21b parent dfd4f2b
5 files changed, +95 −12
  • internal/docs/share.go +19 −12
  • internal/docs/team_access.go +5 −0
  • internal/docs/team_access_test.go +63 −0
  • internal/docs/team_router_test.go +5 −0
  • internal/docs/team_stream.go +3 −0

CI passed — run details

diff --git a/internal/docs/share.go b/internal/docs/share.go
index ddfdd04..6bd3f01 100644
--- a/internal/docs/share.go
+++ b/internal/docs/share.go
@@ -75,7 +75,7 @@ func (h *handlers) shareGet(w http.ResponseWriter, r *http.Request) {
doc, err := h.store.Get(r.Context(), owner, sqid)
if err != nil {
- http.NotFound(w, r)
+ h.ownerLookupFailure(w, r, err)
return
}
grants, err := h.store.ListGrants(r.Context(), owner, sqid)
@@ -85,10 +85,10 @@ func (h *handlers) shareGet(w http.ResponseWriter, r *http.Request) {
return
}
- h.renderShare(w, r, doc, grants, "")
+ h.renderShare(w, r, doc, grants, "", http.StatusOK)
}
-func (h *handlers) renderShare(w http.ResponseWriter, r *http.Request, doc *Document, grants []*Grant, errKey string) {
+func (h *handlers) renderShare(w http.ResponseWriter, r *http.Request, doc *Document, grants []*Grant, errKey string, status int) {
// A failure to read the published state is logged and then rendered as
// "not published" rather than failing the page: the grant list above it
// is the part somebody came here for, and a 500 over the switch would
@@ -96,6 +96,10 @@ func (h *handlers) renderShare(w http.ResponseWriter, r *http.Request, doc *Docu
published, err := h.store.IsPublished(r.Context(), doc.OwnerID, doc.Sqid)
if err != nil {
h.logger.Error("docs: read published state", "sqid", doc.Sqid, "err", err)
+ if h.store.team != nil {
+ h.ownerLookupFailure(w, r, err)
+ return
+ }
}
// The URL appears only in the render that follows the publish that
@@ -152,7 +156,7 @@ func (h *handlers) renderShare(w http.ResponseWriter, r *http.Request, doc *Docu
if page.PublicURL != "" {
w.Header().Set("Cache-Control", "no-store")
}
- if err := h.render.page(w, r, "share", page); err != nil {
+ if err := h.render.pageStatus(w, r, "share", page, status); err != nil {
h.logger.Error("docs: render share", "sqid", doc.Sqid, "err", err)
http.Error(w, rastrillo.T(r, "docs.error.generic"), h.internalFailureStatus())
}
@@ -180,7 +184,7 @@ func (h *handlers) sharePost(w http.ResponseWriter, r *http.Request) {
doc, err := h.store.Get(r.Context(), owner, sqid)
if err != nil {
- http.NotFound(w, r)
+ h.ownerLookupFailure(w, r, err)
return
}
@@ -202,12 +206,7 @@ func (h *handlers) sharePost(w http.ResponseWriter, r *http.Request) {
http.Error(w, rastrillo.T(r, "docs.error.generic"), h.internalFailureStatus())
return
}
- // Content-Type before WriteHeader, or the header render.page
- // sets afterwards is dropped and the response works only by
- // sniffing.
- w.Header().Set("Content-Type", "text/html; charset=utf-8")
- w.WriteHeader(http.StatusBadRequest)
- h.renderShare(w, r, doc, grants, "docs.share.error.address")
+ h.renderShare(w, r, doc, grants, "docs.share.error.address", http.StatusBadRequest)
default:
h.logger.Error("docs: grant", "sqid", sqid, "err", err)
http.Error(w, rastrillo.T(r, "docs.error.generic"), h.internalFailureStatus())
@@ -232,7 +231,7 @@ func (h *handlers) shareRevoke(w http.ResponseWriter, r *http.Request) {
sqid := r.PathValue("sqid")
if _, err := h.store.Get(r.Context(), owner, sqid); err != nil {
- http.NotFound(w, r)
+ h.ownerLookupFailure(w, r, err)
return
}
if err := h.store.Revoke(r.Context(), owner, sqid, r.PostFormValue("address")); err != nil {
@@ -242,3 +241,11 @@ func (h *handlers) shareRevoke(w http.ResponseWriter, r *http.Request) {
}
http.Redirect(w, r, "/d/"+sqid+"/share", http.StatusSeeOther)
}
+
+func (h *handlers) ownerLookupFailure(w http.ResponseWriter, r *http.Request, err error) {
+ if h.store.team != nil && errors.Is(err, errTeamUnavailable) {
+ http.Error(w, rastrillo.T(r, "docs.error.generic"), http.StatusServiceUnavailable)
+ return
+ }
+ http.NotFound(w, r)
+}
diff --git a/internal/docs/team_access.go b/internal/docs/team_access.go
index cb427fe..6b409fb 100644
--- a/internal/docs/team_access.go
+++ b/internal/docs/team_access.go
@@ -12,6 +12,7 @@ import (
type accessContextKey struct{}
type requestAccess struct {
+ uncertain bool
responseStore *Store
responseDocs map[int64]permission
runtime *teamRuntime
@@ -39,7 +40,11 @@ func (a *requestAccess) state() roster.State {
if a == nil {
return roster.Unknown
}
+ if a.uncertain {
+ return roster.Unknown
+ }
if a.decision.State == roster.Member && !a.decision.Lease.Valid() {
+ a.uncertain = true
return roster.Unknown
}
return a.decision.State
diff --git a/internal/docs/team_access_test.go b/internal/docs/team_access_test.go
index b0618ea..af6b6db 100644
--- a/internal/docs/team_access_test.go
+++ b/internal/docs/team_access_test.go
@@ -11,6 +11,7 @@ import (
"github.com/carlosframework/docs/internal/docs/principalstore"
"github.com/carlosframework/docs/internal/docs/teamstate"
"github.com/carlosframework/rastrillo/migrate"
+ "github.com/carlosframework/rastrillo/sessions"
"gorm.io/gorm"
"io"
"net/http"
@@ -114,6 +115,7 @@ func TestTeamPermissionFormerOwnerOnlyReachesExactGrant(t *testing.T) {
}
type teamTestClock struct {
+ next func()
mu sync.Mutex
elapsed time.Duration
}
@@ -121,6 +123,11 @@ type teamTestClock struct {
func (c *teamTestClock) Now() (time.Time, time.Duration) {
c.mu.Lock()
defer c.mu.Unlock()
+ if c.next != nil {
+ fn := c.next
+ c.next = nil
+ fn()
+ }
return time.Unix(1800000000, 0).Add(c.elapsed), c.elapsed
}
func (c *teamTestClock) step(d time.Duration) { c.mu.Lock(); defer c.mu.Unlock(); c.elapsed += d }
@@ -389,3 +396,59 @@ func TestTeamMutationLeaseExpiryRollsBackEveryTable(t *testing.T) {
})
}
}
+
+func TestTeamPermissionContentionCannotRecoverIntoPermanentDenial(t *testing.T) {
+ f := newTeamRouterFixture(t)
+ b := f.browser()
+ b.localSignin(t, "alice@example.com")
+ b.linkProof(t, teamSubject('a'), "alice@example.com")
+ routerRedirect(t, b.do("POST", "/account/home/confirm", ""), "/")
+ session, ok := f.sessions.From(b.request("GET", "/", ""))
+ if !ok {
+ t.Fatal("session")
+ }
+ request := sessions.WithSession(b.request("GET", "/", ""), session)
+ id, e := f.app.store.team.bridge.resolver.resolve(request)
+ if e != nil {
+ t.Fatal(e)
+ }
+ a := f.app.store.team.resolveAccess(t.Context(), id)
+ if a.state() != roster.Member {
+ t.Fatal("member unavailable")
+ }
+ ctx := context.WithValue(t.Context(), accessContextKey{}, a)
+ entered, release, finished := make(chan struct{}), make(chan struct{}), make(chan struct{})
+ f.clock.mu.Lock()
+ f.clock.next = func() { close(entered); <-release }
+ f.clock.mu.Unlock()
+ go func() {
+ f.app.store.team.cache.Check(t.Context(), bridgeIssuer, "TeamA", teamSubject('a'))
+ close(finished)
+ }()
+ <-entered
+ // Cache.Check holds the real shared cache mutex while reading this clock.
+ scope := f.app.store.permissionScope(ctx, f.app.store.db.Model(&Document{}), id.Actor, permissionWrite)
+ close(release)
+ <-finished
+ if !a.decision.Lease.Valid() {
+ t.Fatal("fixture did not recover the same valid lease")
+ }
+ var found Document
+ e = scope.Where("id = ?", f.own.ID).First(&found).Error
+ if !errors.Is(e, gorm.ErrRecordNotFound) {
+ t.Fatal("contended ownership scope did not refuse", e)
+ }
+ if e = f.app.store.permissionError(ctx, id.Actor, e); !errors.Is(e, errTeamUnavailable) {
+ t.Fatal("transient ownership refusal became permanent after lease recovered", e)
+ }
+ if e = f.app.store.checkPermission(ctx, f.app.store.db, id.Actor, f.shared.ID, permissionWrite); e != nil {
+ t.Fatal("sticky uncertainty revoked an independent writer grant", e)
+ }
+ if a.state() != roster.Unknown {
+ t.Fatal("request regained uncertain ownership standing")
+ }
+ fresh := f.app.store.team.resolveAccess(t.Context(), id)
+ if fresh.state() != roster.Member {
+ t.Fatal("uncertainty leaked into a new request")
+ }
+}
diff --git a/internal/docs/team_router_test.go b/internal/docs/team_router_test.go
index 154eafc..1f05d09 100644
--- a/internal/docs/team_router_test.go
+++ b/internal/docs/team_router_test.go
@@ -218,6 +218,11 @@ func TestTeamRouterLocalLinkMembershipRemovalAndIndependentGrant(t *testing.T) {
if e := f.app.store.team.cache.Refresh(t.Context()); e == nil {
t.Fatal("directory failure accepted")
}
+ for _, path := range []string{"/d/" + f.own.Sqid + "/share", "/d/nonexistent/share"} {
+ if r := b.do("GET", path, ""); r.Code != 503 {
+ t.Fatal("unknown owner administration became permanent or leaked existence", r.Code)
+ }
+ }
missing := b.do("GET", "/d/nonexistent", "")
private := b.do("GET", "/d/"+f.own.Sqid, "")
if missing.Code != 503 || private.Code != 503 || missing.Body.String() != private.Body.String() {
diff --git a/internal/docs/team_stream.go b/internal/docs/team_stream.go
index 633d2e6..29d07cf 100644
--- a/internal/docs/team_stream.go
+++ b/internal/docs/team_stream.go
@@ -18,6 +18,9 @@ type teamStreamGuard struct {
}
func (g *teamStreamGuard) remaining() time.Duration {
+ if g.access.uncertain {
+ return 0
+ }
if g.access.decision.State == roster.Member {
if g.deadline.IsZero() {
return 0