diff --git a/pkg/appview/authgate/push_authorizer.go b/pkg/appview/authgate/push_authorizer.go index a5772df..a55cab1 100644 --- a/pkg/appview/authgate/push_authorizer.go +++ b/pkg/appview/authgate/push_authorizer.go @@ -118,7 +118,53 @@ func (a *Authorizer) Authorize(ctx context.Context, did, authMethod string, acce } } - return a.checkQuota(ctx, did, holdDID) + if err := a.checkQuota(ctx, did, holdDID); err != nil { + // Over quota. Denying outright would also deny the delete our own + // error text tells the user to perform, and OCI clients bundle the + // two (docker and crane both request pull,push,delete for a delete), + // so an over-quota user has no way to free space. Grant the non-push + // subset when delete was asked for. A plain push is still denied, so + // the quota message still reaches the client that needs to see it. + if dropPushForDelete(access) { + slog.Info("push gate: over quota, granting delete without push", + "did", did, "hold_did", holdDID, "reason", err) + return nil + } + return err + } + return nil +} + +// dropPushForDelete removes the push action from every repository entry when +// the request also asks for delete, reporting whether it did. +// +// The narrowing is in place because the caller passes this same slice to the +// JWT issuer, so the reduced action set is what the token actually grants. +// See the Authorizer contract in pkg/auth/token. +func dropPushForDelete(access []auth.AccessEntry) bool { + requestsDelete := false + for _, entry := range access { + if entry.Type == "repository" && entry.Name != "*" && slices.Contains(entry.Actions, "delete") { + requestsDelete = true + break + } + } + if !requestsDelete { + return false + } + + for i := range access { + if access[i].Type != "repository" { + continue + } + // Clone first: DeleteFunc compacts in place, so filtering the stored + // slice directly would scramble any other holder of that same array. + access[i].Actions = slices.DeleteFunc( + slices.Clone(access[i].Actions), + func(action string) bool { return action == "push" }, + ) + } + return true } // isCaptain returns true if userDID owns holdDID per the local Jetstream- diff --git a/pkg/appview/authgate/push_authorizer_test.go b/pkg/appview/authgate/push_authorizer_test.go index d846435..711c1ee 100644 --- a/pkg/appview/authgate/push_authorizer_test.go +++ b/pkg/appview/authgate/push_authorizer_test.go @@ -2,6 +2,7 @@ package authgate import ( "context" + "slices" "strings" "testing" @@ -444,6 +445,102 @@ func TestAuthorize_NonCaptainPushOverQuotaDenied(t *testing.T) { } } +// An over-quota user has to be able to delete: the denial message tells them +// to, and docker/crane ask for pull,push,delete on a delete. The gate grants +// the non-push subset instead of failing the whole request. +func TestAuthorize_OverQuotaGrantsDeleteWithoutPush(t *testing.T) { + atproto.SetTestMode(true) + t.Cleanup(func() { atproto.SetTestMode(false) }) + + srv := quotaServer(t, 200, `{"totalSize":1000,"limit":1000}`) + d := newTestDB(t) + seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) + seedCaptain(t, d, srv.holdDID, "did:plc:alice") + seedCrewMember(t, d, srv.holdDID, "did:plc:bob", `["blob:write"]`) + + a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + access := []auth.AccessEntry{ + {Type: "repository", Name: "bob/x", Actions: []string{"pull", "push", "delete"}}, + } + if err := a.Authorize(context.Background(), "did:plc:bob", "", access); err != nil { + t.Fatalf("Authorize(over quota, delete requested) = %v, want nil", err) + } + if got := access[0].Actions; !slices.Equal(got, []string{"pull", "delete"}) { + t.Errorf("granted actions = %v, want [pull delete]", got) + } +} + +// Delete-only never carries push, so it must survive untouched. +func TestAuthorize_OverQuotaAllowsDeleteOnlyScope(t *testing.T) { + atproto.SetTestMode(true) + t.Cleanup(func() { atproto.SetTestMode(false) }) + + srv := quotaServer(t, 200, `{"totalSize":1000,"limit":1000}`) + d := newTestDB(t) + seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) + seedCaptain(t, d, srv.holdDID, "did:plc:alice") + + a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + access := []auth.AccessEntry{ + {Type: "repository", Name: "bob/x", Actions: []string{"delete"}}, + } + if err := a.Authorize(context.Background(), "did:plc:bob", "", access); err != nil { + t.Fatalf("Authorize(delete only) = %v, want nil", err) + } + if got := access[0].Actions; !slices.Equal(got, []string{"delete"}) { + t.Errorf("granted actions = %v, want [delete]", got) + } + if srv.hits != 0 { + t.Errorf("quota endpoint hit %d times for delete-only scope, want 0", srv.hits) + } +} + +// A plain push must keep failing loudly, otherwise the client never sees the +// quota message and just gets an opaque 401 on the first blob upload. +func TestAuthorize_OverQuotaStillDeniesPlainPush(t *testing.T) { + atproto.SetTestMode(true) + t.Cleanup(func() { atproto.SetTestMode(false) }) + + srv := quotaServer(t, 200, `{"totalSize":1000,"limit":1000}`) + d := newTestDB(t) + seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) + seedCaptain(t, d, srv.holdDID, "did:plc:alice") + seedCrewMember(t, d, srv.holdDID, "did:plc:bob", `["blob:write"]`) + + a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + access := pushAccess("bob/x") + err := a.Authorize(context.Background(), "did:plc:bob", "", access) + if err == nil || !strings.Contains(err.Error(), "quota exceeded") { + t.Fatalf("expected 'quota exceeded', got %v", err) + } + if got := access[0].Actions; !slices.Equal(got, []string{"pull", "push"}) { + t.Errorf("denied request should leave actions untouched, got %v", got) + } +} + +// Under quota, a delete request keeps its push action. +func TestAuthorize_UnderQuotaKeepsPushAlongsideDelete(t *testing.T) { + atproto.SetTestMode(true) + t.Cleanup(func() { atproto.SetTestMode(false) }) + + srv := quotaServer(t, 200, `{"totalSize":1,"limit":1000}`) + d := newTestDB(t) + seedUser(t, d, "did:plc:bob", "bob.test", srv.holdDID) + seedCaptain(t, d, srv.holdDID, "did:plc:alice") + seedCrewMember(t, d, srv.holdDID, "did:plc:bob", `["blob:write"]`) + + a := New(d, fakeHoldAuthorizer{}, nil, "", WithHTTPClient(srv.httpClient())) + access := []auth.AccessEntry{ + {Type: "repository", Name: "bob/x", Actions: []string{"pull", "push", "delete"}}, + } + if err := a.Authorize(context.Background(), "did:plc:bob", "", access); err != nil { + t.Fatalf("Authorize(under quota) = %v, want nil", err) + } + if got := access[0].Actions; !slices.Equal(got, []string{"pull", "push", "delete"}) { + t.Errorf("granted actions = %v, want all three preserved", got) + } +} + func TestAuthorize_PullOnlySkipsMembershipAndQuota(t *testing.T) { atproto.SetTestMode(true) t.Cleanup(func() { atproto.SetTestMode(false) }) diff --git a/pkg/auth/token/handler.go b/pkg/auth/token/handler.go index d53fc7d..ae5d86d 100644 --- a/pkg/auth/token/handler.go +++ b/pkg/auth/token/handler.go @@ -45,6 +45,13 @@ type Authorizer interface { // allowed, or an error describing the denial reason. The error message // surfaces to the OCI client inside the distribution error JSON body. // + // Implementations may narrow `access` in place (dropping actions from an + // entry) to grant a subset rather than deny the whole request, and the + // issued JWT carries whatever survives. They must not widen it: every + // entry has already cleared ValidateAccess. The handler reads `access` + // only after draining the gate, so the narrowing is ordered before the + // JWT is signed — keep it that way if this call site moves. + // // authMethod is one of AuthMethodOAuth or AuthMethodAppPassword and lets // the implementation pick the right service-token fetcher when it needs // to talk to the hold (e.g. for crew reconciliation).