Skip to content

Commit 11f0bb4

Browse files
committed
fix(scim): replace the 1000-group hard cap with real pagination + a safety valve
The prior fix (a hard LIMIT/queryEqLimit cap at 1000) traded unbounded scan cost for silent incorrectness: an org past the cap could get a false "not found" on GetScimGroupByOrgAndDisplayName / GetScimGroupByOrgAndExternalID, which breaks CreateGroup's dedup (creates a duplicate), the rename-uniqueness gate (lets a duplicate name through), and the displayName filter (404s a group that exists). Not hypothetical - large enterprises can plausibly sync 1000+ fine-grained IdP groups into one org. Cassandra: drop the CQL LIMIT entirely (it truncates the combined result set across every page, reintroducing the same silent-drop bug server-side instead of client-side). gocql's Scanner already pages through the full ALLOW FILTERING result set lazily as Next() is called, so a normal lookup runs to full exhaustion or an early match. groupScanSafetyCap (100k) now bounds the Go-side examined-item counter instead, purely as a circuit-breaker against a pathologically large partition, not a realistic ceiling. DynamoDB: new queryEqUntil() helper pages through the org_id GSI (like the existing unbounded queryEq) but stops as soon as the caller's match predicate reports a hit, instead of materializing every page before filtering, and carries the same 100k safety valve. Both GetScimGroupByOrgAndDisplayName and GetScimGroupByOrgAndExternalID (both previously capped at groupScanCap=1000) now use it. The narrower queryEqLimit helper is untouched - it's still correct for its many other exact-match, limit=1 callers. Verified against real ScyllaDB and DynamoDB (make test-scylladb, make test-dynamodb), not just SQLite: 31 packages, 0 failures, each.
1 parent dafa013 commit 11f0bb4

3 files changed

Lines changed: 109 additions & 23 deletions

File tree

