2
\$\begingroup\$

I'm writing extension methods to shorten the code required to get custom Claims from an IPrincipal. I have two general forms for the bodies of these methods.

Form 1

public static long? GetFirmId(this IPrincipal principal)
{
    return ((ClaimsPrincipal) principal)?.FindFirst(nameof(User.FirmId)) == null
        ? default(long?)
        : long.TryParse(((ClaimsPrincipal) principal).FindFirst(nameof(User.FirmId)).Value, out var firmId)
            ? firmId
            : default(long?);
}

Form 2

public static long? GetFirmId(this IPrincipal principal)
{
    if (principal == null)
    {
        return default(long?);
    }

    var claim = ((ClaimsPrincipal) principal).FindFirst(nameof(User.FirmId));

    if (claim == null)
    {
        return default(long?);
    }

    if (long.TryParse(claim.Value, out var firmId))
    {
        return firmId;
    }

    return default(long?);
}

Please note that safely casting IPrincipal to ClaimsPrincipal is not necessary as IPrincipal in an ASP.NET Identity application is guaranteed to be a ClaimsPrincipal. I personally like form 1 better due to the brevity. The only thing I'm not super fond of is having to repeat ((ClaimsPrincipal) principal).FindFirst(nameof(User.FirmId)). However, I would say that form 2 is easier to understand for anyone not familiar with modern C# features (or ternary operators, which I have found to be surprisingly unheard of).

ReSharper suggests a middle ground (probably because it isn't sophisticated enough to transcribe form 2 to form 1):

public static long? GetFirmId(this IPrincipal principal)
{
    var claim = ((ClaimsPrincipal) principal)?.FindFirst(nameof(User.FirmId));

    if (claim == null)
    {
        return default(long?);
    }

    return long.TryParse(claim.Value, out var firmId) ? firmId : default(long?);
}

I'm looking for recommendations as to which form is "best," or an alternative suggestion. The project this is from has about ten of these methods for various custom claims of bool, bool?, char, char?, int, int?, long, long? and string. We are using custom claims to prevent needing a database trip (and additional code to make said database trip) to fetch commonly-used user-specific data.

\$\endgroup\$
  • \$\begingroup\$ The (claim == null) block might be able to be removed by adding in ? in as long.TryParse(claim?.Value, out var firmId); I'm not familiar with ClaimsPrincipal \$\endgroup\$ – alexyorke Dec 5 '19 at 2:20
  • 1
    \$\begingroup\$ If you have multiple of these, why then not move ((ClaimsPrincipal) principal)?.FindFirst(claimName) to a method of its own? \$\endgroup\$ – BCdotWEB Dec 5 '19 at 12:13
2
\$\begingroup\$

Sometimes breaking things up into smaller manageable chunks makes them easier to swallow

Break the responsibilities up into more focused concerns.

public static class PrincipalExtension {
    /// <summary>
    /// Retrieves the Firm Id claim if it exists
    /// </summary>
    public static long? GetFirmId(this IPrincipal principal) {
        return long.TryParse(principal.FindFirstOrEmpty(nameof(User.FirmId)), out var firmId)
            ? firmId
            : default(long?);
    }
    /// <summary>
    /// Retrieves the first claim that is matched by the 
    /// specified type if it exists, String.Empty otherwise.
    /// </summary>
    public static string FindFirstOrEmpty(this IPrincipal principal, string type) {
        return principal is ClaimsPrincipal p
            ? p.FindFirst(type)?.Value ?? string.Empty
            : string.Empty;
    }
}

FindFirstOrEmpty handles the finding of the claim value if it exists.

This allows the main function to perform its designed role.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.