-
Notifications
You must be signed in to change notification settings - Fork 1
Guard role grant emission behind WillSyncResourceType #31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |
| "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" | ||
| "go.uber.org/zap" | ||
| "google.golang.org/grpc/codes" | ||
| "google.golang.org/protobuf/proto" | ||
| "google.golang.org/protobuf/types/known/structpb" | ||
| ) | ||
|
|
||
|
|
@@ -28,9 +29,19 @@ | |
| return o.resourceType | ||
| } | ||
|
|
||
| func newUserBuilder(client *client.Client) *userResourceType { | ||
| func newUserBuilder(client *client.Client, syncRoles bool) *userResourceType { | ||
| resourceType := resourceTypeUser | ||
| if !syncRoles { | ||
| // The user builder has no entitlements or grants of its own -- its only | ||
| // Grants() output is the cross-type role grant gated below. When roles | ||
| // aren't being synced, skip entitlement/grant discovery for users entirely. | ||
| rt := proto.Clone(resourceTypeUser).(*v2.ResourceType) | ||
| rt.Annotations = annotations.New(&v2.SkipEntitlementsAndGrants{}) | ||
| resourceType = rt | ||
| } | ||
|
|
||
| return &userResourceType{ | ||
| resourceType: resourceTypeUser, | ||
| resourceType: resourceType, | ||
| client: client, | ||
| managers: make(map[string]*jcapi1.Systemuserreturn), | ||
| usersCache: newUsersCache(client), | ||
|
|
@@ -41,6 +52,11 @@ | |
| return nil, nil, nil | ||
| } | ||
|
|
||
| // Grants emits the cross-type role grant. There is no syncRoles guard here: | ||
| // when roles aren't being synced, newUserBuilder annotates the user resource | ||
| // type SkipEntitlementsAndGrants and the SDK never calls Grants() at all | ||
| // (shouldSkipGrants -> shouldSkipEntitlementsAndGrants in the SDK's | ||
| // pkg/sync/syncer.go), so a guard would be unreachable. | ||
|
Comment on lines
+55
to
+59
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: The claim holds for the syncer path — I verified |
||
| func (o *userResourceType) Grants(ctx context.Context, resource *v2.Resource, _ sdkResources.SyncOpAttrs) ([]*v2.Grant, *sdkResources.SyncOpResults, error) { | ||
| userID := resource.Id.Resource | ||
| // Only admin users have role grants. System users won't be found in the admin users endpoint. | ||
|
|
@@ -177,14 +193,14 @@ | |
| } | ||
|
|
||
| userTraitOps := []sdkResources.UserTraitOption{ | ||
| sdkResources.WithUserProfile(profile), | ||
|
Check failure on line 196 in pkg/connector/users.go
|
||
| } | ||
|
|
||
| status := v2.UserTrait_Status_STATUS_ENABLED | ||
| if user.GetSuspended() { | ||
| status = v2.UserTrait_Status_STATUS_DISABLED | ||
| } | ||
| userTraitOps = append(userTraitOps, sdkResources.WithStatus(status)) | ||
|
Check failure on line 203 in pkg/connector/users.go
|
||
|
|
||
| email := user.GetEmail() | ||
| if email != "" { | ||
|
|
@@ -275,22 +291,22 @@ | |
|
|
||
| switch st := user.GetState(); st { | ||
| case "", "ACTIVATED": | ||
| ret.Status.Status = v2.UserTrait_Status_STATUS_ENABLED | ||
| case "STAGED": | ||
| ret.Status.Status = v2.UserTrait_Status_STATUS_DISABLED | ||
| ret.Status.Details = strings.ToLower(st) | ||
| case "SUSPENDED": | ||
| ret.Status.Status = v2.UserTrait_Status_STATUS_DISABLED | ||
| ret.Status.Details = strings.ToLower(st) | ||
| } | ||
|
|
||
| if user.GetAccountLocked() { | ||
| ret.Status.Status = v2.UserTrait_Status_STATUS_DISABLED | ||
| ret.Status.Details = "locked" | ||
| } | ||
|
|
||
| if user.GetSuspended() { | ||
| ret.Status.Status = v2.UserTrait_Status_STATUS_DISABLED | ||
| ret.Status.Details = "suspended" | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| package connector | ||
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "testing" | ||
|
|
||
| "github.com/conductorone/baton-jumpcloud/pkg/client" | ||
| v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" | ||
| "github.com/conductorone/baton-sdk/pkg/annotations" | ||
| sdkResources "github.com/conductorone/baton-sdk/pkg/types/resource" | ||
| "github.com/stretchr/testify/require" | ||
| "google.golang.org/protobuf/proto" | ||
| "google.golang.org/protobuf/types/known/anypb" | ||
| ) | ||
|
|
||
| // TestUserGrants_RoleSyncFilter covers ConductorOne/baton-linear#55: the user | ||
| // builder emits role grants as a sync optimization, but must not do so when | ||
| // the customer's sync filter excludes the role resource type. | ||
| func TestUserGrants_RoleSyncFilter(t *testing.T) { | ||
|
Comment on lines
+19
to
+22
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: Two staleness nits after the guard removal. The doc comment references |
||
| userResource := &v2.Resource{ | ||
| Id: fmtResourceId(resourceTypeUser.Id, "user-1"), | ||
| } | ||
|
|
||
| // Grants() itself is unconditional: when roles are filtered out the SDK is | ||
| // stopped by the SkipEntitlementsAndGrants annotation before it ever calls | ||
| // Grants, which TestNewUserBuilder_ResourceTypeAnnotations pins. | ||
| t.Run("role type synced -> role grant emitted", func(t *testing.T) { | ||
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _ = json.NewEncoder(w).Encode(map[string]string{ | ||
| "id": "user-1", | ||
| "roleName": "Administrator", | ||
| }) | ||
| })) | ||
| defer srv.Close() | ||
|
|
||
| c, err := client.NewClient(context.Background(), "api-key", "", srv.URL) | ||
| require.NoError(t, err) | ||
|
|
||
| builder := newUserBuilder(c, true) | ||
|
|
||
| grants, _, err := builder.Grants(context.Background(), userResource, sdkResources.SyncOpAttrs{}) | ||
| require.NoError(t, err) | ||
| require.Len(t, grants, 1) | ||
| require.Equal(t, fmtRoleNameAsID("Administrator"), grants[0].Entitlement.Resource.Id.Resource) | ||
| require.Equal(t, resourceTypeRole.Id, grants[0].Entitlement.Resource.Id.ResourceType) | ||
| }) | ||
| } | ||
|
|
||
| // TestNewUserBuilder_ResourceTypeAnnotations covers Step 4 of the | ||
| // WillSyncResourceType gating pattern: when the user builder has no | ||
| // entitlements/grants of its own to offer, mark the emitted resource type | ||
| // with SkipEntitlementsAndGrants so the SDK doesn't bother syncing them. | ||
| func TestNewUserBuilder_ResourceTypeAnnotations(t *testing.T) { | ||
| t.Run("role type filtered out -> SkipEntitlementsAndGrants set", func(t *testing.T) { | ||
| builder := newUserBuilder(nil, false) | ||
| var skip v2.SkipEntitlementsAndGrants | ||
| ok, err := annotationsContain(builder.resourceType.GetAnnotations(), &skip) | ||
| require.NoError(t, err) | ||
| require.True(t, ok) | ||
| }) | ||
|
|
||
| t.Run("role type synced -> no SkipEntitlementsAndGrants", func(t *testing.T) { | ||
| builder := newUserBuilder(nil, true) | ||
| var skip v2.SkipEntitlementsAndGrants | ||
| ok, err := annotationsContain(builder.resourceType.GetAnnotations(), &skip) | ||
| require.NoError(t, err) | ||
| require.False(t, ok) | ||
| }) | ||
| } | ||
|
|
||
| func annotationsContain(annos []*anypb.Any, msg proto.Message) (bool, error) { | ||
| as := annotations.Annotations(annos) | ||
| return as.Pick(msg) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Suggestion: This comment is now stale — it says the role grant is "gated below", but this commit removed the
if !o.syncRolesgate fromGrants(). Reword to say the gating is done entirely by this annotation (or restore the guard and keep the wording).