Skip to content

feat(platform): add Roles service for role and access management - #666

Open
Sarath1018 wants to merge 1 commit into
feat/platform-directoryfrom
feat/platform-roles
Open

feat(platform): add Roles service for role and access management#666
Sarath1018 wants to merge 1 commit into
feat/platform-directoryfrom
feat/platform-roles

Conversation

@Sarath1018

Copy link
Copy Markdown
Collaborator

Summary

PR 4/4 in the platform RBAC stack (Users → Groups → Directory → Roles). Stacked on #665 — merge order: #663, #664, #665, then this; the diff here is Roles-only. Cloudflare whitelist: UiPath/apps-dev-tools#146 (new pap_ base + patterns) must deploy before browser apps can call these.

Adds PlatformRoleService (exported as Roles) to the /platform subpath, backed by the Authorization service (pap_, organization-level routing via the new AUTHORIZATION_BASE). This completes the RBAC story: put users in Groups, grant Roles to the groups, answer "what can this user do" via getEffectiveAccess().

Method Endpoint Notes
getAll(options?) GET /pap_/api/roles Built-ins included, actionDetails permissions on each; filters + paged/fetch-all
getById(roleId) GET /pap_/api/roles/{id} Bound role.delete()
upsert(request) PUT /pap_/api/roles Create-or-update custom role; API returns only {createdRoleId} → service follows up with a read (shared untracked fetchRole avoids double telemetry)
deleteById(roleId) DELETE /pap_/api/roles/{id}
getAssignments(scope, options?) GET /pap_/api/userroleassignments Grouped by principal; scope API-required (/ = org)
updateAssignments(changes) PATCH /pap_/api/userroleassignments Atomic toAdd/toDelete batch
exportAssignments() GET .../export CSV via blob response type
getEffectiveAccess(request) POST /pap_/api/geteffectiveaccess Envelope reshaped: roles[] (each with granting assignments), grantedServices, grantedRoles
getActions(options?) GET /pap_/api/actions Permission catalog — source of actionsGrantedByRole names

Live-verified API quirks (all encoded with comments + tests)

  • actionsGrantedByRole takes fully qualified action names, not GUIDs (400 "actions that do not exist" with GUIDs)
  • roleDescription is server-required on upsert → required field per the structurally-required convention
  • Role type strings are inconsistent across endpoints (BUILTIN/CUSTOM in lists, BuiltIn/Custom in single reads) → normalized to PlatformRoleType via a four-casing map
  • userroleassignments rejects top > 10 while roles allows 1000 → separate AUTHORIZATION_{ROLES,ASSIGNMENTS}_MAX_PAGE_SIZE constants
  • scopeType deliberately left as string — the value vocabulary is unpublished (observed: ORGANIZATION, TENANT, ANY)

Testing

  • Unit: 2444 passing (55 new: transforms incl. casing normalization, fetch-all + paginated filter passing on both list methods incl. the roleIds array, upsert read-follow-up, wire body assertions, CSV blob, effective-access reshape, all validation branches)
  • Model test: bound delete() delegation
  • Integration (live, 8/8): custom-role lifecycle with cleanup, assignment grant/revoke round-trip on a probe role, CSV export, effective access, action catalog
  • Convention review: two-iteration loop clean (3 Important findings fixed: getActions JSDoc said GUIDs, typed destructure instead of as unknown as in toAction, dedicated roles max-page-size constant)

Docs

