Repository navigation
ROCK-8640 Forward-port S&S connect gate card fixes to v16 - #8
Merged
stphnlee merged 2 commits intoAug 17, 2026
Merged
Conversation
… cards
The kanban card's action menu ("...") renders its Connect item from core
ConnectionRequestService.CanConnect() - placement group assigned, state not
Connected/Inactive, ShowConnectButton - which knows nothing about the S&S
gate. So the item stayed visible on secured opportunities at any status. The
prior commit refuses the postback server-side; this hides the affordance so
the menu matches the gated modal button.
Adds a status-based overload to SeccConnectGateHelper (the request-based
overload now delegates to it) so the board can ask "could this person connect
a request at this status?" per status column. GetUserConnectableStatusIds()
runs that shared gate against every status on the selected opportunity, also
applying the edit-rights check the modal button requires, and the resulting
ids ride both board script payloads. The JavaScript checks membership only -
no gate logic lives client-side.
The helper overload should be carried to hotfix-1.16.12 with the port of this
change so the two branches keep a byte-identical helper.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
(cherry picked from commit fb451dd)
…nity The Connection Request Board resolved the connect gate's opportunity from the board's current selection, while the connection request identifier arrives from the client postback, so the two can disagree. A user with edit rights on an opportunity that does not require Safety & Security to connect could send a connect postback carrying the identifier of a request in a gated opportunity: the gate read SecurityToConnect and ConnectableStatuses from the selected opportunity, allowed it, and MarkRequestConnected then connected the client-supplied request. Resolve the opportunity from the request instead, which is what ConnectionRequestDetail already does. The selected opportunity is still used when there is no request in context (modal add mode), since that is the opportunity the new request is created in, and it is reused directly when it already matches so the normal path does not take an extra query. This predates the card gate work - it is present both in the merged 13.7 change and in hotfix-1.16.12 - so this commit stands on its own and can be cherry-picked to v16 independently of the card menu change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit d20cad2)
There was a problem hiding this comment.
Pull request overview
Forward-ports the SECC Safety & Security “Connect” gate parity fixes into the v16 Connection Request Board so the card action-menu Connect item is hidden consistently with the modal Connect button, and server-side enforcement evaluates against the request’s actual opportunity.
Changes:
- Adds a server-computed
userConnectableStatusIdspayload and applies it client-side to suppress the card menu’s Connect action when gated. - Fixes server-side gate evaluation to resolve the opportunity from the request being connected (not just the currently selected board opportunity).
- Extends
SeccConnectGateHelperwith a status-based overload so board/status-column evaluation shares the same gate implementation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| RockWeb/Scripts/Rock/Controls/ConnectionRequestBoard/connectionRequestBoard.js | Captures server-provided connectable status IDs and applies them when rendering cards to hide the Connect action item. |
| RockWeb/Blocks/Connection/ConnectionRequestBoard.ascx.cs | Adds gate-aware opportunity resolution and computes per-status connectability list to send to the client. |
| RockWeb/App_Code/SeccConnectGateHelper.cs | Adds a status-based overload and delegates request-based gate checks to it. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+2904
to
+2907
| return new ConnectionOpportunityService( new RockContext() ) | ||
| .Queryable() | ||
| .AsNoTracking() | ||
| .FirstOrDefault( co => co.Id == connectionRequest.ConnectionOpportunityId ); |
Comment on lines
+2872
to
+2873
| connectionRequest, | ||
| GetGateConnectionOpportunity( connectionRequest ), |
stphnlee
approved these changes
Aug 17, 2026
stphnlee
left a comment
stphnlee
left a comment
There was a problem hiding this comment.
There are some improvements that need to be made, but I'm approving this PR to keep feature parity with our current production environment. We can implement the fixes after the upgrade.
stphnlee
added a commit
that referenced
this pull request
Oct 2, 2026
#19) * ROCK-9044: Close connect-gate enforcement gaps and fix review findings Follow-up to the ROCK-8640 Safety & Security connect gate (PR #8). Addresses all 13 findings from the code review of the v16 port. Enforcement - Bulk Update Requests was ungated and could move requests into Connected. Add a Safety & Security Role block setting and run the shared gate per request, against the target opportunity and status, before any write. - Card-menu and modal Connect now require the request to be in the selected opportunity, since the edit check is evaluated against that opportunity and the request id is client-supplied. - The edit-modal save gates against the opportunity and status the request is being saved with, not its pre-save opportunity. Crashes and feedback - Resolve and null-check the opportunity before the edit check that dereferenced it (yellow screen when the opportunity was deactivated). - Refused connects now show a message instead of silently doing nothing. Display - The card-menu gate list is evaluated with no request in context so one request's per-request Edit result no longer applies to every card. This also keeps the client options hash stable. - Clear the cached request after a transfer so the modal gates against the new opportunity. - Re-render the card after a drag-and-drop so Connect reflects the new status. Cleanup - Hoist the status-independent half of the gate into SeccConnectGateHelper.GetConnectableStatusIds (one evaluation per board render instead of one per status) and only LoadAttributes when needed. - Use the request's ConnectionOpportunity navigation property instead of a hand-rolled query on an unowned RockContext. - Collapse the helper's forwarding wrapper; drop the fabricated Active state. - JS: capture the gate list before fetchAndRefreshCard's early return. Cherry-pick: applies cleanly on the ROCK-8640 commit alone. Depends on secc/ROCK-8640-connect-gate; independent of ROCK-9046 and ROCK-9138. Config after deploy: set "Safety & Security Role" on the Bulk Update Requests block instance (fails closed to Rock Administrators until set). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * ROCK-9044: Fix second-round review findings on connect-gate enforcement Connection Request Board: - Refuse the modal edit-save when the request is not in the selected opportunity, so Edit rights on one opportunity cannot rewrite or connect a request in another. - Refuse the edit-save with a notification when the connect gate blocks a Connected state, instead of silently saving the old state. - Require the request to be in the selected opportunity for the view-mode Connect button, matching its click handler. - Null-guard the selected opportunity inside CanUserEditConnectionRequest so every caller fails closed instead of throwing when the opportunity is deactivated mid-session. - Reset the block-level notification box on every postback so a refused connect does not leave a persistent banner; title the refusal "Not Authorized". - Drop GetGateConnectionOpportunity; the gate now always evaluates the selected opportunity, which every caller already requires to be the request's own. Bulk Update Requests: - Null-check the target opportunity before the gate so the refusal message cannot throw. - Also gate already-Connected requests being moved into a different opportunity with State left blank. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * ROCK-9044: Fix third-round review findings on connect-gate enforcement - Modal save no longer refuses edits to a request that is already Connected; only a transition into Connected is gated (regression from the refuse-the-save change). - Bulk update skips requests already Connected on the target opportunity, and gates every other request entering Connected as a new connect, so the target's ConnectableStatuses always apply. - Clear the request modal notification when a request is opened from a card or the grid, so a refusal does not carry over. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ROCK-8640 — Forward-port S&S connect gate card fixes to v16
Internal SECC PR. Base:
hotfix-1.16.12.Why
v16 got the connect gate and its server-side enforcement via PR #5 (
17acbdfb62).Two follow-up fixes landed on the 13.7 line eight days later and never reached v16 —
hotfix-1.13.7andhotfix-1.16.12share no common ancestor, so nothing mergesacross the version lines. This brings v16 to parity with the merged 13.7 tip
(
b320d58c0d, PR #7).Commits
ab1f32cd7bfb451dda1c70eee0ea45d20cad2570Verified
SeccConnectGateHelper.csandconnectionRequestBoard.jsbyte-identical tob320d58c0dhotfix-1.16.12is a direct ancestor → fast-forward, nothing reverted