fix(AspNet.Security.OAuth.Weixin):WeixinAuthenticationHandler BuildChallengeUrl method has a error: state too long

[wuweilaiya]

Describe the bug

WeixinAuthenticationHandler BuildChallengeUrl method has a error: state too long,But I see you guys have solved this problem I think !IsWeixinAuthorizationEndpointInUse() should change to IsWeixinAuthorizationEndpointInUse()，why if(!IsWeixinAuthorizationEndpointInUse())

Steps To reproduce

Expected behaviour

Actual behaviour

System information

• OS: [e.g. Windows 10]
• Library Version [e.g. 6.0.1]
• .NET version (6)

Additional context

[LeaFrock]

It's not a bug. These codes are imported by PR #262. Due to some historical reasons, wechat provides several auth-ways for different web apps. You can read these two docs below,

1、https://developers.weixin.qq.com/doc/oplatform/Website_App/WeChat_Login/Wechat_Login.html (WeixinAuthenticationDefaults.AuthorizationEndpoint, which an user should scan a QR Code with his/her device to move on the auth) 2、https://developers.weixin.qq.com/doc/offiaccount/OA_Web_Apps/Wechat_webpage_authorization.html (Another way for wechat official accounts)

As you can see, the second one, which the original PR aims to, says it limits the size of state in 128 bytes, when the default one does not mention it at all. At the beginning, the codes were all right because the QR API did not (says it) have such a limit. But now the default one has an implicit limit (I'm not sure whether the max size is 128 bytes) of state because I met the same problem before as yours.

You have two solutions/workarounds,

1、Write your own implementation of StateDataFormat，which stores a session(GUID) with the actual content of your properties to a db/redis, and replace your current state with the session. So only the session is protected and transferred as state, and I'm sure it won't trigger the limit. When the state is returned by the callback, you can unprotect it and get the previous content by it.

2、Commit (or wait for) a PR to adjust it. But it's actually a break change since you will change the behavior of the default wechat authz-endpoint.

Since these codes are working well for long versions/years, I'm not sure how big the impact is. We need to be cautious for changes. I'll do a test first when I have free time.

What do you think/suggest on this issue? @kinosang @saber-wang

[LeaFrock]

Well, I've done a test and find that the QR API also limits the length of redirect_uri.

The following is my query of sign-in URL:

And the page tells redirect_uri param error,

So if your state is too long, the way you want still does not work @15168440402 .

p.s. I find the codes in HandleRemoteAuthenticateAsync can do a small optimization. I'll make a PR to improve it.

[wuweilaiya]

@LeaFrock Thank you for your reply,I solved the problem by adopting your plan one.

[LeaFrock]

It's nothing. If you can, please share your implementation of StateDataFormat, which may help others trapped in this issue. Or you just close this issue as it's been solved by you 😄.

[wuweilaiya]

1.Implement ISecureDataFormat 2.WeixinAuthenticationOptions.StateDataFormat = PropertiesDataFormatCache

[martincostello]

Could you please post the code rather than screenshots of the code? People can then just copy and paste it, rather than have to type it all out by hand from reading the image.

[LeaFrock]

Well, I reorganize the codes based on @15168440402 .

    public abstract class CachedStateDataFormat : ISecureDataFormat<AuthenticationProperties>
    {
        private const string CacheKeyPrefix = "AnyWordsYouLike:";

        public CachedStateDataFormat(IDataProtectionProvider provider)
        {
            Protector = provider.CreateProtector(typeof(CachedStateDataFormat).FullName);
        }

        protected IDataProtector Protector { get; }

        protected TimeSpan ExpiredTimeSpan { get; } = TimeSpan.FromMinutes(10);

        protected static string GetCacheKey(string guid) => CacheKeyPrefix + guid;

        public string Protect(AuthenticationProperties data) => Protect(data, default);

        public abstract string Protect(AuthenticationProperties data, string purpose);

        public AuthenticationProperties Unprotect(string protectedText) => Unprotect(protectedText, default);

        public abstract AuthenticationProperties Unprotect(string protectedText, string purpose);
    }

    internal class RedisCachedStateDataFormat : CachedStateDataFormat
    {
        private readonly DistributedCacheEntryOptions _cacheOpt;
        private readonly IDistributedCache _cache;

        public RedisCachedStateDataFormat(
            IDistributedCache cache,
            IDataProtectionProvider provider) : base(provider)
        {
            _cache = cache;
            _cacheOpt = new() { AbsoluteExpirationRelativeToNow = ExpiredTimeSpan };
        }

        public override string Protect(AuthenticationProperties data, string purpose)
        {
            var protector = Protector;
            if (!string.IsNullOrEmpty(purpose))
            {
                protector = protector.CreateProtector(purpose);
            }

            string uid = Guid.NewGuid().ToString("N");
            string cacheKey = GetCacheKey(uid);
            byte[] userData = PropertiesSerializer.Default.Serialize(data);
            _cache.Set(cacheKey, userData, _cacheOpt);

            return protector.Protect(uid);
        }

        public override AuthenticationProperties Unprotect(string protectedText, string purpose)
        {
            if (protectedText == null)
            {
                return default;
            }
            var protector = Protector;
            if (!string.IsNullOrEmpty(purpose))
            {
                protector = protector.CreateProtector(purpose);
            }

            try
            {
                var uid = protector.Unprotect(protectedText);
                if (string.IsNullOrEmpty(uid))
                {
                    return default;
                }
                string cacheKey = GetCacheKey(uid);
                var userData = _cache.Get(cacheKey);
                if (userData is null)
                {
                    return default;
                }
                _cache.Remove(cacheKey);
                return PropertiesSerializer.Default.Deserialize(userData);
            }
            catch
            {
                // TODO something ...
                return default;
            }
        }
    }

    internal class MemoryCachedStateDataFormat : CachedStateDataFormat
    {
        private readonly IMemoryCache _cache;

        public MemoryCachedStateDataFormat(
            IMemoryCache cache,
            IDataProtectionProvider provider) : base(provider)
        {
            _cache = cache;
        }

        public override string Protect(AuthenticationProperties data, string purpose)
        {
            var protector = Protector;
            if (!string.IsNullOrEmpty(purpose))
            {
                protector = protector.CreateProtector(purpose);
            }

            string uid = Guid.NewGuid().ToString("N");
            string cacheKey = GetCacheKey(uid);
            _cache.Set(cacheKey, data, ExpiredTimeSpan);

            return protector.Protect(uid);
        }

        public override AuthenticationProperties Unprotect(string protectedText, string purpose)
        {
            if (protectedText == null)
            {
                return default;
            }
            var protector = Protector;
            if (!string.IsNullOrEmpty(purpose))
            {
                protector = protector.CreateProtector(purpose);
            }

            string uid;
            try
            {
                uid = protector.Unprotect(protectedText);
            }
            catch
            {
                // TODO something ...
                return default;
            }
            if (string.IsNullOrEmpty(uid))
            {
                return default;
            }
            string cacheKey = GetCacheKey(uid);
            if (!_cache.TryGetValue(cacheKey, out AuthenticationProperties data))
            {
                return default;
            }
            _cache.Remove(cacheKey);
            return data;
        }
    }