Skip to content

test: ATF coverage for the user/admin security split - #3

Merged
jschuller merged 2 commits into
mainfrom
fix/review-2026-10-17
Oct 19, 2026
Merged

jschuller merged 2 commits into
mainfrom
fix/review-2026-10-17

Conversation

@jschuller

Copy link
Copy Markdown
Owner

This PR fixes the high finding from the 2026-10-17 repo review: the ATF suite didn't test the security split it advertises.

What changed

  • itemUserRoleTest: an admin seeds an item, then the test impersonates a user who has only x_snc_example.user.
    • ATF Record Update returns record_not_updated and Record Delete returns record_not_deleted, both with security enforced. The record is checked unchanged afterwards.
    • Through AppService: list returns can_write/can_delete false, update and delete return 403, and create returns 201.
  • itemAdminRoleTest: impersonates a user who has only x_snc_example.admin.
    • The listed item shows can_write/can_delete true.
    • Update returns 200, delete returns 204, and update after delete returns 404.
    • This tests the app's admin role itself. sysadmin would pass the ACLs through admin override, so a sysadmin run proves nothing about the role.
  • Suite: both tests are wired in. keys.ts is regenerated, and there is a CHANGELOG entry.

Verification

  • npm run lint, npm run build and npm run build:ci (--frozenKeys) pass.
  • I inspected the built XML. The step order, impersonation, enforce-security flags and the {{step[…].record_id}} wiring are all correct.
  • An independent review checked the step names and parameters against SDK 4.13.6 Test.ts. Its should-fix, that the admin path was running as sysadmin, is addressed by itemAdminRoleTest.
  • gitleaks flags 20 generic-api-key lines in src/fluent/generated/keys.ts. They are SDK-generated record sys_ids, the same false positive that already appears 17 times in main's history.

Not verified: needs an instance

The ATF tests have not run on an instance yet. To verify:

  1. npm run deploy -- --auth <alias>, then npm run test:instance -- --auth <alias>.
  2. Expect 4 tests to pass in the suite.

Specifically unconfirmed:

  • runServerSideScript sees the impersonated user after the intervening steps.
  • A user-role-only user can list and create through GlideRecordSecure.
  • Record Update/Delete under ACL denial return their not_* asserts rather than a step error.

Conflict note

Open PR #2 also adds a section under [Unreleased] in CHANGELOG.md. Keep both sections.

CRB2205 and others added 2 commits October 18, 2026 19:39
The suite only tested a user with no roles trying to create, so a regression
that let the user role update or delete would have passed. Add a test that
seeds an item as admin, impersonates a user holding only x_snc_example.user,
and checks that secured record update and delete are denied, that AppService
returns 403 for them and 201 for create, and that can_write/can_delete are
false. Extend the admin service test with update (200), delete (204) and
update after delete (404). keys.ts regenerated by npm run build.

Co-Authored-By: Claude <noreply@anthropic.com>
The admin CRUD checks ran as the ATF runner, a sysadmin who passes every
ACL, so they did not show that x_snc_example.admin grants write and delete.
Move them into a test that impersonates a user holding only that role:
AppService reports can_write/can_delete true, update returns 200, delete
204, and update after delete 404. keys.ts regenerated by npm run build.

Co-Authored-By: Claude <noreply@anthropic.com>
@jschuller
jschuller merged commit db98849 into main Oct 19, 2026
2 checks passed
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.

2 participants