diff --git a/README.md b/README.md index 0ead4789..c45c0591 100644 --- a/README.md +++ b/README.md @@ -132,6 +132,12 @@ the connector's dominant cost. It is paginated rather than cached so memory stays bounded by one page regardless of server size; disgo's rate limiter handles the resulting request volume. +Channel grants additionally look up each member named by a channel permission +overwrite, because Discord keeps those overwrites after the member leaves and a +grant for a departed user would point at a principal the sync never emitted. +Member overwrites are uncommon next to role overwrites, so this is a small +number of extra requests per channel. + ## Known limitations **Concurrent channel permission changes.** Discord replaces a permission @@ -149,10 +155,17 @@ data is correct — but the parent recorded for such an account depends on sync order and should not be treated as "the" server for that account. Server membership is authoritatively the `access` grant on each server. +Fields on the account are deliberately account-scoped rather than per-server for +this reason: `created_at` is the account's creation time, decoded from the +snowflake, rather than the per-server join date, and no server ID is recorded on +the profile. + **Rate limiting is handled inside disgo** rather than surfaced to the Baton SDK, so a sync cannot pace itself against Discord's rate-limit headers or checkpoint on a 429. disgo blocks and retries, which is correct but opaque to -the syncer. +the syncer. Failures that do surface carry a gRPC status code, so a permission +refusal is distinguishable from a transient one and the SDK's provisioning +retryer can act on the difference. # Contributing, Support and Issues diff --git a/go.mod b/go.mod index e9a6ce8d..31762af7 100644 --- a/go.mod +++ b/go.mod @@ -10,6 +10,7 @@ require ( github.com/grpc-ecosystem/go-grpc-middleware v1.4.0 github.com/quasilyte/go-ruleguard/dsl v0.3.23 go.uber.org/zap v1.28.0 + google.golang.org/grpc v1.83.0 ) require ( @@ -46,7 +47,7 @@ require ( github.com/cockroachdb/logtags v0.0.0-20230118201751-21c54148d20b // indirect github.com/cockroachdb/pebble/v2 v2.1.5 // indirect github.com/cockroachdb/redact v1.1.5 // indirect - github.com/cockroachdb/swiss v0.0.0-20251224182025-b0f6560f979b // indirect + github.com/cockroachdb/swiss v0.0.0-20260820225851-333444432258 // indirect github.com/cockroachdb/tokenbucket v0.0.0-20230807174530-cc333fc44b06 // indirect github.com/conductorone/dpop v0.2.6 // indirect github.com/conductorone/dpop/integrations/dpop_grpc v0.2.4 // indirect @@ -137,7 +138,6 @@ require ( golang.org/x/text v0.40.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20260526163538-3dc84a4a5aaa // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20260729162451-8efbd57d26e0 // indirect - google.golang.org/grpc v1.83.0 // indirect google.golang.org/protobuf v1.36.11 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect diff --git a/go.sum b/go.sum index bb65edf4..d1cb00f0 100644 --- a/go.sum +++ b/go.sum @@ -80,8 +80,8 @@ github.com/cockroachdb/pebble/v2 v2.1.5 h1:1ziHpaSau6qCXnFpQX3EBOH14yPHA8W66vKxs github.com/cockroachdb/pebble/v2 v2.1.5/go.mod h1:Reo1RTniv1UjVTAu/Fv74y5i3kJ5gmVrPhO9UtFiKn8= github.com/cockroachdb/redact v1.1.5 h1:u1PMllDkdFfPWaNGMyLD1+so+aq3uUItthCFqzwPJ30= github.com/cockroachdb/redact v1.1.5/go.mod h1:BVNblN9mBWFyMyqK1k3AAiSxhvhfK2oOZZ2lK+dpvRg= -github.com/cockroachdb/swiss v0.0.0-20251224182025-b0f6560f979b h1:VXvSNzmr8hMj8XTuY0PT9Ane9qZGul/p67vGYwl9BFI= -github.com/cockroachdb/swiss v0.0.0-20251224182025-b0f6560f979b/go.mod h1:yBRu/cnL4ks9bgy4vAASdjIW+/xMlFwuHKqtmh3GZQg= +github.com/cockroachdb/swiss v0.0.0-20260820225851-333444432258 h1:IJ+uNItEm0qx9FE2AgIc1PMsCUtk8nbSIzhQE1t5GWw= +github.com/cockroachdb/swiss v0.0.0-20260820225851-333444432258/go.mod h1:yBRu/cnL4ks9bgy4vAASdjIW+/xMlFwuHKqtmh3GZQg= github.com/cockroachdb/tokenbucket v0.0.0-20230807174530-cc333fc44b06 h1:zuQyyAKVxetITBuuhv3BI9cMrmStnpT18zmgmTxunpo= github.com/cockroachdb/tokenbucket v0.0.0-20230807174530-cc333fc44b06/go.mod h1:7nc4anLGjupUW/PeY5qiNYsdNXj7zopG+eqsS7To5IQ= github.com/conductorone/baton-sdk v0.24.6 h1:mORfZrBdsxXSYqZxlGMEQTFf6I2fu2/PBF+0c7a73KU= diff --git a/pkg/client/client.go b/pkg/client/client.go index 981597f5..8ff75d5b 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -8,13 +8,18 @@ import ( "context" "errors" "fmt" + "log/slog" "net/http" + "os" "strings" "time" "github.com/disgoorg/disgo/discord" "github.com/disgoorg/disgo/rest" "github.com/disgoorg/snowflake/v2" + "google.golang.org/grpc/codes" + + "github.com/conductorone/baton-sdk/pkg/uhttp" ) // Discord's maximum page sizes for the cursor-paginated collections used here. @@ -46,7 +51,18 @@ func New(ctx context.Context, token string, baseURL string) (*Client, error) { } httpClient := &http.Client{Timeout: requestTimeout} - opts := []rest.ClientConfigOpt{rest.WithHTTPClient(httpClient)} + opts := []rest.ClientConfigOpt{ + rest.WithHTTPClient(httpClient), + // Pin the logger rather than inheriting slog.Default(). At debug level + // disgo logs every request and response body verbatim, which includes + // the create-invite response carrying the invite code that + // guildBuilder.Grant deliberately keeps out of its errors. Flooring the + // level here makes that guarantee a property of this client instead of + // an accident of whatever the process default happens to be. + rest.WithLogger(slog.New(slog.NewJSONHandler(os.Stderr, &slog.HandlerOptions{ + Level: slog.LevelInfo, + }))), + } if baseURL != "" { // disgo addresses the API through a single configurable base, so a test @@ -80,18 +96,62 @@ func (c *Client) Close() error { func parseID(kind string, id string) (snowflake.ID, error) { parsed, err := snowflake.Parse(id) if err != nil { - return 0, fmt.Errorf("baton-discord: %s ID %q is not a valid Discord snowflake: %w", kind, id, err) + return 0, uhttp.WrapErrors(codes.InvalidArgument, + fmt.Sprintf("baton-discord: %s ID %q is not a valid Discord snowflake", kind, id), err) } return parsed, nil } +// statusCodeFor maps a Discord HTTP status onto the gRPC code the Baton SDK +// reasons about. +// +// Without this every failure reaches the SDK as codes.Unknown, with two +// consequences: C1 cannot tell a 403 role-hierarchy refusal from a transient +// blip, and the provisioning retryer never fires, because it retries only +// Unavailable and DeadlineExceeded. +// +// 5xx maps to Unavailable rather than Internal for exactly that reason — +// Internal is not in the retry set, so classifying a transient upstream failure +// as Internal would describe it accurately and still never retry it. +func statusCodeFor(err error) codes.Code { + statusCode := httpStatus(err) + switch statusCode { + case http.StatusBadRequest: + return codes.InvalidArgument + case http.StatusUnauthorized: + return codes.Unauthenticated + case http.StatusForbidden: + return codes.PermissionDenied + case http.StatusNotFound: + return codes.NotFound + case http.StatusTooManyRequests: + // disgo blocks and retries rate limits internally, so a 429 reaching + // here means it gave up rather than that we raced one request. + return codes.ResourceExhausted + } + if statusCode >= 500 && statusCode < 600 { + return codes.Unavailable + } + return codes.Unknown +} + +// wrapErr annotates a Discord failure with its gRPC status code while keeping +// the original error unwrappable, so IsNotFound and friends still work. +func wrapErr(err error, format string, args ...any) error { + if err == nil { + return nil + } + msg := fmt.Sprintf(format, args...) + return uhttp.WrapErrors(statusCodeFor(err), msg, fmt.Errorf("%s: %w", msg, err)) +} + // CurrentUser returns the bot's own user, the cheapest proof that a token is // valid. Passing an empty bearer token leaves the client's bot authorization in // place. func (c *Client) CurrentUser(ctx context.Context) (*discord.OAuth2User, error) { user, err := c.rest.GetCurrentUser("", rest.WithCtx(ctx)) if err != nil { - return nil, fmt.Errorf("baton-discord: failed to identify the bot: %w", err) + return nil, wrapErr(err, "baton-discord: failed to identify the bot") } return user, nil } @@ -104,7 +164,7 @@ func (c *Client) Guild(ctx context.Context, guildID string) (*discord.RestGuild, } guild, err := c.rest.GetGuild(id, false, rest.WithCtx(ctx)) if err != nil { - return nil, fmt.Errorf("baton-discord: failed to get guild %s: %w", guildID, err) + return nil, wrapErr(err, "baton-discord: failed to get guild %s", guildID) } return guild, nil } @@ -126,7 +186,7 @@ func (c *Client) GuildsPage(ctx context.Context, after string) ([]discord.OAuth2 guilds, err := c.rest.GetCurrentUserGuilds("", 0, afterID, GuildPageSize, false, rest.WithCtx(ctx)) if err != nil { - return nil, "", fmt.Errorf("baton-discord: failed to list guilds: %w", err) + return nil, "", wrapErr(err, "baton-discord: failed to list guilds") } next := "" @@ -163,7 +223,7 @@ func (c *Client) MembersPage(ctx context.Context, guildID string, after string) "baton-discord: not allowed to list members of guild %s; enable the "+ "Server Members Intent on the bot application: %w", guildID, err) } - return nil, "", fmt.Errorf("baton-discord: failed to list members of guild %s: %w", guildID, err) + return nil, "", wrapErr(err, "baton-discord: failed to list members of guild %s", guildID) } // A member with no user is a misconfiguration, not a member to skip. @@ -187,6 +247,29 @@ func (c *Client) MembersPage(ctx context.Context, guildID string, after string) return members, next, nil } +// Member returns one guild member. The second result is false when the user is +// not a member of the guild, which Discord reports as a 404. +func (c *Client) Member(ctx context.Context, guildID, userID string) (*discord.Member, bool, error) { + guild, err := parseID("guild", guildID) + if err != nil { + return nil, false, err + } + user, err := parseID("user", userID) + if err != nil { + return nil, false, err + } + + member, err := c.rest.GetMember(guild, user, rest.WithCtx(ctx)) + if err != nil { + if IsNotFound(err) { + return nil, false, nil + } + return nil, false, fmt.Errorf( + "baton-discord: failed to get member %s of guild %s: %w", userID, guildID, err) + } + return member, true, nil +} + // Roles returns every role in a guild. Discord returns this collection whole. func (c *Client) Roles(ctx context.Context, guildID string) ([]discord.Role, error) { id, err := parseID("guild", guildID) @@ -195,7 +278,7 @@ func (c *Client) Roles(ctx context.Context, guildID string) ([]discord.Role, err } roles, err := c.rest.GetRoles(id, rest.WithCtx(ctx)) if err != nil { - return nil, fmt.Errorf("baton-discord: failed to list roles of guild %s: %w", guildID, err) + return nil, wrapErr(err, "baton-discord: failed to list roles of guild %s", guildID) } return roles, nil } @@ -208,7 +291,7 @@ func (c *Client) Channels(ctx context.Context, guildID string) ([]discord.GuildC } channels, err := c.rest.GetGuildChannels(id, rest.WithCtx(ctx)) if err != nil { - return nil, fmt.Errorf("baton-discord: failed to list channels of guild %s: %w", guildID, err) + return nil, wrapErr(err, "baton-discord: failed to list channels of guild %s", guildID) } return channels, nil } @@ -221,7 +304,7 @@ func (c *Client) Channel(ctx context.Context, channelID string) (discord.GuildCh } channel, err := c.rest.GetChannel(id, rest.WithCtx(ctx)) if err != nil { - return nil, fmt.Errorf("baton-discord: failed to get channel %s: %w", channelID, err) + return nil, wrapErr(err, "baton-discord: failed to get channel %s", channelID) } guildChannel, ok := channel.(discord.GuildChannel) @@ -240,8 +323,7 @@ func (c *Client) AddMemberRole(ctx context.Context, guildID, userID, roleID, rea } if err := c.rest.AddMemberRole(guild, user, role, rest.WithCtx(ctx), rest.WithReason(reason)); err != nil { - return fmt.Errorf("baton-discord: failed to add role %s to user %s in guild %s: %w", - roleID, userID, guildID, err) + return wrapErr(err, "baton-discord: failed to add role %s to user %s in guild %s", roleID, userID, guildID) } return nil } @@ -254,8 +336,7 @@ func (c *Client) RemoveMemberRole(ctx context.Context, guildID, userID, roleID, } if err := c.rest.RemoveMemberRole(guild, user, role, rest.WithCtx(ctx), rest.WithReason(reason)); err != nil { - return fmt.Errorf("baton-discord: failed to remove role %s from user %s in guild %s: %w", - roleID, userID, guildID, err) + return wrapErr(err, "baton-discord: failed to remove role %s from user %s in guild %s", roleID, userID, guildID) } return nil } @@ -287,7 +368,7 @@ func (c *Client) RemoveGuildMember(ctx context.Context, guildID, userID, reason return err } if err := c.rest.RemoveMember(guild, user, rest.WithCtx(ctx), rest.WithReason(reason)); err != nil { - return fmt.Errorf("baton-discord: failed to remove user %s from guild %s: %w", userID, guildID, err) + return wrapErr(err, "baton-discord: failed to remove user %s from guild %s", userID, guildID) } return nil } @@ -324,8 +405,7 @@ func (c *Client) SetChannelOverwrite( if err := c.rest.UpdatePermissionOverwrite(channel, target, update, rest.WithCtx(ctx), rest.WithReason(reason)); err != nil { - return fmt.Errorf("baton-discord: failed to set permission overwrite for %s on channel %s: %w", - targetID, channelID, err) + return wrapErr(err, "baton-discord: failed to set permission overwrite for %s on channel %s", targetID, channelID) } return nil } @@ -342,8 +422,7 @@ func (c *Client) DeleteChannelOverwrite(ctx context.Context, channelID, targetID } if err := c.rest.DeletePermissionOverwrite(channel, target, rest.WithCtx(ctx), rest.WithReason(reason)); err != nil { - return fmt.Errorf("baton-discord: failed to delete permission overwrite for %s on channel %s: %w", - targetID, channelID, err) + return wrapErr(err, "baton-discord: failed to delete permission overwrite for %s on channel %s", targetID, channelID) } return nil } @@ -364,7 +443,7 @@ func (c *Client) CreateInvite(ctx context.Context, channelID string, maxAgeSecon Unique: true, }, rest.WithCtx(ctx)) if err != nil { - return nil, fmt.Errorf("baton-discord: failed to create an invite for channel %s: %w", channelID, err) + return nil, wrapErr(err, "baton-discord: failed to create an invite for channel %s", channelID) } return invite, nil } @@ -372,7 +451,7 @@ func (c *Client) CreateInvite(ctx context.Context, channelID string, maxAgeSecon // DeleteInvite revokes an invite so it can no longer be redeemed. func (c *Client) DeleteInvite(ctx context.Context, code string) error { if _, err := c.rest.DeleteInvite(code, rest.WithCtx(ctx)); err != nil { - return fmt.Errorf("baton-discord: failed to delete invite: %w", err) + return wrapErr(err, "baton-discord: failed to delete invite") } return nil } @@ -386,12 +465,12 @@ func (c *Client) SendDirectMessage(ctx context.Context, userID, content string) channel, err := c.rest.CreateDMChannel(user, rest.WithCtx(ctx)) if err != nil { - return fmt.Errorf("baton-discord: failed to open a DM channel with user %s: %w", userID, err) + return wrapErr(err, "baton-discord: failed to open a DM channel with user %s", userID) } if _, err := c.rest.CreateMessage(channel.ID(), discord.MessageCreate{Content: content}, rest.WithCtx(ctx)); err != nil { - return fmt.Errorf("baton-discord: failed to send a DM to user %s: %w", userID, err) + return wrapErr(err, "baton-discord: failed to send a DM to user %s", userID) } return nil } diff --git a/pkg/connector/channels.go b/pkg/connector/channels.go index 89593ed7..e4427f14 100644 --- a/pkg/connector/channels.go +++ b/pkg/connector/channels.go @@ -200,6 +200,16 @@ func (c *channelBuilder) Grants( permissions := permissionsForChannel(channel.Type()) + // Member overwrites outlive the membership they were created for: Discord + // keeps them when the targeted member leaves the server. Emitting those + // would produce grants pointing at users that the user listing never + // returned, which the SDK reports as dangling principals. Role overwrites + // need no such check, because a deleted role takes its overwrites with it. + guildID, err := parentGuildID(resource) + if err != nil { + return nil, nil, err + } + var grants []*v2.Grant for _, overwrite := range channel.PermissionOverwrites() { target, ok := describeOverwrite(overwrite) @@ -207,6 +217,16 @@ func (c *channelBuilder) Grants( continue } + if target.ResourceTypeID == userResourceTypeID { + _, stillAMember, err := c.client.Member(ctx, guildID, target.ID) + if err != nil { + return nil, nil, err + } + if !stillAMember { + continue + } + } + principal, err := resource_sdk.NewResourceID( resourceTypeFor(target.ResourceTypeID), target.ID) if err != nil { @@ -320,6 +340,12 @@ func (c *channelBuilder) Revoke(ctx context.Context, g *v2.Grant) (annotations.A if err := requireResourceType(g.Entitlement.Resource, channelResourceTypeID); err != nil { return nil, err } + // A channel overwrite targets a role or a member, so both are valid here, + // but the principal still has to be one of them and has to exist. See + // guildBuilder.Revoke. + if _, err := overwriteIsForRole(g.Principal); err != nil { + return nil, err + } permission, err := channelPermissionForEntitlement(g.Entitlement) if err != nil { diff --git a/pkg/connector/connector_test.go b/pkg/connector/connector_test.go index 692f1759..a030ad01 100644 --- a/pkg/connector/connector_test.go +++ b/pkg/connector/connector_test.go @@ -11,12 +11,16 @@ import ( "strings" "sync" "testing" + "time" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/pagination" resource_sdk "github.com/conductorone/baton-sdk/pkg/types/resource" "github.com/disgoorg/disgo/discord" "github.com/disgoorg/disgo/rest" + "github.com/disgoorg/snowflake/v2" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "github.com/ConductorOne/baton-discord/pkg/client" ) @@ -100,8 +104,8 @@ func newFakeDiscord(t *testing.T) *fakeDiscord { return } - // The RoundTripper override preserves discordgo's versioned path, so - // strip it to keep handler keys readable. + // disgo builds paths under its versioned API root, which rest.WithURL + // preserves. Strip that prefix to keep handler keys readable. path := r.URL.Path if trimmed, ok := trimAPIPrefix(path); ok { path = trimmed @@ -135,7 +139,9 @@ func newFakeDiscord(t *testing.T) *fakeDiscord { return fake } -// trimAPIPrefix strips discordgo's "/api/v" prefix from a request path. +// trimAPIPrefix strips disgo's "/api/v" prefix from a request path. The +// version is matched rather than hard-coded so a disgo upgrade does not +// silently break every handler lookup. func trimAPIPrefix(path string) (string, bool) { const prefix = "/api/v" if !strings.HasPrefix(path, prefix) { @@ -206,6 +212,19 @@ func (f *fakeDiscord) newClient(t *testing.T) *client.Client { return c } +// handleMemberLookup registers the per-member probe that channel grants use to +// keep member overwrites from naming users who have left the server. Passing +// present=false makes the fake answer the way Discord does for a departed +// member. +func (f *fakeDiscord) handleMemberLookup(userID string, present bool) { + path := "/guilds/" + testGuildID + "/members/" + userID + if !present { + f.handleStatus("GET", path, http.StatusNotFound, int(rest.JSONErrorCodeUnknownMember)) + return + } + f.handleJSON("GET", path, memberJSON(userID, "member-"+userID, "", "", false, nil)) +} + // afterCursor reads a paging cursor the way Discord interprets it: absent and // zero both mean "from the beginning", because 0 sorts below every snowflake. func afterCursor(rec recordedRequest) string { @@ -795,6 +814,8 @@ func TestChannelGrantsComeFromOverwriteAllowBits(t *testing.T) { discord.PermissionSendMessagesInThreads, discord.PermissionAttachFiles), })) + fake.handleMemberLookup(userAliceID, true) + builder := newChannelBuilder(fake.newClient(t)) grants, _, err := builder.Grants(context.Background(), resourceFor(t, channelResourceType, channelTextID, "general", guildResourceID(t)), @@ -1226,6 +1247,8 @@ func TestVoiceChannelGrantsReportActivities(t *testing.T) { discord.PermissionUseEmbeddedActivities, 0), })) + fake.handleMemberLookup(userAliceID, true) + builder := newChannelBuilder(fake.newClient(t)) grants, _, err := builder.Grants(context.Background(), resourceFor(t, channelResourceType, channelVoiceID, "voice-room", guildResourceID(t)), @@ -1411,3 +1434,152 @@ func TestGuildIDForProvisioningFallsBackToPrincipal(t *testing.T) { t.Error("expected an error when no parent server can be resolved") } } + +// TestChannelGrantsSkipDepartedMembers covers the dangling-principal case. +// +// Discord keeps a channel's permission overwrites when the targeted member +// leaves the server, so a member overwrite can name a user the member listing +// never returns. Emitting a grant for one produces a principal that does not +// exist in the sync, which the SDK reports as a dangling reference. +func TestChannelGrantsSkipDepartedMembers(t *testing.T) { + fake := newFakeDiscord(t) + + fake.handleJSON("GET", "/channels/"+channelTextID, channelJSON( + channelTextID, "general", discord.ChannelTypeGuildText, []map[string]any{ + // A role overwrite needs no probe: deleting a role takes its + // overwrites with it. + overwriteJSON(roleEveryoneID, overwriteWireTypeRole, discord.PermissionViewChannel, 0), + // A current member. + overwriteJSON(userAliceID, overwriteWireTypeMember, discord.PermissionViewChannel, 0), + // Someone who has since left the server. + overwriteJSON(userStrangerID, overwriteWireTypeMember, discord.PermissionViewChannel, 0), + })) + fake.handleMemberLookup(userAliceID, true) + fake.handleMemberLookup(userStrangerID, false) + + builder := newChannelBuilder(fake.newClient(t)) + grants, _, err := builder.Grants(context.Background(), + resourceFor(t, channelResourceType, channelTextID, "general", guildResourceID(t)), + resource_sdk.SyncOpAttrs{}) + if err != nil { + t.Fatalf("Grants: %v", err) + } + + principals := map[string]bool{} + for _, g := range grants { + principals[g.GetPrincipal().GetId().GetResource()] = true + } + + if !principals[roleEveryoneID] { + t.Error("a role overwrite must still produce a grant") + } + if !principals[userAliceID] { + t.Error("a current member's overwrite must produce a grant") + } + if principals[userStrangerID] { + t.Error("a departed member's overwrite must not produce a grant with an unsynced principal") + } +} + +// TestUserResourceFieldsAreAccountScoped checks that fields on a user resource +// describe the account rather than one of the servers it happens to belong to. +// The resource is keyed by the global account snowflake, so per-server values +// would be whichever server was written last. +func TestUserResourceFieldsAreAccountScoped(t *testing.T) { + fake := newFakeDiscord(t) + fake.handleJSON("GET", "/guilds/"+testGuildID+"/members", []map[string]any{ + memberJSON(userAliceID, "alice", "", "", false, nil), + }) + + builder := newUserBuilder(fake.newClient(t)) + resources, _, err := builder.List(context.Background(), guildResourceID(t), resource_sdk.SyncOpAttrs{}) + if err != nil { + t.Fatalf("List: %v", err) + } + if len(resources) != 1 { + t.Fatalf("got %d users, want 1", len(resources)) + } + user := resources[0] + + // guild_id is per-server and has no global equivalent, so it must not be on + // the account's profile. + if _, ok := user.GetProfile().AsMap()[profileKeyGuildID]; ok { + t.Error("the user profile must not carry a per-server guild_id") + } + + // created_at must be the account's creation time, which Discord encodes in + // the snowflake, not the per-server join date the fixture supplies. + aliceID, err := snowflake.Parse(userAliceID) + if err != nil { + t.Fatalf("parsing fixture snowflake: %v", err) + } + got := user.GetCreatedAt().AsTime() + if !got.Equal(aliceID.Time()) { + t.Errorf("created_at = %s, want the account creation time encoded in the snowflake (%s)", + got, aliceID.Time()) + } + if got.Equal(mustParseTime(t, testJoinedAt)) { + t.Error("created_at is the per-server join date, which is not account-scoped") + } +} + +func mustParseTime(t *testing.T, value string) time.Time { + t.Helper() + parsed, err := time.Parse(time.RFC3339Nano, value) + if err != nil { + t.Fatalf("parsing %q: %v", value, err) + } + return parsed +} + +// TestDiscordErrorsCarryStatusCodes covers error classification at the SDK +// boundary. Without a gRPC status code every failure arrives as Unknown, so C1 +// cannot tell a permission refusal from a transient one, and the provisioning +// retryer — which retries only Unavailable and DeadlineExceeded — never fires. +func TestDiscordErrorsCarryStatusCodes(t *testing.T) { + for _, tc := range []struct { + name string + httpStatus int + want codes.Code + }{ + {"unauthorized", http.StatusUnauthorized, codes.Unauthenticated}, + {"forbidden", http.StatusForbidden, codes.PermissionDenied}, + {"not found", http.StatusNotFound, codes.NotFound}, + {"rate limited", http.StatusTooManyRequests, codes.ResourceExhausted}, + // Transient upstream failures must land in the retry set. + {"server error", http.StatusInternalServerError, codes.Unavailable}, + {"bad gateway", http.StatusBadGateway, codes.Unavailable}, + } { + t.Run(tc.name, func(t *testing.T) { + fake := newFakeDiscord(t) + fake.handleStatus("GET", "/guilds/"+testGuildID+"/roles", tc.httpStatus, 0) + + _, err := fake.newClient(t).Roles(context.Background(), testGuildID) + if err == nil { + t.Fatalf("expected an error for HTTP %d", tc.httpStatus) + } + if got := status.Code(err); got != tc.want { + t.Errorf("status.Code = %v, want %v", got, tc.want) + } + }) + } +} + +// TestNotFoundStaysUnwrappable checks that adding a status code did not break +// the error inspection the idempotent revoke paths depend on. +func TestNotFoundStaysUnwrappable(t *testing.T) { + fake := newFakeDiscord(t) + fake.handleStatus("GET", "/guilds/"+testGuildID+"/roles", + http.StatusNotFound, int(rest.JSONErrorCodeUnknownGuild)) + + _, err := fake.newClient(t).Roles(context.Background(), testGuildID) + if err == nil { + t.Fatal("expected an error") + } + if !client.IsNotFound(err) { + t.Error("IsNotFound must still see through the status wrapper") + } + if got := status.Code(err); got != codes.NotFound { + t.Errorf("status.Code = %v, want NotFound", got) + } +} diff --git a/pkg/connector/guilds.go b/pkg/connector/guilds.go index 4d9ee2bf..f9539421 100644 --- a/pkg/connector/guilds.go +++ b/pkg/connector/guilds.go @@ -270,6 +270,14 @@ func (o *guildBuilder) Revoke(ctx context.Context, g *v2.Grant) (annotations.Ann if err := requireResourceType(g.Entitlement.Resource, guildResourceTypeID); err != nil { return nil, err } + // The SDK guarantees the entitlement chain but passes Grant.Principal + // through unvalidated. Without this an absent principal panics, and a + // role-typed one would hand a role snowflake to RemoveGuildMember whose 404 + // is swallowed below as "already revoked" — reporting success for a revoke + // that never happened. + if err := requireResourceType(g.Principal, userResourceTypeID); err != nil { + return nil, err + } guildID := g.Entitlement.Resource.Id.Resource userID := g.Principal.Id.Resource diff --git a/pkg/connector/roles.go b/pkg/connector/roles.go index 043650f5..49751352 100644 --- a/pkg/connector/roles.go +++ b/pkg/connector/roles.go @@ -232,6 +232,10 @@ func (r *roleBuilder) Revoke(ctx context.Context, g *v2.Grant) (annotations.Anno if err := requireResourceType(g.Entitlement.Resource, roleResourceTypeID); err != nil { return nil, err } + // See guildBuilder.Revoke: Grant.Principal arrives unvalidated. + if err := requireResourceType(g.Principal, userResourceTypeID); err != nil { + return nil, err + } guildID, err := guildIDForProvisioning(g.Entitlement.Resource, g.Principal) if err != nil { diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 51d42d6f..92b82294 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -67,10 +67,9 @@ func newMemberResource(member discord.Member, guildID string) (*v2.Resource, err } profile := map[string]any{ - profileKeyUserID: member.User.ID.String(), - "username": member.User.Username, - profileKeyGuildID: guildID, - "is_bot": member.User.Bot, + profileKeyUserID: member.User.ID.String(), + "username": member.User.Username, + "is_bot": member.User.Bot, } if member.Nick != nil && *member.Nick != "" { profile["nickname"] = *member.Nick @@ -99,9 +98,12 @@ func newMemberResource(member discord.Member, guildID string) (*v2.Resource, err if avatarURL := member.User.EffectiveAvatarURL(); avatarURL != "" { resourceOptions = append(resourceOptions, resource_sdk.WithResourceIcon(&v2.AssetRef{Id: avatarURL})) } - if member.JoinedAt != nil && !member.JoinedAt.IsZero() { - resourceOptions = append(resourceOptions, resource_sdk.WithResourceCreatedAt(*member.JoinedAt)) - } + // Discord snowflakes encode their creation timestamp, so the account's + // creation time is derivable and is a property of the account rather than of + // any one server. The member's join date is per-server and would be + // whichever server was written last on an account in several of them. + resourceOptions = append(resourceOptions, + resource_sdk.WithResourceCreatedAt(member.User.ID.Time())) return resource_sdk.NewUserResource( memberDisplayName(member), diff --git a/vendor/github.com/cockroachdb/swiss/runtime_go1.20.go b/vendor/github.com/cockroachdb/swiss/runtime_go1.20.go index f3149b83..79d530d1 100644 --- a/vendor/github.com/cockroachdb/swiss/runtime_go1.20.go +++ b/vendor/github.com/cockroachdb/swiss/runtime_go1.20.go @@ -17,12 +17,12 @@ // bumping of the go versions supported by adjusting the build tags below. The // way go version tags work the tag for goX.Y will be declared for every // subsequent release. So go1.20 will be defined for go1.21, go1.22, etc. The -// build tag "go1.20 && !go1.27" defines the range [go1.20, go1.27) (inclusive -// on go1.20, exclusive on go1.27). +// build tag "go1.20 && !go1.28" defines the range [go1.20, go1.28) (inclusive +// on go1.20, exclusive on go1.28). // The untested_go_version flag enables building on any go version, intended // to ease testing against Go at tip. -//go:build (go1.20 && !go1.27) || untested_go_version +//go:build (go1.20 && !go1.28) || untested_go_version package swiss diff --git a/vendor/modules.txt b/vendor/modules.txt index 82ec1f20..7c636e5f 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -266,7 +266,7 @@ github.com/cockroachdb/redact/internal/markers github.com/cockroachdb/redact/internal/redact github.com/cockroachdb/redact/internal/rfmt github.com/cockroachdb/redact/internal/rfmt/fmtsort -# github.com/cockroachdb/swiss v0.0.0-20251224182025-b0f6560f979b +# github.com/cockroachdb/swiss v0.0.0-20260820225851-333444432258 ## explicit; go 1.21 github.com/cockroachdb/swiss # github.com/cockroachdb/tokenbucket v0.0.0-20230807174530-cc333fc44b06