Skip to content

fix: enforce open restrictions and type-specific staff roles - #33

Merged
Musiker15 merged 9 commits into
MSK-Scripts:mainfrom
Uncharted-Roleplay:fix/permissions
Oct 23, 2026
Merged

Musiker15 merged 9 commits into
MSK-Scripts:mainfrom
Uncharted-Roleplay:fix/permissions

Conversation

@shane615

@shane615 shane615 commented Oct 10, 2026 •

Copy link
Copy Markdown
Contributor

Description

Permission fixes found while reviewing the bot before deploying it for our community (Uncharted Roleplay). Each commit is one change:

  • One eligibility check for every open path. New getOpenRefusal() is used by the panel button, both select menus, the questions modal and openTicket() itself. Before:
    • rolesWhoCanNotCreateTickets was validated in config.js but never checked anywhere.
    • cantAccess was checked only in the two select menus, not on the single-type tb_open path or in openTicket().
  • The open-ticket limit could be raced. The limit check and the DB insert are separated by guild.channels.create, so a burst of select-menu picks or modal submits each passed the check. openTicket() now holds a per-user in-flight lock across check, creation and insert, and returns { channel, refusal } so callers can show the real reason.
  • Type-specific staffRoles now apply to staff actions. The channel permissions already gave a type's staffRoles access instead of the global roles (as config.example.jsonc documents: "overrides"), but every action except reopen called isStaff(member) without the type. Members holding only a type's staff role could not claim, close, lock, move, add notes and so on; reminders pinged global roles that cannot see the channel.
    • isStaff(member, type) now uses the type's staffRoles when it has any, the global roles otherwise.
    • New isStaffIn(member, channelId) is used by all ticket-scoped commands, buttons and menus; new isAnyStaff(member) covers /stats, /broadcast and rating comments.
    • /broadcast only posts into tickets the sender is staff for.
    • Staff reminders and the "staff replied" DM use the ticket type's roles.
  • Close, lock and reopen apply to every participant, not only the creator. Users added with /add kept reading a closed ticket (including staff discussion after the close) and could still write in a locked one; reopen unlocked the creator even if the ticket was locked. Staff members who were added are skipped, because a member deny would override their role access. /add gives read-only access while the ticket is locked. /lock now defers, since it may edit several overwrites.
  • Only the ticket creator can rate a ticket (button and modal).
  • Re-checks: the close-reason modal re-checks STAFFONLY, and the delete confirmation refuses non-ticket channels. (/blacklist / /setup permissions stay with the server admin via Integrations, as requested in review.)

New locale keys messages.cannotCreateTickets, messages.ticketOpenInProgress and messages.onlyCreatorCanRate, translated in all languages.

Related Issue

None opened; these came out of a code review. Happy to open issues if you prefer that workflow.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 💥 Breaking change (fix or feature that changes existing behavior)
  • 🌍 Translation / locale update
  • 📖 Documentation only
  • 🔧 Refactor / chore (no functional change)

Breaking in one respect: global staff no longer act on tickets of a type that has its own staffRoles (they already could not see those channels). Servers that relied on the old fallback should list the global role in that type's staffRoles; config.example.jsonc now says so, and the changelog has a Breaking section for it.

How Was This Tested?

  • Node.js version: 24.20.0
  • npm test: all existing tests pass, plus new node:test suites (tests/open-eligibility.test.js including the concurrency case, tests/staff-roles.test.js, tests/participant-access.test.js, tests/rating.test.js).
  • Live test (2026-10-23): run on our test server (PostgreSQL, bot without Administrator) with a non-admin second account: open limit and double-pick race, type-only staff (claim, /lock, /unlock) vs. global staff, /add + lock + close for added users, rating by DM. All passed.

Checklist

  • My code follows the project's conventions (CommonJS, no TypeScript, tb_ custom-id prefix, client.logger, etc.)
  • No new production dependencies were added (or it was discussed in an issue first)
  • User-facing strings go through client.t(...) and all three locale files (main.json, en.json, de.json) are in sync (all eight locale files, in fact)
  • Any DB schema change uses an inline migration in initDatabase() (no schema change)
  • The bot still works without MSK_API_KEY (premium features degrade gracefully)
  • The bot starts cleanly (npm start) and I verified my change on a test server (see above)
  • I updated the relevant docs (README.md / README_GER.md, config.example.jsonc, CLAUDE.md) where behavior, config keys, commands or the schema changed (config.example.jsonc)
  • I added a CHANGELOG.md entry under [Unreleased]
  • My commit messages are in English and follow Conventional Commits (feat:, fix:, docs:, …)

Note on the related PRs

This is one of three PRs from the same review (robustness, permissions, transcripts). They are independent and each applies to main on its own, but they touch some of the same lines (ticketActions.js exports, ready.js, CHANGELOG.md). Following your order (#32, #34, then this one), I will rebase this after the other two are merged.

https://claude.ai/code/session_01Kp1SLbxxAbXw3f42C1qThH

@Musiker15

Copy link
Copy Markdown
Member

Thanks for digging into this one, there was more broken here than I expected.

Everything you describe is still the case on current main: rolesWhoCanNotCreateTickets is validated but never checked, cantAccess is missing on the single-type button, the open limit can be raced, anyone who sees an in-channel rating request can rate, close/lock/reopen only touch the creator, and almost every staff action calls isStaff(member) without the ticket type. That last one is the big one, since type-only staff can see the channel but can't do anything in it.

On the two points you raised:

  1. Type staffRoles replacing the global roles: yes, that's intended. It matches what the channel permissions already do and what the example config says ("overrides"). Please keep it, and keep the breaking change clearly marked in the changelog so server owners know they need to add the global role to a type if it should keep access.
  2. Enforcing ManageGuild / Administrator in /blacklist and /setup: please drop that part. I'd rather leave it to the server admin to open these commands up via Integrations. The rest of that commit (rating and modal re-checks, delete confirmation) is fine to keep.

Tests pass on your branch (186/186). Since this PR touches the same parts of ticketActions.js and ready.js as #32 and #34, I'd merge it last. Once those two are in, please rebase this one onto main.

shane615 added a commit to Uncharted-Roleplay/ucrp-ticketbot that referenced this pull request Oct 23, 2026
Matches upstream review of PR MSK-Scripts#33: the commands keep their default member
permissions, and admins restrict or open them up under Server Settings >
Integrations instead of a second check in code.

Claude-Session: https://claude.ai/code/session_01HZPZxD3edzJNFFsiWsHYY1
@shane615

Copy link
Copy Markdown
Contributor Author

Thanks for the clear answers. Done:

  • Dropped the ManageGuild / Administrator enforcement in /blacklist and /setup (the rest of that commit stays) and took it out of the description.
  • Kept the type staffRoles override and added a Breaking section at the top of the [Unreleased] changelog block.
  • Rebased onto main.

Verified live with a non-admin account: type-only staff can claim, /lock and /unlock tickets of their type and don't see other types; global staff see only types without their own staffRoles; added users can't write once a ticket is locked and lose access when it's closed. I'll rebase after #32 and #34 are merged and mark it ready then.

@Musiker15

Copy link
Copy Markdown
Member

#32, #34, #38 and #39 are all merged now, so this is the last one. Could you rebase onto main and mark it ready? It overlaps with #32 and #34 in ticketActions.js and ready.js, and CHANGELOG.md will conflict too. Keep the Breaking section at the top of [Unreleased] and put your other entries after the existing ones. I'll go through it once more after the rebase and merge it then.

- New getOpenRefusal() is the single eligibility check for the panel button,
  both select menus, the questions modal and openTicket() itself.
- rolesWhoCanNotCreateTickets was validated but never checked; it is now.
- cantAccess was skipped on the single-type button path and inside
  openTicket(); it is now checked on every path.
- openTicket() holds a per-user in-flight lock across the limit check,
  channel creation and DB insert, so a burst of select-menu picks or modal
  submits can no longer create several channels past maxTicketOpened.
- openTicket() returns { channel, refusal } so callers show the real reason.
- New locale keys messages.cannotCreateTickets and
  messages.ticketOpenInProgress (all languages).

Claude-Session: https://claude.ai/code/session_01Kp1SLbxxAbXw3f42C1qThH
A type's staffRoles already decided who gets access to the channel
(overriding the global roles), but every action except reopen checked
isStaff() without the type and always fell back to the global roles. So
type-only staff could not claim, close, lock, move or annotate their own
tickets, and reminders pinged roles that cannot see the channel.

- isStaff(member, type) uses the type's staffRoles when it has any, the
  global roles otherwise — the same rule as the channel permissions.
- New isStaffIn(member, channelId) resolves the ticket's type; all
  ticket-scoped commands, buttons and menus use it.
- New isAnyStaff(member) for /stats, /broadcast and rating comments;
  /broadcast only posts into tickets the sender is staff for.
- Staff reminders and "staff replied" DMs use the ticket type's roles.
- /blacklist and /setup enforce ManageGuild / Administrator in code instead
  of relying only on setDefaultMemberPermissions.
- Document the override rule in config.example.jsonc.

Claude-Session: https://claude.ai/code/session_01Kp1SLbxxAbXw3f42C1qThH
Closing and locking only changed the creator's overwrite, so users added
with /add kept reading a closed ticket (including staff discussion after the
close) and could still write in a locked one. Reopen also unlocked the
creator even when the ticket was still locked.

- New editParticipantAccess() edits the creator and every member overwrite
  except the bot and members who are staff for the ticket's type (a member
  deny would override their role access).