docs/oauth-scopes.md (honest note: the Authorization service publishes no OAuth scopes yet — access governed by the caller's platform roles, PAT-verified), docs/pagination.md (2 rows), mkdocs.yml nav.

Stack

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://UiPath.github.io/uipath-typescript/pr-preview/pr-666/

Built to branch gh-pages at 2026-09-10 19:15 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

private toEffectiveRole(raw: RawPlatformEffectiveRole): PlatformEffectiveRole {
const { roleAssignments, ...role } = raw;
return {
...role,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toEffectiveRole() skips PlatformRoleTypeMap normalization, but toPrincipalAssignments() (just above, line ~377) applies it to every assignment's roleType. The PR description explicitly calls out that role-type strings are inconsistent across endpoints (BUILTIN/CUSTOM in lists vs BuiltIn/Custom in single reads). If the effective-access endpoint also returns mixed casing, callers comparing role.roleType / assignment.roleType from getEffectiveAccess() with values from getAll() or getAssignments() will see silently inconsistent strings.

Two options:

  1. Apply the same normalization and type both PlatformEffectiveRole.roleType and PlatformEffectiveRoleAssignment.roleType as PlatformRoleType | null:
private toEffectiveRole(raw: RawPlatformEffectiveRole): PlatformEffectiveRole {
  const { roleAssignments, roleType, ...rest } = raw;
  return {
    ...rest,
    roleType: roleType != null ? (PlatformRoleTypeMap[roleType] ?? roleType) : null,
    assignments: (roleAssignments ?? []).map(assignment =>
      applyDataTransforms(
        transformData({ ...assignment }, PlatformRoleMap) as Record<string, unknown>,
        { field: 'roleType', valueMap: PlatformRoleTypeMap }
      ) as unknown as PlatformEffectiveRoleAssignment
    ),
  };
}
  1. If live-testing confirmed the effective-access endpoint consistently returns BUILTIN/CUSTOM (already matching the enum values), leave roleType as string | null but add an inline comment explaining why normalization is intentionally skipped here.

@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Review summary

One finding posted:

src/services/platform/roles.ts line 394toEffectiveRole() skips PlatformRoleTypeMap normalization

toPrincipalAssignments() applies PlatformRoleTypeMap to normalize every assignment's roleType to PlatformRoleType. toEffectiveRole() does not: the effective role's own roleType is spread via ...role unmodified, and nested assignments only get the timestamp rename — no roleType normalization. Since the PR documents that role-type strings are inconsistent across endpoints (BUILTIN/CUSTOM vs BuiltIn/Custom), and the effective-access endpoint is yet another code path, callers comparing roleType values across getAssignments() and getEffectiveAccess() responses could silently see different strings for the same role. See the inline comment for two resolution options.

@Sarath1018
Sarath1018 requested a review from a team September 9, 2026 18:02
Comment on lines +130 to +140
it('should retrieve a role by ID with the transform pipeline applied', async () => {
mockApiClient.get.mockResolvedValue(createBasicRawPlatformRole());

const role = await rolesService.getById(roleId);

expect(mockApiClient.get).toHaveBeenCalledWith(AUTHORIZATION_ENDPOINTS.ROLE.GET_BY_ID(roleId), {});
expect(role.id).toBe(roleId);
expect(role.createdTime).toBe(PLATFORM_ROLE_TEST_CONSTANTS.CREATED_ON);
expect((role as any).createdOn).toBeUndefined();
expect(typeof role.delete).toBe('function');
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getById endpoint returns mixed-case role types (BuiltIn/Custom) while list endpoints return all-caps (BUILTIN/CUSTOM) — the PR description explicitly calls this out as a live-API quirk. The mock here uses the default type: 'BUILTIN', which passes through PlatformRoleTypeMap unchanged, so this test never exercises the normalization that getById actually needs. Per convention, mocks for transform completeness tests must use raw wire-format values (not post-transform values), and getAll/getById need separate completeness tests.

Use the single-read wire format to make the assertion meaningful:

Suggested change
it('should retrieve a role by ID with the transform pipeline applied', async () => {
mockApiClient.get.mockResolvedValue(createBasicRawPlatformRole());
const role = await rolesService.getById(roleId);
expect(mockApiClient.get).toHaveBeenCalledWith(AUTHORIZATION_ENDPOINTS.ROLE.GET_BY_ID(roleId), {});
expect(role.id).toBe(roleId);
expect(role.createdTime).toBe(PLATFORM_ROLE_TEST_CONSTANTS.CREATED_ON);
expect((role as any).createdOn).toBeUndefined();
expect(typeof role.delete).toBe('function');
});
it('should retrieve a role by ID with the transform pipeline applied', async () => {
// getById returns mixed-case type strings (live-API quirk) — use the single-read wire format
mockApiClient.get.mockResolvedValue(createBasicRawPlatformRole({ type: 'BuiltIn' }));
const role = await rolesService.getById(roleId);
expect(mockApiClient.get).toHaveBeenCalledWith(AUTHORIZATION_ENDPOINTS.ROLE.GET_BY_ID(roleId), {});
expect(role.id).toBe(roleId);
expect(role.createdTime).toBe(PLATFORM_ROLE_TEST_CONSTANTS.CREATED_ON);
expect((role as any).createdOn).toBeUndefined();
// PlatformRoleTypeMap normalizes 'BuiltIn' (single-read wire) → PlatformRoleType.BuiltIn ('BUILTIN')
expect(role.type).toBe(PlatformRoleType.BuiltIn);
expect(typeof role.delete).toBe('function');
});

@claude

claude Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Review summary

One new finding posted (lines 130-140 of the test file, getById transform test does not exercise mixed-case normalization).

The PR documents that single-read endpoints return BuiltIn/Custom while list endpoints return BUILTIN/CUSTOM. The getById transform completeness test uses the default mock (type: 'BUILTIN'), which passes through PlatformRoleTypeMap unchanged so the key normalization code path for getById is never exercised. The fix is to supply type: 'BuiltIn' in the mock and assert role.type === PlatformRoleType.BuiltIn.

The pre-existing unresolved thread on src/services/platform/roles.ts line 394 (toEffectiveRole skipping PlatformRoleTypeMap normalization) is still open and was not re-raised.

Adds PlatformRoleService (exported as Roles) to the /platform subpath,
backed by the Authorization service (org-level routing, new
AUTHORIZATION_BASE):

- getAll(options?) / getById(roleId) — roles with their permissions
  (actionDetails); paged or fetch-all
- upsert(request) — create-or-update custom roles; the API returns only
  {createdRoleId}, so the service follows up with a read (shared
  private fetchRole keeps getById/upsert singly tracked); bound
  role.delete()
- deleteById(roleId)
- getAssignments(scope, options?) — assignments grouped by principal;
  scope is API-required ('/' = whole organization)
- updateAssignments(changes) — atomic add/remove batch
- exportAssignments() — CSV via blob response
- getEffectiveAccess(request) — a principal's effective roles in a
  tenant, envelope reshaped (roles/assignments/grantedServices/
  grantedRoles)
- getActions(options?) — permission definitions catalog

Live-verified API quirks encoded: actionsGrantedByRole takes action
names (not GUIDs); roleDescription is server-required; role type
strings are inconsistent across endpoints (BUILTIN vs BuiltIn —
normalized to the enum); userroleassignments caps top at 10 while
roles allows 1000 (separate max-page-size constants); createdOn
renamed to createdTime throughout.

Verified against the live API: 2444 unit tests passing, integration
8/8 (role lifecycle, assignment grant/revoke, CSV export, effective
access), build clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
expect(spec.params.securityPrincipalId).toBe(PLATFORM_USER_TEST_CONSTANTS.USER_ID);
expect(spec.params.top).toBe(10);
expect(spec.params).not.toHaveProperty('$scope');
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The paginated path for getAssignments is missing a companion test for roleIds. The fetch-all path handles roleIds with an explicit conditional spread (opts.roleIds !== undefined && { roleIds: opts.roleIds }) to preserve array serialization — a distinct code path from the paginated route, which relies on PaginationHelpers.getAll() receiving roleIds inside opts. Per the convention: "When sibling methods share a configuration pattern, each method needs its own test for that pattern" — but there's currently no paginated-path test that passes roleIds and asserts it reaches the API call without a $ prefix.

Suggested change
});
});
it('should pass roleIds on the paginated path too', async () => {
mockApiClient.get.mockResolvedValue(
createRawPlatformRoleAssignmentListResponse([createBasicRawPlatformPrincipalRoleAssignments()], 1)
);
await rolesService.getAssignments('/', { roleIds: [roleId], pageSize: 5 });
const spec = mockApiClient.get.mock.calls[0][1] as { params: Record<string, unknown> };
expect(spec.params.roleIds).toEqual([roleId]);
expect(spec.params).not.toHaveProperty('$roleIds');
});

@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Review summary

One new finding posted:

tests/unit/services/platform/roles.test.ts line 308 — Missing roleIds coverage on the paginated path of getAssignments

The fetch-all path handles roleIds with an explicit conditional spread (different code than the paginated path, which passes it through opts to PaginationHelpers.getAll()). Per convention, each sibling path needs its own test for any shared configuration pattern. A companion test is suggested inline.

The two pre-existing unresolved threads (line 394: toEffectiveRole skipping PlatformRoleTypeMap normalization; line 140: getById transform test using list-endpoint wire format instead of single-read format) are still open and were not re-raised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant