Fix SendGrid teammate-detail decode and harden MFA/auth coverage
The teammate detail endpoint returns a bare object, not a {"result":...}
envelope, so the N+1 scope fetch silently discarded scopes and MFA was
always Unknown for every teammate. Decode the bare object.
Found by re-recording the driver cassette against the live API; the
hand-written cassette had hidden the mismatch. Also from that review:
- Map is_sso/is_partner_sso to AuthMethod (SSO vs Password) instead of
hardcoding Unknown
- Log the swallowed teammate-detail fetch error (wire the driver logger,
matching github.go)
- Add a restricted teammate + a missing-email row to the cassette so the
N+1 decode, the empty-email skip, and the SSO path are load-bearing
- Drop the stale Content-Length response header; document ExternalID
Signed-off-by: Aurélien Sibiril <81782+aureliensibiril@users.noreply.github.com>
This commit is contained in:
@@ -23,23 +23,27 @@ import (
|
|||||||
"strconv"
|
"strconv"
|
||||||
"strings"
|
"strings"
|
||||||
|
|
||||||
|
"go.gearno.de/kit/log"
|
||||||
"go.probo.inc/probo/pkg/coredata"
|
"go.probo.inc/probo/pkg/coredata"
|
||||||
)
|
)
|
||||||
|
|
||||||
type SendGridDriver struct {
|
type SendGridDriver struct {
|
||||||
httpClient *http.Client
|
httpClient *http.Client
|
||||||
|
logger *log.Logger
|
||||||
}
|
}
|
||||||
|
|
||||||
var _ Driver = (*SendGridDriver)(nil)
|
var _ Driver = (*SendGridDriver)(nil)
|
||||||
|
|
||||||
type sendGridTeammate struct {
|
type sendGridTeammate struct {
|
||||||
Username string `json:"username"`
|
Username string `json:"username"`
|
||||||
Email string `json:"email"`
|
Email string `json:"email"`
|
||||||
FirstName string `json:"first_name"`
|
FirstName string `json:"first_name"`
|
||||||
LastName string `json:"last_name"`
|
LastName string `json:"last_name"`
|
||||||
UserType string `json:"user_type"`
|
UserType string `json:"user_type"`
|
||||||
IsAdmin bool `json:"is_admin"`
|
IsAdmin bool `json:"is_admin"`
|
||||||
Scopes []string `json:"scopes"`
|
IsSSO bool `json:"is_sso"`
|
||||||
|
IsPartnerSSO bool `json:"is_partner_sso"`
|
||||||
|
Scopes []string `json:"scopes"`
|
||||||
}
|
}
|
||||||
|
|
||||||
type sendGridTeammatesResponse struct {
|
type sendGridTeammatesResponse struct {
|
||||||
@@ -47,18 +51,15 @@ type sendGridTeammatesResponse struct {
|
|||||||
Results []sendGridTeammate `json:"results"`
|
Results []sendGridTeammate `json:"results"`
|
||||||
}
|
}
|
||||||
|
|
||||||
type sendGridTeammateResponse struct {
|
|
||||||
Result sendGridTeammate `json:"result"`
|
|
||||||
}
|
|
||||||
|
|
||||||
const (
|
const (
|
||||||
sendGridTeammatesEndpoint = "https://api.sendgrid.com/v3/teammates"
|
sendGridTeammatesEndpoint = "https://api.sendgrid.com/v3/teammates"
|
||||||
sendGridTeammatesPageLimit = 500
|
sendGridTeammatesPageLimit = 500
|
||||||
)
|
)
|
||||||
|
|
||||||
func NewSendGridDriver(httpClient *http.Client) *SendGridDriver {
|
func NewSendGridDriver(httpClient *http.Client, logger *log.Logger) *SendGridDriver {
|
||||||
return &SendGridDriver{
|
return &SendGridDriver{
|
||||||
httpClient: httpClient,
|
httpClient: httpClient,
|
||||||
|
logger: logger,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -80,22 +81,35 @@ func (d *SendGridDriver) ListAccounts(ctx context.Context) ([]AccountRecord, err
|
|||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// The teammate list carries no scopes, so MFA must be read from
|
||||||
|
// the per-teammate detail endpoint (N+1).
|
||||||
mfaStatus := sendGridMFAStatus(teammate.Scopes)
|
mfaStatus := sendGridMFAStatus(teammate.Scopes)
|
||||||
if mfaStatus == coredata.MFAStatusUnknown && teammate.Username != "" {
|
if mfaStatus == coredata.MFAStatusUnknown && teammate.Username != "" {
|
||||||
detailedTeammate, err := d.fetchTeammate(ctx, teammate.Username)
|
detailedTeammate, err := d.fetchTeammate(ctx, teammate.Username)
|
||||||
if err == nil {
|
if err != nil {
|
||||||
|
// Best-effort: a failed detail fetch leaves MFA Unknown
|
||||||
|
// rather than dropping the account. Log it (PII-free) so a
|
||||||
|
// wholesale detail-endpoint outage is observable.
|
||||||
|
d.logger.WarnCtx(
|
||||||
|
ctx,
|
||||||
|
"cannot fetch sendgrid teammate details, reporting MFA unknown",
|
||||||
|
log.Error(err),
|
||||||
|
)
|
||||||
|
} else {
|
||||||
mfaStatus = sendGridMFAStatus(detailedTeammate.Scopes)
|
mfaStatus = sendGridMFAStatus(detailedTeammate.Scopes)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
records = append(records, AccountRecord{
|
records = append(records, AccountRecord{
|
||||||
Email: teammate.Email,
|
Email: teammate.Email,
|
||||||
FullName: sendGridFullName(teammate.FirstName, teammate.LastName),
|
FullName: sendGridFullName(teammate.FirstName, teammate.LastName),
|
||||||
Role: sendGridRole(teammate.UserType, teammate.IsAdmin),
|
Role: sendGridRole(teammate.UserType, teammate.IsAdmin),
|
||||||
IsAdmin: teammate.IsAdmin,
|
IsAdmin: teammate.IsAdmin,
|
||||||
|
// SendGrid exposes no UUID for teammates; the username is the
|
||||||
|
// only stable handle. For unified accounts it equals the email.
|
||||||
ExternalID: strings.TrimSpace(teammate.Username),
|
ExternalID: strings.TrimSpace(teammate.Username),
|
||||||
MFAStatus: mfaStatus,
|
MFAStatus: mfaStatus,
|
||||||
AuthMethod: coredata.AccessEntryAuthMethodUnknown,
|
AuthMethod: sendGridAuthMethod(teammate),
|
||||||
AccountType: coredata.AccessEntryAccountTypeUser,
|
AccountType: coredata.AccessEntryAccountTypeUser,
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
@@ -173,12 +187,14 @@ func (d *SendGridDriver) fetchTeammate(ctx context.Context, username string) (*s
|
|||||||
return nil, fmt.Errorf("cannot fetch sendgrid teammate details: unexpected status %d", httpResp.StatusCode)
|
return nil, fmt.Errorf("cannot fetch sendgrid teammate details: unexpected status %d", httpResp.StatusCode)
|
||||||
}
|
}
|
||||||
|
|
||||||
var resp sendGridTeammateResponse
|
// The teammate detail endpoint returns a bare teammate object, NOT a
|
||||||
|
// {"result": {...}} envelope (the list endpoint is the wrapped one).
|
||||||
|
var resp sendGridTeammate
|
||||||
if err := json.NewDecoder(httpResp.Body).Decode(&resp); err != nil {
|
if err := json.NewDecoder(httpResp.Body).Decode(&resp); err != nil {
|
||||||
return nil, fmt.Errorf("cannot decode sendgrid teammate details response: %w", err)
|
return nil, fmt.Errorf("cannot decode sendgrid teammate details response: %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
return &resp.Result, nil
|
return &resp, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
func sendGridResponseItems(resp *sendGridTeammatesResponse) []sendGridTeammate {
|
func sendGridResponseItems(resp *sendGridTeammatesResponse) []sendGridTeammate {
|
||||||
@@ -212,6 +228,18 @@ func sendGridRole(userType string, isAdmin bool) string {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// sendGridAuthMethod maps SendGrid's SSO flags to an auth method. A teammate
|
||||||
|
// authenticated through SSO (native or partner) is SSO; otherwise they sign in
|
||||||
|
// with SendGrid's own credentials. Both flags are always present on the
|
||||||
|
// teammate payload, so this is a definitive signal.
|
||||||
|
func sendGridAuthMethod(t sendGridTeammate) coredata.AccessEntryAuthMethod {
|
||||||
|
if t.IsSSO || t.IsPartnerSSO {
|
||||||
|
return coredata.AccessEntryAuthMethodSSO
|
||||||
|
}
|
||||||
|
|
||||||
|
return coredata.AccessEntryAuthMethodPassword
|
||||||
|
}
|
||||||
|
|
||||||
// sendGridMFAStatus derives a teammate's MFA status from the auto-set 2fa
|
// sendGridMFAStatus derives a teammate's MFA status from the auto-set 2fa
|
||||||
// scopes SendGrid attaches to the teammate detail. A restricted teammate
|
// scopes SendGrid attaches to the teammate detail. A restricted teammate
|
||||||
// carries exactly one of them to reflect their real status. Full-access
|
// carries exactly one of them to reflect their real status. Full-access
|
||||||
@@ -222,6 +250,7 @@ func sendGridRole(userType string, isAdmin bool) string {
|
|||||||
// both-or-neither is ambiguous, so report Unknown rather than guessing.
|
// both-or-neither is ambiguous, so report Unknown rather than guessing.
|
||||||
func sendGridMFAStatus(scopes []string) coredata.MFAStatus {
|
func sendGridMFAStatus(scopes []string) coredata.MFAStatus {
|
||||||
var exempt, required bool
|
var exempt, required bool
|
||||||
|
|
||||||
for _, scope := range scopes {
|
for _, scope := range scopes {
|
||||||
switch scope {
|
switch scope {
|
||||||
case "2fa_exempt":
|
case "2fa_exempt":
|
||||||
|
|||||||
@@ -21,6 +21,7 @@ import (
|
|||||||
|
|
||||||
"github.com/stretchr/testify/assert"
|
"github.com/stretchr/testify/assert"
|
||||||
"github.com/stretchr/testify/require"
|
"github.com/stretchr/testify/require"
|
||||||
|
"go.gearno.de/kit/log"
|
||||||
"go.probo.inc/probo/pkg/coredata"
|
"go.probo.inc/probo/pkg/coredata"
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -29,15 +30,16 @@ func TestSendGridDriver(t *testing.T) {
|
|||||||
|
|
||||||
rec := newRecorder(t, "testdata/sendgrid", "SENDGRID_API_KEY")
|
rec := newRecorder(t, "testdata/sendgrid", "SENDGRID_API_KEY")
|
||||||
client := newVCRClient(rec, bearerAuth(os.Getenv("SENDGRID_API_KEY")))
|
client := newVCRClient(rec, bearerAuth(os.Getenv("SENDGRID_API_KEY")))
|
||||||
driver := NewSendGridDriver(client)
|
driver := NewSendGridDriver(client, log.NewLogger(log.WithName("test")))
|
||||||
|
|
||||||
records, err := driver.ListAccounts(context.Background())
|
records, err := driver.ListAccounts(context.Background())
|
||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
require.Len(t, records, 1)
|
// Two records: the owner + a restricted teammate. A third list row with an
|
||||||
|
// empty email is skipped.
|
||||||
|
require.Len(t, records, 2)
|
||||||
|
|
||||||
// Recorded against a live SendGrid account that has only the owner. The
|
// The owner record is the real recording. The list endpoint carries no
|
||||||
// list endpoint carries no scopes, so the driver fetches the teammate
|
// scopes, so the driver fetches the teammate detail to read them.
|
||||||
// detail to read them.
|
|
||||||
owner := records[0]
|
owner := records[0]
|
||||||
assert.Equal(t, "owner@example.com", owner.Email)
|
assert.Equal(t, "owner@example.com", owner.Email)
|
||||||
assert.Empty(t, owner.FullName)
|
assert.Empty(t, owner.FullName)
|
||||||
@@ -45,10 +47,27 @@ func TestSendGridDriver(t *testing.T) {
|
|||||||
assert.True(t, owner.IsAdmin)
|
assert.True(t, owner.IsAdmin)
|
||||||
assert.Equal(t, "owner@example.com", owner.ExternalID)
|
assert.Equal(t, "owner@example.com", owner.ExternalID)
|
||||||
assert.Equal(t, coredata.AccessEntryAccountTypeUser, owner.AccountType)
|
assert.Equal(t, coredata.AccessEntryAccountTypeUser, owner.AccountType)
|
||||||
|
// is_sso=false on the owner -> authenticates with SendGrid credentials.
|
||||||
|
assert.Equal(t, coredata.AccessEntryAuthMethodPassword, owner.AuthMethod)
|
||||||
// The owner is a full-access user whose scope catalog contains BOTH
|
// The owner is a full-access user whose scope catalog contains BOTH
|
||||||
// 2fa_exempt and 2fa_required, so the MFA signal is ambiguous and the
|
// 2fa_exempt and 2fa_required, so the MFA signal is ambiguous and the
|
||||||
// driver reports Unknown rather than guessing from scope ordering.
|
// driver reports Unknown rather than guessing from scope ordering.
|
||||||
assert.Equal(t, coredata.MFAStatusUnknown, owner.MFAStatus)
|
assert.Equal(t, coredata.MFAStatusUnknown, owner.MFAStatus)
|
||||||
|
|
||||||
|
// A restricted teammate, synthetic (the trial account has only the owner)
|
||||||
|
// but modelled on the real detail shape: a BARE object whose scopes carry
|
||||||
|
// a single 2fa flag. This makes the N+1 detail fetch load-bearing — an
|
||||||
|
// Enabled MFA here is reachable ONLY by correctly decoding the detail
|
||||||
|
// response, so it guards against the {"result":...}-envelope regression.
|
||||||
|
teammate := records[1]
|
||||||
|
assert.Equal(t, "taylor@example.com", teammate.Email)
|
||||||
|
assert.Equal(t, "Taylor Teammate", teammate.FullName)
|
||||||
|
assert.Equal(t, "Teammate", teammate.Role)
|
||||||
|
assert.False(t, teammate.IsAdmin)
|
||||||
|
// Non-unified teammate: username is a handle distinct from the email.
|
||||||
|
assert.Equal(t, "taylor-teammate", teammate.ExternalID)
|
||||||
|
assert.Equal(t, coredata.AccessEntryAuthMethodSSO, teammate.AuthMethod)
|
||||||
|
assert.Equal(t, coredata.MFAStatusEnabled, teammate.MFAStatus)
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestSendGridRole(t *testing.T) {
|
func TestSendGridRole(t *testing.T) {
|
||||||
|
|||||||
36
pkg/accessreview/drivers/testdata/sendgrid.yaml
vendored
36
pkg/accessreview/drivers/testdata/sendgrid.yaml
vendored
@@ -24,7 +24,7 @@ interactions:
|
|||||||
proto_minor: 1
|
proto_minor: 1
|
||||||
content_length: -1
|
content_length: -1
|
||||||
body: |
|
body: |
|
||||||
{"result":[{"username":"owner@example.com","email":"owner@example.com","first_name":"","last_name":"","address":"","address2":"","city":"","state":"","zip":"","country":"","company":"","website":"","phone":"","is_admin":true,"is_sso":false,"user_type":"owner","is_unified":true,"is_partner_sso":false}]}
|
{"result":[{"username":"owner@example.com","email":"owner@example.com","first_name":"","last_name":"","address":"","address2":"","city":"","state":"","zip":"","country":"","company":"","website":"","phone":"","is_admin":true,"is_sso":false,"user_type":"owner","is_unified":true,"is_partner_sso":false},{"username":"taylor-teammate","email":"taylor@example.com","first_name":"Taylor","last_name":"Teammate","is_admin":false,"is_sso":true,"user_type":"teammate","is_unified":false,"is_partner_sso":false},{"username":"pending-invite","email":"","first_name":"","last_name":"","is_admin":false,"is_sso":false,"user_type":"teammate","is_unified":false,"is_partner_sso":false}]}
|
||||||
headers:
|
headers:
|
||||||
Access-Control-Allow-Headers:
|
Access-Control-Allow-Headers:
|
||||||
- AUTHORIZATION, Content-Type, On-behalf-of, x-sg-elas-acl, X-Recaptcha, X-Request-Source, Browser-Fingerprint
|
- AUTHORIZATION, Content-Type, On-behalf-of, x-sg-elas-acl, X-Recaptcha, X-Request-Source, Browser-Fingerprint
|
||||||
@@ -40,8 +40,6 @@ interactions:
|
|||||||
- no-cache
|
- no-cache
|
||||||
Connection:
|
Connection:
|
||||||
- keep-alive
|
- keep-alive
|
||||||
Content-Length:
|
|
||||||
- "334"
|
|
||||||
Content-Security-Policy:
|
Content-Security-Policy:
|
||||||
- default-src https://api.sendgrid.com; frame-src 'none'; object-src 'none'
|
- default-src https://api.sendgrid.com; frame-src 'none'; object-src 'none'
|
||||||
Content-Type:
|
Content-Type:
|
||||||
@@ -143,3 +141,35 @@ interactions:
|
|||||||
status: 200 OK
|
status: 200 OK
|
||||||
code: 200
|
code: 200
|
||||||
duration: 283.823083ms
|
duration: 283.823083ms
|
||||||
|
- id: 2
|
||||||
|
request:
|
||||||
|
proto: HTTP/1.1
|
||||||
|
proto_major: 1
|
||||||
|
proto_minor: 1
|
||||||
|
content_length: 0
|
||||||
|
host: api.sendgrid.com
|
||||||
|
headers:
|
||||||
|
Accept:
|
||||||
|
- application/json
|
||||||
|
url: https://api.sendgrid.com/v3/teammates/taylor-teammate
|
||||||
|
method: GET
|
||||||
|
response:
|
||||||
|
proto: HTTP/1.1
|
||||||
|
proto_major: 1
|
||||||
|
proto_minor: 1
|
||||||
|
transfer_encoding:
|
||||||
|
- chunked
|
||||||
|
content_length: -1
|
||||||
|
uncompressed: true
|
||||||
|
body: |
|
||||||
|
{"username":"taylor-teammate","email":"taylor@example.com","first_name":"Taylor","last_name":"Teammate","is_admin":false,"is_sso":true,"user_type":"teammate","is_unified":false,"is_partner_sso":false,"scopes":["mail.send","2fa_required"]}
|
||||||
|
headers:
|
||||||
|
Content-Type:
|
||||||
|
- application/json; charset=utf8
|
||||||
|
Date:
|
||||||
|
- Thu, 04 Jun 2026 12:30:34 GMT
|
||||||
|
Server:
|
||||||
|
- nginx
|
||||||
|
status: 200 OK
|
||||||
|
code: 200
|
||||||
|
duration: 18ms
|
||||||
|
|||||||
@@ -29,8 +29,8 @@ func sendgridRegistration() *Registration {
|
|||||||
DisplayName: "SendGrid",
|
DisplayName: "SendGrid",
|
||||||
ProbeURL: "https://api.sendgrid.com/v3/teammates?limit=1&offset=0",
|
ProbeURL: "https://api.sendgrid.com/v3/teammates?limit=1&offset=0",
|
||||||
SupportsAPIKey: true,
|
SupportsAPIKey: true,
|
||||||
NewDriver: func(_ context.Context, c *http.Client, _ *coredata.Connector, _ *log.Logger) (drivers.Driver, error) {
|
NewDriver: func(_ context.Context, c *http.Client, _ *coredata.Connector, logger *log.Logger) (drivers.Driver, error) {
|
||||||
return drivers.NewSendGridDriver(c), nil
|
return drivers.NewSendGridDriver(c, logger.Named("sendgrid")), nil
|
||||||
},
|
},
|
||||||
NewNameResolver: func(_ context.Context, c *http.Client, _ *coredata.Connector, _ *log.Logger) drivers.NameResolver {
|
NewNameResolver: func(_ context.Context, c *http.Client, _ *coredata.Connector, _ *log.Logger) drivers.NameResolver {
|
||||||
return drivers.NewSendGridNameResolver(c)
|
return drivers.NewSendGridNameResolver(c)
|
||||||
|
|||||||
Reference in New Issue
Block a user