- Close, /lock, /unlock, the dashboard lock action and reopen use it.
- Reopen restores SendMessages only if the ticket is not locked.
- /add gives read-only access while the ticket is locked.

Claude-Session: https://claude.ai/code/session_01Kp1SLbxxAbXw3f42C1qThH
- Only the ticket creator can rate a ticket (button and modal); staff and
  users added with /add could rate it from an in-channel request.
- The close-reason modal re-checks whoCanCloseTicket: STAFFONLY, since a
  modal submit is its own interaction.
- The delete confirmation refuses to delete a channel that is not a ticket.
- New locale key messages.onlyCreatorCanRate (all languages).

Claude-Session: https://claude.ai/code/session_01Kp1SLbxxAbXw3f42C1qThH
- /lock and /unlock defer before editing each participant's permissions;
  several edits could outlast Discord's 3 s reply window.
- config.example.jsonc: tickets of a type removed from the config fall back
  to the global staff roles.

Claude-Session: https://claude.ai/code/session_01Kp1SLbxxAbXw3f42C1qThH
Review feedback: admins open these commands up via Integrations on purpose,
so the code no longer re-checks ManageGuild / Administrator on top of
setDefaultMemberPermissions.

Claude-Session: https://claude.ai/code/session_01HZPZxD3edzJNFFsiWsHYY1
Review feedback: server owners need to see that a type with its own
staffRoles no longer falls back to the global roles.

Claude-Session: https://claude.ai/code/session_01HZPZxD3edzJNFFsiWsHYY1
openTicket now looks the member up for getOpenRefusal, so the MSK-Scripts#38 test's
fake guild needs members.fetch; and since MSK-Scripts#38 the channel overwrites read
client.user.id, so the race test's fake client needs a bot user. Test
fixtures only.

Claude-Session: https://claude.ai/code/session_01HZPZxD3edzJNFFsiWsHYY1
@shane615

Copy link
Copy Markdown
Contributor Author

Rebased onto main and marked ready.

Tests pass (206/206).

@shane615
shane615 marked this pull request as ready for review October 23, 2026 21:39
@Musiker15
Musiker15 merged commit e8d7008 into MSK-Scripts:main Oct 23, 2026
5 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