internal/storage/db/cassandradb/scim_group.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,15 @@ import (
1414

1515
const scimGroupColumns = "id, org_id, display_name, external_id, created_at, updated_at"
1616

17+
// groupScanSafetyCap bounds the org-scoped scan-and-compare-in-app
18+
// displayName lookup below. It is NOT a realistic ceiling on an org's group
19+
// count — gocql's Scanner already pages through the full ALLOW FILTERING
20+
// result set as Next() is called, so a normal lookup runs to full exhaustion
21+
// (or an early match) regardless of how many pages that takes. This exists
22+
// purely as a circuit-breaker against unbounded work on a pathologically
23+
// large partition, mirroring DynamoDB's groupScanSafetyCap.
24+
const groupScanSafetyCap = 100_000
25+
1726
// scanScimGroup maps the scimGroupColumns projection onto a struct.
1827
func scanScimGroup(scan func(...interface{}) error, group *schemas.ScimGroup) error {
1928
return scan(&group.ID, &group.OrgID, &group.DisplayName, &group.ExternalID, &group.CreatedAt, &group.UpdatedAt)
@@ -91,8 +100,15 @@ func (p *provider) GetScimGroupByID(ctx context.Context, id string) (*schemas.Sc
91100
// query to the org and compare displayName with strings.EqualFold in Go (an
92101
// org's group set is small), mirroring the DynamoDB fetch-then-filter shape.
93102
func (p *provider) GetScimGroupByOrgAndDisplayName(ctx context.Context, orgID, displayName string) (*schemas.ScimGroup, error) {
103+
// No LIMIT here deliberately: a CQL LIMIT truncates the combined result set
104+
// across every page, which would silently drop a match beyond it — the
105+
// exact bug groupScanSafetyCap exists to avoid. gocql's Scanner pages
106+
// through ALLOW FILTERING lazily as Next() is called, so leaving LIMIT off
107+
// lets a real match anywhere in the org's group set still be found; the
108+
// safety cap below bounds the Go-side loop instead, not the CQL query.
94109
query := fmt.Sprintf("SELECT %s FROM %s WHERE org_id = ? ALLOW FILTERING", scimGroupColumns, KeySpace+"."+schemas.Collections.ScimGroup)
95110
scanner := p.db.Query(query, orgID).Consistency(gocql.One).Iter().Scanner()
111+
examined := 0
96112
for scanner.Next() {
97113
var group schemas.ScimGroup
98114
if err := scanScimGroup(scanner.Scan, &group); err != nil {
@@ -101,6 +117,12 @@ func (p *provider) GetScimGroupByOrgAndDisplayName(ctx context.Context, orgID, d
101117
if strings.EqualFold(group.DisplayName, displayName) {
102118
return &group, nil
103119
}
120+
examined++
121+
if examined >= groupScanSafetyCap {
122+
p.dependencies.Log.Warn().Str("org_id", orgID).Int("examined", examined).
123+
Msg("GetScimGroupByOrgAndDisplayName: hit the scan safety cap without a match")
124+
return nil, gocql.ErrNotFound
125+
}
104126
}
105127
if err := scanner.Err(); err != nil {
106128
return nil, err

internal/storage/db/dynamodb/ops.go

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,59 @@ func (p *provider) queryEqLimit(ctx context.Context, table, indexName, pkAttr, p
211211
return res.Items, nil
212212
}
213213

214+
// queryEqUntil pages through the pkAttr=pkVal partition (like queryEq) but
215+
// stops as soon as match reports a hit, instead of materializing every page
216+
// before filtering. maxItems is a defensive circuit-breaker against unbounded
217+
// scan cost on a pathologically large partition — NOT a realistic ceiling;
218+
// normal callers exhaust the partition long before reaching it. match may
219+
// return an error (e.g. a decode failure), which aborts the scan immediately
220+
// rather than being swallowed as a non-match; on a hit it is responsible for
221+
// capturing whatever it needs from the item (queryEqUntil only reports found/
222+
// not-found, it does not return the raw item — callers already decode it
223+
// inside match to test it, so returning it again would be redundant).
224+
func (p *provider) queryEqUntil(ctx context.Context, table, indexName, pkAttr, pkVal string, maxItems int, match func(map[string]types.AttributeValue) (bool, error)) (bool, error) {
225+
kc := expression.Key(pkAttr).Equal(expression.Value(pkVal))
226+
expr, err := expression.NewBuilder().WithKeyCondition(kc).Build()
227+
if err != nil {
228+
return false, err
229+
}
230+
var start map[string]types.AttributeValue
231+
examined := 0
232+
for {
233+
in := &dynamodb.QueryInput{
234+
TableName: aws.String(table),
235+
IndexName: aws.String(indexName),
236+
KeyConditionExpression: expr.KeyCondition(),
237+
ExpressionAttributeNames: expr.Names(),
238+
ExpressionAttributeValues: expr.Values(),
239+
ExclusiveStartKey: start,
240+
}
241+
res, err := p.client.Query(ctx, in)
242+
if err != nil {
243+
return false, err
244+
}
245+
for _, it := range res.Items {
246+
ok, err := match(it)
247+
if err != nil {
248+
return false, err
249+
}
250+
if ok {
251+
return true, nil
252+
}
253+
examined++
254+
if examined >= maxItems {
255+
p.dependencies.Log.Warn().Str("table", table).Str("partition_key", pkVal).Int("examined", examined).
256+
Msg("queryEqUntil: hit the scan safety cap without a match")
257+
return false, nil
258+
}
259+
}
260+
if res.LastEvaluatedKey == nil {
261+
return false, nil
262+
}
263+
start = res.LastEvaluatedKey
264+
}
265+
}
266+
214267
func (p *provider) scanFilteredLimit(ctx context.Context, table string, index *string, filter *expression.ConditionBuilder, limit int32) ([]map[string]types.AttributeValue, error) {
215268
in := &dynamodb.ScanInput{
216269
TableName: aws.String(table),

internal/storage/db/dynamodb/scim_group.go

Lines changed: 34 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -7,16 +7,17 @@ import (
77
"strings"
88
"time"
99

10+
"github.com/aws/aws-sdk-go-v2/service/dynamodb/types"
1011
"github.com/google/uuid"
1112

1213
"github.com/authorizerdev/authorizer/internal/storage/schemas"
1314
)
1415

15-
// groupScanCap bounds the org_id query used to resolve a group by displayName.
16-
// An org's group count is small; this closes over the realistic range without
17-
// paginating.
18-
// ponytail: cap at 1000; add pagination if an org ever exceeds it.
19-
const groupScanCap = 1000
16+
// groupScanSafetyCap bounds the org-scoped scan-and-compare-in-app lookups
17+
// below (queryEqUntil). It is NOT a realistic ceiling on an org's group
18+
// count — normal lookups page through the whole partition — it exists purely
19+
// as a circuit-breaker against unbounded work on a pathologically large one.
20+
const groupScanSafetyCap = 100_000
2021

2122
// AddScimGroup creates a new SCIM group record.
2223
func (p *provider) AddScimGroup(ctx context.Context, group *schemas.ScimGroup) (*schemas.ScimGroup, error) {
@@ -76,20 +77,25 @@ func (p *provider) GetScimGroupByID(ctx context.Context, id string) (*schemas.Sc
7677
// caseExact:false (RFC 7644 §3.4.2.2), and a DynamoDB GSI lookup is exact-match
7778
// only, so the case-fold happens here in Go with strings.EqualFold.
7879
func (p *provider) GetScimGroupByOrgAndDisplayName(ctx context.Context, orgID, displayName string) (*schemas.ScimGroup, error) {
79-
items, err := p.queryEqLimit(ctx, schemas.Collections.ScimGroup, "org_id", "org_id", orgID, nil, groupScanCap)
80-
if err != nil {
81-
return nil, err
82-
}
83-
for _, it := range items {
80+
var found schemas.ScimGroup
81+
ok, err := p.queryEqUntil(ctx, schemas.Collections.ScimGroup, "org_id", "org_id", orgID, groupScanSafetyCap, func(it map[string]types.AttributeValue) (bool, error) {
8482
var group schemas.ScimGroup
8583
if err := unmarshalItem(it, &group); err != nil {
86-
return nil, err
84+
return false, err
8785
}
88-
if strings.EqualFold(group.DisplayName, displayName) {
89-
return &group, nil
86+
if !strings.EqualFold(group.DisplayName, displayName) {
87+
return false, nil
9088
}
89+
found = group
90+
return true, nil
91+
})
92+
if err != nil {
93+
return nil, err
94+
}
95+
if !ok {
96+
return nil, errors.New("no document found")
9197
}
92-
return nil, errors.New("no document found")
98+
return &found, nil
9399
}
94100

95101
// GetScimGroupByOrgAndExternalID resolves the single group with the given
@@ -98,18 +104,23 @@ func (p *provider) GetScimGroupByOrgAndDisplayName(ctx context.Context, orgID, d
98104
// and match in-app (an org's group set is small).
99105
func (p *provider) GetScimGroupByOrgAndExternalID(ctx context.Context, orgID, externalID string) (*schemas.ScimGroup, error) {
100106
want := orgID + ":" + externalID
101-
items, err := p.queryEqLimit(ctx, schemas.Collections.ScimGroup, "org_id", "org_id", orgID, nil, groupScanCap)
102-
if err != nil {
103-
return nil, err
104-
}
105-
for _, it := range items {
107+
var found schemas.ScimGroup
108+
ok, err := p.queryEqUntil(ctx, schemas.Collections.ScimGroup, "org_id", "org_id", orgID, groupScanSafetyCap, func(it map[string]types.AttributeValue) (bool, error) {
106109
var group schemas.ScimGroup
107110
if err := unmarshalItem(it, &group); err != nil {
108-
return nil, err
111+
return false, err
109112
}
110-
if group.ExternalID != nil && *group.ExternalID == want {
111-
return &group, nil
113+
if group.ExternalID == nil || *group.ExternalID != want {
114+
return false, nil
112115
}
116+
found = group
117+
return true, nil
118+
})
119+
if err != nil {
120+
return nil, err
121+
}
122+
if !ok {
123+
return nil, errors.New("no document found")
113124
}
114-
return nil, errors.New("no document found")
125+
return &found, nil
115126
}

0 commit comments

Comments
 (0)