Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend RSAlgorithmFactory to support all ctors of RS256Algorithm #221

Merged
merged 8 commits into from
Oct 1, 2019
Merged

Extend RSAlgorithmFactory to support all ctors of RS256Algorithm #221

merged 8 commits into from
Oct 1, 2019

Conversation

flerka
Copy link
Contributor

@flerka flerka commented Sep 26, 2019

This is my draft implantation for #220

@abatishchev
Copy link
Member

Hi, thanks for the contribution!
Can you explain the idea behind heaving Func over instantiating an algorithm directly?

@flerka
Copy link
Contributor Author

flerka commented Sep 27, 2019

This was done to avoid confusing if inside switch statement. All Func can be rewritten with:

            switch (algorithm)
            {
                case JwtHashAlgorithm.RS256:
                    {
                        if (_publicKey != null)
                        {
                            return new RS256Algorithm(publicKey);
                        }

                        var certificate = _certFactory();
#if NETSTANDARD1_3
            return new RS256Algorithm((RSACryptoServiceProvider)certificate.GetRSAPublicKey(), certificate.GetRSAPrivateKey());
#else
                        return new RS256Algorithm((RSACryptoServiceProvider)certificate.PublicKey.Key, (RSA)certificate.PrivateKey);
#endif

                    }
                default:
                    throw new NotSupportedException($"For algorithm {Enum.GetName(typeof(JwtHashAlgorithm), algorithm)} please use the appropriate factory by implementing {nameof(IAlgorithmFactory)}");
            }

If this is better from your point of view, I can rollback previous changes and commit this. What should I do @abatishchev ?

@abatishchev
Copy link
Member

I was thinking more about this factory and realized we might not need it at all. Since it supports a single algorithm anyway. So it just mimics the algorithm's own constrictors. Does it add any value in top of it?
The initial ideas was to support multiple algorithms, help to instantiate one or another. But that has never happened.
So I'd remove it together. What do you think?

@abatishchev
Copy link
Member

I'd remove JwtHashAlgorithm as well. Wondering what's the percentage of the users of this library uses symmetric algorithms still. Hope that not much.

@abatishchev
Copy link
Member

abatishchev commented Sep 30, 2019

Actually I got your idea and I like it. It's better than mine in #224. Can you please rebase on the latest master?

@abatishchev abatishchev self-assigned this Sep 30, 2019
@abatishchev abatishchev added this to the 5.0 milestone Sep 30, 2019
src/JWT/Algorithms/RSAlgorithmFactory.cs Outdated Show resolved Hide resolved
src/JWT/Algorithms/RSAlgorithmFactory.cs Outdated Show resolved Hide resolved
@abatishchev abatishchev marked this pull request as ready for review September 30, 2019 17:48
@abatishchev abatishchev merged commit 2de2bd0 into jwt-dotnet:master Oct 1, 2019
@abatishchev
Copy link
Member

@flerka thanks for your contribution, much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants