Skip to content

Commit

Permalink
+ (Finance) Fixed a bug where Contribution Statement Generator could …
Browse files Browse the repository at this point in the history 
…throw a null reference error when no valid person passed via query string. (Fixes #5705)
  • Loading branch information
@unlearnd
unlearnd committed Jan 10, 2024
1 parent 5ae34af commit 11292c2
Showing 1 changed file with 11 additions and 3 deletions.
14 changes: 11 additions & 3 deletions RockWeb/Blocks/Finance/ContributionStatementGenerator.ascx.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
using System;
using System.ComponentModel;
using System.Linq;
using System.Net;
using System.Web.UI;

using Rock;
Expand Down Expand Up @@ -113,10 +114,17 @@ protected void Block_BlockUpdated( object sender, EventArgs e )

private void DisplayResults()
{
RockContext rockContext = new RockContext();

Person targetPerson = CurrentPerson;


if ( targetPerson == null )
{
Response.StatusCode = ( int ) HttpStatusCode.BadRequest;
Response.Write( "Invalid Person" );
Response.End();
}

RockContext rockContext = new RockContext();

var statementYear = PageParameter( PageParameterKey.StatementYear ).AsIntegerOrNull() ?? RockDateTime.Now.Year;
var personActionId = PageParameter( PageParameterKey.PersonActionIdentifier );
var personGuid = PageParameter( PageParameterKey.PersonGuid ).AsGuidOrNull();
Expand Down

3 comments on commit 11292c2

@MichaelAllen
Copy link
Contributor

Choose a reason for hiding this comment

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

@unlearnd I think that conditional may need to move down a bit. As is, this will show the "Bad Request" error to anyone who is not logged in (CurrentPerson == null) without considering a PersonGuid or Action Identifier in the URL. If it were moved to immediately after this section, then it would allow those URL parameters to be considered while still preventing the NRE from #5705:

Rock/RockWeb/Blocks/Finance/ContributionStatementGenerator.ascx.cs

Lines 137 to 159 in 06a3106

if ( personActionId.IsNotNullOrWhiteSpace() )
{
var person = new PersonService( rockContext ).GetByPersonActionIdentifier( personActionId, "contribution-statement" );
var isCurrentPersonsBusiness = targetPerson != null && targetPerson.GetBusinesses().Any( b => b.Guid == person.Guid );
if ( person != null && ( allowPersonQueryString || isCurrentPersonsBusiness ) )
{
targetPerson = person;
}
}
else if ( personGuid.HasValue )
{
// if "AllowPersonQueryString is False", only use the PersonGuid if it is a Guid of one of the current person's businesses
var isCurrentPersonsBusiness = targetPerson != null && targetPerson.GetBusinesses().Any( b => b.Guid == personGuid.Value );
if ( allowPersonQueryString || isCurrentPersonsBusiness )
{
var person = new PersonService( rockContext ).Get( personGuid.Value );
if ( person != null )
{
targetPerson = person;
}
}
}

@bmurphy-dev
Copy link
Contributor

Choose a reason for hiding this comment

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

@unlearnd can this be re-opended or fixed with @MichaelAllen 's suggestion?

@unlearnd
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right @MichaelAllen ! Really appreciate you catching that! I'll make the modifications in the 16.3 version. Here's what I am expecting the method to look like once amended. If you see anything amiss please don't hesitate to let me know otherwise these changes shouild be in 16.3 within the next day or two.

private void DisplayResults()
{
    Person targetPerson = CurrentPerson;

    RockContext rockContext = new RockContext();
                
    var statementYear = PageParameter( PageParameterKey.StatementYear ).AsIntegerOrNull() ?? RockDateTime.Now.Year;
    var statementEndMonth = PageParameter( PageParameterKey.StatementEndMonth ).AsIntegerOrNull() ?? 0;
    var personActionId = PageParameter( PageParameterKey.PersonActionIdentifier );
    var personGuid = PageParameter( PageParameterKey.PersonGuid ).AsGuidOrNull();
    var allowPersonQueryString = GetAttributeValue( AttributeKey.AllowPersonQueryString ).AsBoolean();

    if ( personActionId.IsNotNullOrWhiteSpace() )
    {
        var person = new PersonService( rockContext ).GetByPersonActionIdentifier( personActionId, "contribution-statement" );
        var isCurrentPersonsBusiness = targetPerson != null && targetPerson.GetBusinesses().Any( b => b.Guid == person.Guid );
        if ( person != null && ( allowPersonQueryString || isCurrentPersonsBusiness ) )
        {
            targetPerson = person;
        }
    }
    else if ( personGuid.HasValue )
    {
        // if "AllowPersonQueryString is False", only use the PersonGuid if it is a Guid of one of the current person's businesses
        var isCurrentPersonsBusiness = targetPerson != null && targetPerson.GetBusinesses().Any( b => b.Guid == personGuid.Value );

        if ( allowPersonQueryString || isCurrentPersonsBusiness )
        {
            var person = new PersonService( rockContext ).Get( personGuid.Value );
            if ( person != null )
            {
                targetPerson = person;
            }
        }
    }

    if ( targetPerson == null )
    {
        Response.StatusCode = ( int ) HttpStatusCode.BadRequest;
        Response.Write( "Invalid Person" );
        Response.End();
    }
    // Other code here...
}

Please sign in to comment.