diff --git a/RockWeb/App_Code/SeccConnectGateHelper.cs b/RockWeb/App_Code/SeccConnectGateHelper.cs index ba45bf62f85..b1894658588 100644 --- a/RockWeb/App_Code/SeccConnectGateHelper.cs +++ b/RockWeb/App_Code/SeccConnectGateHelper.cs @@ -15,6 +15,7 @@ // // using System; +using System.Collections.Generic; using System.Linq; using Rock; @@ -95,12 +96,7 @@ public static bool IsPersonAuthorizedToConnect( Person currentPerson, Guid? safe /// public static bool CanConnect( ConnectionRequest connectionRequest, ConnectionOpportunity opportunity, Person currentPerson, Guid? safetySecurityRoleGuid ) { - if ( connectionRequest == null ) - { - return CanConnect( ( int? ) null, null, opportunity, currentPerson, safetySecurityRoleGuid ); - } - - return CanConnect( connectionRequest.ConnectionStatusId, connectionRequest.ConnectionState, opportunity, currentPerson, safetySecurityRoleGuid ); + return CanConnect( connectionRequest?.ConnectionStatusId, connectionRequest?.ConnectionState, opportunity, currentPerson, safetySecurityRoleGuid ); } /// @@ -111,12 +107,68 @@ public static bool CanConnect( ConnectionRequest connectionRequest, ConnectionOp /// public static bool CanConnect( int? connectionStatusId, ConnectionState? connectionState, ConnectionOpportunity opportunity, Person currentPerson, Guid? safetySecurityRoleGuid ) { + List connectableStatuses; + if ( !TryGetStatusRestriction( opportunity, currentPerson, safetySecurityRoleGuid, out connectableStatuses ) ) + { + return false; + } + + if ( connectableStatuses == null || !connectionStatusId.HasValue ) + { + return true; + } + + return connectableStatuses.Contains( connectionStatusId.Value ) + || connectionState == ConnectionState.Connected; + } + + /// + /// Returns the subset of at which the person could connect a + /// (not yet connected) request on the opportunity. Evaluates the opportunity-level half of the gate + /// once instead of once per status, which is what the board needs when it builds its card action menu. + /// Fails closed: returns an empty list if the opportunity cannot be resolved. + /// + public static List GetConnectableStatusIds( IEnumerable connectionStatusIds, ConnectionOpportunity opportunity, Person currentPerson, Guid? safetySecurityRoleGuid ) + { + var statusIds = new List(); + + if ( connectionStatusIds == null ) + { + return statusIds; + } + + List connectableStatuses; + if ( !TryGetStatusRestriction( opportunity, currentPerson, safetySecurityRoleGuid, out connectableStatuses ) ) + { + return statusIds; + } + + statusIds.AddRange( connectableStatuses == null + ? connectionStatusIds + : connectionStatusIds.Where( connectableStatuses.Contains ) ); + + return statusIds; + } + + /// + /// Evaluates the status-independent half of the gate. Returns false when the person may not connect + /// anything on the opportunity. Returns true otherwise, with + /// set to the status ids the opportunity restricts connecting to, or null when there is no restriction. + /// + private static bool TryGetStatusRestriction( ConnectionOpportunity opportunity, Person currentPerson, Guid? safetySecurityRoleGuid, out List connectableStatuses ) + { + connectableStatuses = null; + if ( opportunity == null ) { return false; } - opportunity.LoadAttributes(); + if ( opportunity.Attributes == null ) + { + opportunity.LoadAttributes(); + } + var requiresSecurityToConnect = GetRequiresSecurityToConnect( opportunity ); if ( !requiresSecurityToConnect.HasValue || !requiresSecurityToConnect.Value ) @@ -129,23 +181,18 @@ public static bool CanConnect( int? connectionStatusId, ConnectionState? connect return false; } - var connectableStatuses = opportunity.GetAttributeValue( "ConnectableStatuses" ).SplitDelimitedValues() + var statuses = opportunity.GetAttributeValue( "ConnectableStatuses" ).SplitDelimitedValues() .Select( v => v.AsIntegerOrNull() ) .Where( v => v.HasValue ) + .Select( v => v.Value ) .ToList(); - if ( connectableStatuses.Count == 0 ) - { - return true; - } - - if ( !connectionStatusId.HasValue ) + if ( statuses.Count > 0 ) { - return true; + connectableStatuses = statuses; } - return connectableStatuses.Contains( connectionStatusId.Value ) - || connectionState == ConnectionState.Connected; + return true; } } } diff --git a/RockWeb/Blocks/Connection/BulkUpdateRequests.ascx.cs b/RockWeb/Blocks/Connection/BulkUpdateRequests.ascx.cs index c75c4ddc026..f427e227d73 100644 --- a/RockWeb/Blocks/Connection/BulkUpdateRequests.ascx.cs +++ b/RockWeb/Blocks/Connection/BulkUpdateRequests.ascx.cs @@ -51,6 +51,13 @@ namespace RockWeb.Blocks.Connection Order = 1, DefaultValue = Rock.SystemGuid.Page.CONNECTIONS_BOARD )] + [SecurityRoleField( + "Safety & Security Role", + Description = "Members of this security role (plus Rock Administrators) can bulk update requests into the Connected state on opportunities that require security to connect. If an opportunity requires security to connect and no role is set here, only Rock Administrators can.", + IsRequired = false, + Order = 2, + Key = AttributeKeys.SafetySecurityRole )] + #endregion [Rock.SystemGuid.BlockTypeGuid( "175158F8-F10E-476F-809E-A76825E0AC5D" )] @@ -61,6 +68,7 @@ public partial class BulkUpdateRequests : RockBlock private static class AttributeKeys { public const string PreviousPage = "PreviousPage"; + public const string SafetySecurityRole = "SafetySecurityRole"; } #endregion AttributeKeys @@ -403,6 +411,47 @@ protected void btnConfirm_Click( object sender, EventArgs e ) .Where( cr => RequestIdsState.Contains( cr.Id ) && ( ( includeNoCampus && !cr.CampusId.HasValue ) || selectedCampusIds.Contains( cr.CampusId.Value ) ) ) .ToList(); + // SECC (ROCK-9044): nothing has been modified yet, so returning from either guard below persists nothing. + if ( connectionOpportunity == null ) + { + mdConfirmUpdateRequests.Hide(); + ShowNotification( NotificationBoxType.Danger, "The selected connection opportunity could not be found. No requests were updated." ); + return; + } + + // SECC (ROCK-9044): the State list offers Connected, which makes this the one board path that can move + // requests into Connected without the Safety & Security connect gate the board and detail blocks enforce. + // Apply the same gate here, against the opportunity and status the requests are being moved to. A request + // enters Connected on the target opportunity when it ends up Connected (State set to Connected, or left + // unchanged on an already Connected request) and was not already Connected on that opportunity. Each one + // is gated as a new connect (no state passed), so the target's ConnectableStatuses always apply. + var targetState = ddlState.SelectedValueAsEnumOrNull(); + var targetOpportunityId = connectionOpportunity.Id; + var targetStatusId = ddlStatus.SelectedValue.AsIntegerOrNull(); + var safetySecurityRoleGuid = GetAttributeValue( AttributeKeys.SafetySecurityRole ).AsGuidOrNull(); + + var blockedCount = connectionRequests.Count( cr => + ( targetState ?? cr.ConnectionState ) == ConnectionState.Connected + && !( cr.ConnectionState == ConnectionState.Connected && cr.ConnectionOpportunityId == targetOpportunityId ) + && !SeccConnectGateHelper.CanConnect( + targetStatusId ?? cr.ConnectionStatusId, + null, + connectionOpportunity, + CurrentPerson, + safetySecurityRoleGuid ) ); + + if ( blockedCount > 0 ) + { + mdConfirmUpdateRequests.Hide(); + ShowNotification( + NotificationBoxType.Danger, + string.Format( + "You are not authorized to connect {0} of the selected connection requests on {1}. No requests were updated.", + blockedCount.ToString( "N0" ), + connectionOpportunity.Name.EncodeHtml() ) ); + return; + } + foreach ( var connectionRequest in connectionRequests ) { connectionRequest.ConnectionOpportunityId = ddlOpportunity.SelectedValue.AsInteger(); diff --git a/RockWeb/Blocks/Connection/ConnectionRequestBoard.ascx.cs b/RockWeb/Blocks/Connection/ConnectionRequestBoard.ascx.cs index 53724f7878f..5e647d90448 100644 --- a/RockWeb/Blocks/Connection/ConnectionRequestBoard.ascx.cs +++ b/RockWeb/Blocks/Connection/ConnectionRequestBoard.ascx.cs @@ -747,6 +747,11 @@ protected override void OnLoad( EventArgs e ) } else { + // ROCK-9044: the block-level notification box is only ever shown, never hidden, and it lives in an + // always-updating panel - so a message raised on one postback (e.g. a refused connect) would persist + // across every later partial postback. Reset it here so a message lives for exactly one postback. + nbNotificationBox.Visible = false; + var causingControlClientId = Request["__EVENTTARGET"].ToStringSafe(); var causingControl = Page.FindControl( causingControlClientId ); @@ -850,6 +855,8 @@ will properly support deletes from list view mode until we rewrite the block if ( action == "view" ) { + // ROCK-9044: a refusal shown for the previously opened request must not carry over to this one. + HideRequestModalNotification(); ViewAllActivities = false; IsRequestModalAddEditMode = false; RequestModalViewModeSubMode = RequestModalViewModeSubMode_View; @@ -860,11 +867,17 @@ will properly support deletes from list view mode until we rewrite the block if ( action == "connect" ) { // ROCK-8640: enforce edit rights and the S&S connect gate on the card-menu connect action. - if ( CanUserEditConnectionRequest() && CanUserConnect() ) + // ROCK-9044: the request id is client-supplied, so also require the request to be in the selected + // opportunity (the one the edit check runs against), and tell the user when the connect is refused. + if ( IsRequestInSelectedOpportunity() && CanUserEditConnectionRequest() && CanUserConnect() ) { MarkRequestConnected(); RefreshRequestCard(); } + else + { + ShowError( ConnectNotAuthorizedMessage, ConnectNotAuthorizedTitle ); + } return; } @@ -874,6 +887,11 @@ will properly support deletes from list view mode until we rewrite the block if ( newStatusId.HasValue && newIndex.HasValue ) { ProcessConfirmedDragEvent( newStatusId.Value, newIndex.Value ); + + // ROCK-9044: the drop changed the request's status, which is an input to the connect gate, + // but the client re-used the original card markup. Re-render the card so its Connect item + // reflects the new status. + RefreshRequestCard(); } return; @@ -1105,10 +1123,13 @@ private void BindRequestModalViewMode() which ultimately controlling btnRequestModalViewModeConnect Visibility. */ //btnRequestModalViewModeConnect.Visible = viewModel.CanConnect && CanUserEditConnectionRequest(); + // ROCK-9044: same three checks btnRequestModalViewModeConnect_Click enforces, so the button is never + // rendered for a connect the server would refuse. btnRequestModalViewModeConnect.Visible = viewModel.ConnectionState != ConnectionState.Inactive && viewModel.ConnectionState != ConnectionState.Connected && connectionRequest.ConnectionOpportunity.ShowConnectButton && + IsRequestInSelectedOpportunity() && CanUserEditConnectionRequest() && CanUserConnect(); btnRequestModalViewModeEdit.Visible = CanUserEditConnectionRequest(); @@ -1525,6 +1546,16 @@ protected void lbRequestModalAddEditModeSave_Click( object sender, EventArgs e ) var isAddMode = IsRequestModalAddMode(); + // ROCK-9044: the request id is client-supplied and the edit check runs against the selected opportunity, + // and the save below re-points the request at that opportunity. Refuse an edit of a request that is not + // in the selected opportunity, otherwise Edit on one opportunity could rewrite (and connect) a request + // in another. Same rule the two connect paths apply. + if ( !isAddMode && !IsRequestInSelectedOpportunity() ) + { + ShowRequestModalNotification( EditNotAuthorizedMessage, NotificationBoxType.Danger ); + return; + } + var rockContext = new RockContext(); var connectionRequestService = new ConnectionRequestService( rockContext ); var connectionRequest = isAddMode ? @@ -1541,10 +1572,23 @@ protected void lbRequestModalAddEditModeSave_Click( object sender, EventArgs e ) var state = rblRequestModalAddEditModeState.SelectedValueAsEnumOrNull(); // ROCK-8640: prevent unauthorized users from transitioning a request into Connected state. - // If the selected state is Connected but the user isn't authorized, preserve the existing state. - if ( state == ConnectionState.Connected && !CanUserConnect() ) - { - state = connectionRequest.ConnectionState; + // ROCK-9044: evaluate the gate against the opportunity and status the request is being saved with + // (the selected opportunity, assigned above), not the request's pre-save opportunity. Refuse the save + // and say so, like the other two connect paths, rather than silently saving with the old state. + // Only the in-memory connectionRequest has been touched so far, so returning here persists nothing. + // A request that is already Connected is not being connected by this save, so it is not gated; the + // check above keeps it in the selected opportunity, so the save cannot move it into another one. + if ( oldConnectionState != ConnectionState.Connected + && state == ConnectionState.Connected + && !SeccConnectGateHelper.CanConnect( + rblRequestModalAddEditModeStatus.SelectedValueAsInt(), + connectionRequest.ConnectionState, + GetConnectionOpportunity(), + CurrentPerson, + GetAttributeValue( AttributeKey.SafetySecurityRole ).AsGuidOrNull() ) ) + { + ShowRequestModalNotification( ConnectNotAuthorizedMessage, NotificationBoxType.Danger ); + return; } // If a value is selected in the radio button list, use it, otherwise use "Active". @@ -2519,6 +2563,9 @@ private void BindRequestsGrid( bool isExporting = false, bool isMergeDocumentExp protected void gRequests_RowSelected( object sender, RowEventArgs e ) { ConnectionRequestId = e.RowKeyId; + + // ROCK-9044: a refusal shown for the previously opened request must not carry over to this one. + HideRequestModalNotification(); ViewAllActivities = false; IsRequestModalAddEditMode = false; RequestModalViewModeSubMode = RequestModalViewModeSubMode_View; @@ -2887,9 +2934,27 @@ private bool DoShowTransferButton() } private bool CanUserEditConnectionRequest() + { + return CanUserEditConnectionRequest( GetConnectionRequest() ); + } + + /// + /// SECC (ROCK-9044): Same edit check for an explicit request. Pass null to evaluate opportunity-level + /// edit rights with no request in context, which is what board-wide decisions (the card action menu's + /// gate list) need - otherwise whichever request happens to be in context on that postback would have + /// its per-request Edit result applied to every card. + /// + private bool CanUserEditConnectionRequest( ConnectionRequest connectionRequest ) { var connectionOpportunity = GetConnectionOpportunity(); - var connectionRequest = GetConnectionRequest(); + + // ROCK-9044: fail closed when the selected opportunity cannot be resolved (e.g. deactivated mid-session). + // Every branch below is scoped to it, and the connector-group query dereferences it. + if ( connectionOpportunity == null ) + { + return false; + } + var connectionType = GetConnectionType(); var userCanEditConnectionRequest = false; @@ -2898,7 +2963,7 @@ private bool CanUserEditConnectionRequest() { userCanEditConnectionRequest = connectionRequest.IsAuthorized( Authorization.EDIT, CurrentPerson ); } - else if ( connectionOpportunity != null ) + else { userCanEditConnectionRequest = connectionOpportunity.IsAuthorized( Authorization.EDIT, CurrentPerson ); } @@ -2943,19 +3008,53 @@ private bool CanUserEditConnectionRequest() return userCanEditConnectionRequest; } + /// + /// SECC (ROCK-9044): Message shown when a connect is refused by the edit check or the S&S connect gate. + /// + private const string ConnectNotAuthorizedMessage = "You are not authorized to connect this request."; + + /// + /// SECC (ROCK-9044): Title for the block-level notification shown when a connect is refused. + /// + private const string ConnectNotAuthorizedTitle = "Not Authorized"; + + /// + /// SECC (ROCK-9044): Message shown when an edit is refused because the request is not in the selected opportunity. + /// + private const string EditNotAuthorizedMessage = "You are not authorized to edit this request."; + + /// + /// SECC (ROCK-9044): Returns true if the request in context belongs to the opportunity currently selected + /// on the board. evaluates Edit against the selected opportunity + /// while the request id arrives from the client, so a connect has to be refused when the two do not match - + /// otherwise Edit rights on one opportunity could connect a request in another. The board only renders cards + /// for the selected opportunity, and the transfer flow moves the selection with the request, so every + /// legitimate connect path passes this check. Fails closed when either side cannot be resolved. + /// + private bool IsRequestInSelectedOpportunity() + { + var connectionRequest = GetConnectionRequest(); + var connectionOpportunity = GetConnectionOpportunity(); + + return connectionRequest != null + && connectionOpportunity != null + && connectionOpportunity.Id == connectionRequest.ConnectionOpportunityId; + } + /// /// SECC (ROCK-8640): Returns true if the current user may connect the request, based on the /// opportunity's SecurityToConnect flag and the configured Safety & Security role. /// Shared gate logic lives in (also used by ConnectionRequestDetail). /// Fails closed: returns false if the opportunity cannot be resolved. + /// ROCK-9044: evaluated against the selected opportunity. Every caller first requires + /// (or has no request in context, in modal add mode, where + /// the selected opportunity is the one the new request will be created in), so it is also the request's own. /// private bool CanUserConnect() { - var connectionRequest = GetConnectionRequest(); - return SeccConnectGateHelper.CanConnect( - connectionRequest, - GetGateConnectionOpportunity( connectionRequest ), + GetConnectionRequest(), + GetConnectionOpportunity(), CurrentPerson, GetAttributeValue( AttributeKey.SafetySecurityRole ).AsGuidOrNull() ); } @@ -2968,38 +3067,6 @@ private bool IsCampusRequiredSettingEnabled() return GetAttributeValue( AttributeKey.RequireCampus ).AsBooleanOrNull() ?? true; } - /// - /// SECC (ROCK-8640): Returns the opportunity whose connect rules apply to the given request. - /// The request identifier arrives from the client, so it can belong to an opportunity other than the - /// one currently selected on the board. The gate has to read SecurityToConnect and ConnectableStatuses - /// from the request's own opportunity - which is what ConnectionRequestDetail does - otherwise a user - /// on an opportunity that does not require security could connect a request in one that does. - /// Falls back to the selected opportunity when there is no request in context (modal add mode), which - /// is the opportunity the new request will be created in. - /// - /// The connection request, or null in modal add mode. - private ConnectionOpportunity GetGateConnectionOpportunity( ConnectionRequest connectionRequest ) - { - var selectedConnectionOpportunity = GetConnectionOpportunity(); - - if ( connectionRequest == null ) - { - return selectedConnectionOpportunity; - } - - // Reuse the already loaded instance when it is the right one, which is the normal case. - if ( selectedConnectionOpportunity != null - && selectedConnectionOpportunity.Id == connectionRequest.ConnectionOpportunityId ) - { - return selectedConnectionOpportunity; - } - - return new ConnectionOpportunityService( new RockContext() ) - .Queryable() - .AsNoTracking() - .FirstOrDefault( co => co.Id == connectionRequest.ConnectionOpportunityId ); - } - /// /// SECC (ROCK-8640): Runs the shared gate against every status on the selected opportunity, so the /// board card action menu can hide its Connect item using exactly the same rule that hides the @@ -3010,21 +3077,6 @@ private List GetUserConnectableStatusIds() { var statusIds = new List(); - /* - The modal's Connect button also requires edit rights, so apply the same check here. - - Note that no request is in context at bind time, so when the connection type has - EnableRequestSecurity turned on this evaluates opportunity-level Edit rather than the - per-request Edit the modal evaluates, and the card menu can show Connect for a request - the modal would hide. The card menu's postback is still evaluated per-request in - ProcessJavaScriptCommand, so this is a presentation difference only. Matching the modal - exactly here would require evaluating the gate per request instead of per status. - */ - if ( !CanUserEditConnectionRequest() ) - { - return statusIds; - } - var connectionOpportunity = GetConnectionOpportunity(); if ( connectionOpportunity == null @@ -3034,25 +3086,29 @@ exactly here would require evaluating the gate per request instead of per status return statusIds; } - var safetySecurityRoleGuid = GetAttributeValue( AttributeKey.SafetySecurityRole ).AsGuidOrNull(); + /* + The modal's Connect button also requires edit rights, so apply the same check here. - // Ordered so the same board state always produces the same list. The client stores these in its - // options object and re-renders the board when that object changes. - foreach ( var connectionStatus in connectionOpportunity.ConnectionType.ConnectionStatuses.OrderBy( cs => cs.Id ) ) - { - /* - Each status is evaluated as Active. State only affects the gate when it is Connected, - and the client applies this list only to cards whose core CanConnect is already true -- - which is false for both Connected and Inactive requests -- so the state passed here - cannot change the outcome. - */ - if ( SeccConnectGateHelper.CanConnect( connectionStatus.Id, ConnectionState.Active, connectionOpportunity, CurrentPerson, safetySecurityRoleGuid ) ) - { - statusIds.Add( connectionStatus.Id ); - } + ROCK-9044: evaluated with no request in context (opportunity-level Edit), because this list + applies to every card on the board. A request often IS in context here (deep link, card click, + the rebind after a modal save), and letting its per-request Edit result through would hide or + show Connect on every card. The card menu's postback is still evaluated per-request in + ProcessJavaScriptCommand. Keeping this request-independent also keeps the client's options hash + stable, so postbacks that do not need a board re-render keep hitting its early return. + */ + if ( !CanUserEditConnectionRequest( null ) ) + { + return statusIds; } - return statusIds; + // Ordered so the same board state always produces the same list. The client stores these in its + // options object and re-renders the board when that object changes. + // ROCK-9044: one gate evaluation for all statuses instead of one per status. + return SeccConnectGateHelper.GetConnectableStatusIds( + connectionOpportunity.ConnectionType.ConnectionStatuses.Select( cs => cs.Id ).OrderBy( id => id ), + connectionOpportunity, + CurrentPerson, + GetAttributeValue( AttributeKey.SafetySecurityRole ).AsGuidOrNull() ); } /// @@ -3431,6 +3487,10 @@ protected void btnRequestModalViewModeTransferModeSave_Click( object sender, Eve rockContext.SaveChanges(); + // ROCK-9044: the edit check above cached the pre-transfer request. Drop it so the modal + // shown below evaluates the connect gate against the opportunity the request is now in. + _connectionRequest = null; + if ( ConnectionOpportunityId != connectionRequest.ConnectionOpportunityId ) { // Connection opportunity changed @@ -3667,8 +3727,11 @@ protected void btnRequestViewModeViewHistory_Click( object sender, EventArgs e ) protected void btnRequestModalViewModeConnect_Click( object sender, EventArgs e ) { // ROCK-8640: also enforce the S&S connect gate server-side. - if ( !CanUserEditConnectionRequest() || !CanUserConnect() ) + // ROCK-9044: require the request to be in the selected opportunity (the one the edit check runs + // against), and tell the user when the connect is refused instead of silently doing nothing. + if ( !IsRequestInSelectedOpportunity() || !CanUserEditConnectionRequest() || !CanUserConnect() ) { + ShowRequestModalNotification( ConnectNotAuthorizedMessage, NotificationBoxType.Danger ); return; } @@ -4731,9 +4794,10 @@ private void HideRequestModalNotification() /// Shows the error. /// /// The text. - private void ShowError( string text ) + /// The title. ROCK-9044: defaults to the original "Oops". + private void ShowError( string text, string title = "Oops" ) { - nbNotificationBox.Title = "Oops"; + nbNotificationBox.Title = title; nbNotificationBox.NotificationBoxType = NotificationBoxType.Danger; nbNotificationBox.Text = text; nbNotificationBox.Visible = true; diff --git a/RockWeb/Scripts/Rock/Controls/ConnectionRequestBoard/connectionRequestBoard.js b/RockWeb/Scripts/Rock/Controls/ConnectionRequestBoard/connectionRequestBoard.js index 90fe17f9f25..470580ae086 100644 --- a/RockWeb/Scripts/Rock/Controls/ConnectionRequestBoard/connectionRequestBoard.js +++ b/RockWeb/Scripts/Rock/Controls/ConnectionRequestBoard/connectionRequestBoard.js @@ -271,12 +271,14 @@ // Region: DOM Manipulation // const fetchAndRefreshCard = function (options) { + // ROCK-9044: capture before the early return, matching initialize, so a future emission without a + // request id still refreshes the gate list. + captureConnectGate(options); + if (!options || !options.connectionRequestId) { return } - captureConnectGate(options); - fetchRequestViewModel(options, function (requestViewModel) { refreshCard(options.connectionRequestId, requestViewModel); });