ClaimValueToggle fails on multiple claims

[gkfischer]

The current implementation of ClaimValueToggle only works properly if only one claim with a type exists in the identity claims. Although there's a loop over all claimValues, only the first claim get's evaluated, because the result of the evaluation is returned in line 57:

public ValueTask<bool> IsActiveAsync(ToggleExecutionContext context, CancellationToken cancellationToken = default)
        {
            string claimType = context.Data[ClaimType]?.ToString();
            string allowedValues = context.Data[ClaimValues]?.ToString();

            if (claimType != null
                &&
                allowedValues != null)
            {
                var user = _httpContextAccessor.HttpContext.User;
                if (user != null && user.Identity.IsAuthenticated)
                {
                    var tokenizer = new StringTokenizer(allowedValues, EsquioConstants.DEFAULT_SPLIT_SEPARATOR);

                    var claimValues = user.Claims
                        .Where(claim => claim.Type == claimType)
                        .Select(claim => claim.Value);

                    foreach (var item in claimValues)
                    {
                        if (item != null)
                        {
                            var active = tokenizer.Contains(item, StringSegmentComparer.OrdinalIgnoreCase);
                            return new ValueTask<bool>(active);
                        }
                    }
                }
            }

            return new ValueTask<bool>(false);
        }

I don't know if this behaviour is intentional, but if not, the line should probably read:

if (active) return new ValueTask<bool>(true);

Or the whole block could be turned into

if ((from item in claimValues where item != null select tokenizer.Contains(item, StringSegmentComparer.OrdinalIgnoreCase)).Any(active => active))
   {
      return new ValueTask<bool>(true);
   }

Then only successfull evaluations would return.

[risogolo]

The current implementation of ClaimValueToggle only works properly if only one claim with a type exists in the identity claims. Although there's a loop over all claimValues, only the first claim get's evaluated, because the result of the evaluation is returned in line 57:

public ValueTask<bool> IsActiveAsync(ToggleExecutionContext context, CancellationToken cancellationToken = default)
        {
            string claimType = context.Data[ClaimType]?.ToString();
            string allowedValues = context.Data[ClaimValues]?.ToString();

            if (claimType != null
                &&
                allowedValues != null)
            {
                var user = _httpContextAccessor.HttpContext.User;
                if (user != null && user.Identity.IsAuthenticated)
                {
                    var tokenizer = new StringTokenizer(allowedValues, EsquioConstants.DEFAULT_SPLIT_SEPARATOR);

                    var claimValues = user.Claims
                        .Where(claim => claim.Type == claimType)
                        .Select(claim => claim.Value);

                    foreach (var item in claimValues)
                    {
                        if (item != null)
                        {
                            var active = tokenizer.Contains(item, StringSegmentComparer.OrdinalIgnoreCase);
                            return new ValueTask<bool>(active);
                        }
                    }
                }
            }

            return new ValueTask<bool>(false);
        }

I don't know if this behaviour is intentional, but if not, the line should probably read:

if (active) return new ValueTask<bool>(true);

Or the whole block could be turned into

if ((from item in claimValues where item != null select tokenizer.Contains(item, StringSegmentComparer.OrdinalIgnoreCase)).Any(active => active))
   {
      return new ValueTask<bool>(true);
   }

Then only successfull evaluations would return.

The current implementation of ClaimValueToggle only works properly if only one claim with a type exists in the identity claims. Although there's a loop over all claimValues, only the first claim get's evaluated, because the result of the evaluation is returned in line 57:

public ValueTask<bool> IsActiveAsync(ToggleExecutionContext context, CancellationToken cancellationToken = default)
        {
            string claimType = context.Data[ClaimType]?.ToString();
            string allowedValues = context.Data[ClaimValues]?.ToString();

            if (claimType != null
                &&
                allowedValues != null)
            {
                var user = _httpContextAccessor.HttpContext.User;
                if (user != null && user.Identity.IsAuthenticated)
                {
                    var tokenizer = new StringTokenizer(allowedValues, EsquioConstants.DEFAULT_SPLIT_SEPARATOR);

                    var claimValues = user.Claims
                        .Where(claim => claim.Type == claimType)
                        .Select(claim => claim.Value);

                    foreach (var item in claimValues)
                    {
                        if (item != null)
                        {
                            var active = tokenizer.Contains(item, StringSegmentComparer.OrdinalIgnoreCase);
                            return new ValueTask<bool>(active);
                        }
                    }
                }
            }

            return new ValueTask<bool>(false);
        }

I don't know if this behaviour is intentional, but if not, the line should probably read:

if (active) return new ValueTask<bool>(true);

Or the whole block could be turned into

if ((from item in claimValues where item != null select tokenizer.Contains(item, StringSegmentComparer.OrdinalIgnoreCase)).Any(active => active))
   {
      return new ValueTask<bool>(true);
   }

Then only successfull evaluations would return.

I guess this has been fixed for v2 https://github.com/Xabaril/Esquio/issues/111

[unaizorrilla]

Hi @gkfischer

On wich version ?

[risogolo]

Hi @gkfischer

On wich version ?

v 3.1.0 I reported the same for v2 a couple months ago

[unaizorrilla]

Well,

I review the issue, and well at this moment the behavior is intentional and covered on this tests

https://github.com/Xabaril/Esquio/blob/a056c8abfdcc2dc995a164d1e5755a54c07088e0/tests/UnitTests/Esquio.AspNetCore/Toggles/ClaimValueToggleTests.cs#L94

but i think this behavior is incorrect and i try to fix on 3.1.X, let me work on this!

[unaizorrilla]

Hi all

This is a regregation bug from #111, a new package Esquio.AspNetCore 3.1.1 will be on NuGet!

I'm so sorry for that!

Please close the issue if new package solve this!

[gkfischer]

Thanks for the fast reponse and solution