Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds Tableau "View" resource support: new connector resource type, client APIs and models for views and view permissions, site annotations updated to include views, and group logic updated to skip ServerAdministrator users when computing grants. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgba(200,200,255,0.5)
participant Client as Baton Client
end
rect rgba(200,255,200,0.5)
participant Connector as Baton Connector
end
rect rgba(255,200,200,0.5)
participant TableauClient as Tableau Client
end
rect rgba(255,255,200,0.5)
participant API as Tableau API
end
Client->>Connector: List views / Get entitlements / Grant / Revoke
Connector->>TableauClient: GetPaginatedViews() / GetViewPermissions(viewID) / Add/Delete View Permission
TableauClient->>API: GET /sites/{siteId}/views (paginated)
TableauClient->>API: GET /views/{viewId}/permissions
TableauClient->>API: PUT/DELETE /views/{viewId}/permissions (user|group)
API-->>TableauClient: Views pages / GranteeCapabilities / Success
TableauClient-->>Connector: Views / Capabilities / Result
Connector->>Client: Resources / Entitlements / Operation outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/connector/group.go`:
- Around line 18-21: Run goimports to fix import formatting and lint errors in
the file containing the constants memberEntitlement and siteRoleServerAdmin;
re-run goimports (or goimports -w .) to rewrite the file with properly
grouped/ordered imports and formatted code, then save and re-run CI to ensure
the verify step passes.
- Around line 111-121: The debug log in the for loop over users logs user.Email
(PII); remove or redact that field — update the logging call in the loop (for _,
user := range users) that uses l.Debug to omit zap.String("user_email",
user.Email) (or replace it with a non-PII placeholder) and keep non-sensitive
identifiers like "group_id" and "user_id" only; ensure the change is applied
where server admin users are skipped (the skip branch referencing
siteRoleServerAdmin and variables groupId, user.ID, user.Email).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/connector/view.go`:
- Around line 106-159: The Grants method on viewResourceType may emit grants for
unknown capability names; update Grants to filter capability.Name against the
known viewCapabilities set before creating grants. Specifically, in Grants
(function viewResourceType.Grants) build or reuse a lookup (e.g.,
map[string]struct{} from viewCapabilities) and skip any capability whose Name is
not present; apply this check before the user grant creation block (where
rs.NewResourceID/resourceTypeUser is used) and before the group grant creation
block (where resourceTypeGroup and grant.WithAnnotation are used) so only known
entitlement names produce Grant objects.
🧹 Nitpick comments (1)
pkg/connector/view.go (1)
66-87: Consider honoring pagination tokens for large sites.
Listcurrently ignorestokenand always loads all views, which can be costly for large tenants. Consider using the token to page through views incrementally rather than fetching everything at once.
| func (v *viewResourceType) Grants(ctx context.Context, resource *v2.Resource, token *pagination.Token) ([]*v2.Grant, string, annotations.Annotations, error) { | ||
| l := ctxzap.Extract(ctx) | ||
| var rv []*v2.Grant | ||
|
|
||
| viewID := resource.Id.Resource | ||
|
|
||
| permissions, err := v.client.GetViewPermissions(ctx, viewID) | ||
| if err != nil { | ||
| l.Debug( | ||
| "baton-tableau: failed to get view permissions, skipping grants for this view", | ||
| zap.String("view_id", viewID), | ||
| zap.String("view_name", resource.DisplayName), | ||
| zap.Error(err), | ||
| ) | ||
| return rv, "", nil, nil | ||
| } | ||
|
|
||
| for _, grantee := range permissions { | ||
| for _, capability := range grantee.Capabilities.Capability { | ||
| if capability.Mode != "Allow" { | ||
| continue | ||
| } | ||
|
|
||
| if grantee.User != nil { | ||
| principalID, err := rs.NewResourceID(resourceTypeUser, grantee.User.ID) | ||
| if err != nil { | ||
| return nil, "", nil, fmt.Errorf("tableau-connector: failed to create user resource ID: %w", err) | ||
| } | ||
| g := grant.NewGrant(resource, capability.Name, principalID) | ||
| rv = append(rv, g) | ||
| } | ||
|
|
||
| if grantee.Group != nil { | ||
| groupID := grantee.Group.ID | ||
| principalID, err := rs.NewResourceID(resourceTypeGroup, groupID) | ||
| if err != nil { | ||
| return nil, "", nil, fmt.Errorf("tableau-connector: failed to create group resource ID: %w", err) | ||
| } | ||
| g := grant.NewGrant( | ||
| resource, | ||
| capability.Name, | ||
| principalID, | ||
| grant.WithAnnotation(&v2.GrantExpandable{ | ||
| EntitlementIds: []string{fmt.Sprintf("group:%s:%s", groupID, memberEntitlement)}, | ||
| Shallow: true, | ||
| }), | ||
| ) | ||
| rv = append(rv, g) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return rv, "", nil, nil | ||
| } |
There was a problem hiding this comment.
Filter grants to known capability entitlements.
If Tableau returns a capability not present in viewCapabilities, this will emit grants for entitlements that don’t exist, which can break reconciliation. Consider filtering to known capabilities before creating grants.
✅ Suggested filter
for _, grantee := range permissions {
for _, capability := range grantee.Capabilities.Capability {
+ if _, ok := viewCapabilities[capability.Name]; !ok {
+ continue
+ }
if capability.Mode != "Allow" {
continue
}🤖 Prompt for AI Agents
In `@pkg/connector/view.go` around lines 106 - 159, The Grants method on
viewResourceType may emit grants for unknown capability names; update Grants to
filter capability.Name against the known viewCapabilities set before creating
grants. Specifically, in Grants (function viewResourceType.Grants) build or
reuse a lookup (e.g., map[string]struct{} from viewCapabilities) and skip any
capability whose Name is not present; apply this check before the user grant
creation block (where rs.NewResourceID/resourceTypeUser is used) and before the
group grant creation block (where resourceTypeGroup and grant.WithAnnotation are
used) so only known entitlement names produce Grant objects.
| return rv, "", nil, nil | ||
| } | ||
|
|
||
| func (v *viewResourceType) Entitlements(_ context.Context, resource *v2.Resource, _ *pagination.Token) ([]*v2.Entitlement, string, annotations.Annotations, error) { |
pkg/connector/view.go
Outdated
| case resourceTypeGroup.Id: | ||
| err = v.client.DeleteViewGroupPermission(ctx, viewID, principalID, capabilityName, "Allow") | ||
| default: | ||
| l.Warn( |
There was a problem hiding this comment.
Warn -> Debug, always. Please leave a comment in the code to explain the warn if you think there's a reason to use warn.
| principalID, | ||
| grant.WithAnnotation(&v2.GrantExpandable{ | ||
| EntitlementIds: []string{fmt.Sprintf("group:%s:%s", groupID, memberEntitlement)}, | ||
| Shallow: true, |
There was a problem hiding this comment.
I noticed I think that views can also be assignable to Group Sets, which we don't support as a resource type, but maybe we should?
https://help.tableau.com/current/online/en-us/group_sets.htm
For example, you might restrict access to different workbook views based on a user’s regional group affiliation. Suppose you have users who are, contractors, contractor managers, full time employees, and full time managers accessing these workbook views.
Groups that have default permissions to view the workbook: All, North, South, East, West, Full Time, and Managers.
For the North Region Detailed view:
Permissions are based on group set: North Region
Groups in the group set: Full Time, North
Outcome: Only full time employees and full time managers in the north can see the data in the North Region Detailed view.For the All Region Detailed view:
Group set is called Managers
Groups in the group set: All, Managers
Outcome: Only contractor managers and full time managers can see the data in the All Region Detailed view.
3 participants
Summary by CodeRabbit
New Features
Bug Fixes