Custom issuer #77
Custom issuer #77
Conversation
There was a problem hiding this comment.
I'll summon @joshcanhelp for a second pair of eyes as this is not my language.
| @@ -10,6 +10,7 @@ | |||
| let(:client_id) { 'CLIENT_ID' } | |||
| let(:client_secret) { 'CLIENT_SECRET' } | |||
| let(:domain) { 'samples.auth0.com' } | |||
| let(:issuer) { 'samples.auth0.com' } | |||
There was a problem hiding this comment.
if the issuer is the same as the domain, how can the test check that it changed?
| describe 'JWT verifier custom issuer' do | ||
| context 'same as domain' do | ||
| let(:jwt_validator) do | ||
| make_jwt_validator(opt_issuer: issuer) |
There was a problem hiding this comment.
I rather you pass the issuer value here instead of declaring a global variable that is only going to be used for this single test context.
| ) | ||
| ) | ||
| def make_jwt_validator(opt_domain: domain, opt_issuer: nil) | ||
| opts = if opt_issuer.nil? |
There was a problem hiding this comment.
You already have as part of the initialize method of the verifier a check for the presence of the issuer value.
if options.respond_to?(:issuer)
Can't you instantiate it directly using named arguments? Like you did here https://github.com/auth0/omniauth-auth0/pull/77/files#diff-708c8cee2bf0dc225b55a0c36afaa97dR260. Why do you need to use Options and OptionsWithIssuer?
There was a problem hiding this comment.
Ah, didn't realize Options was a Struct and not an OpenStruct before and was confused why I couldn't add values, which is why I made a different struct. Switching the implementation to use an OpenStruct where I add the issuer if it's there. Can't pass in the issuer if it's nil because the OpenStruct/Struct will return true for the respond_to? method, even though the value it returns is actually nil.
|
@ryan-rosenfeld - Thanks for the PR here! I agree with @lbalmaceda's assessment here. If you can walk through the changes here (and run |
|
Thanks so much for the attention @joshcanhelp and @lbalmaceda, really appreciate it! I will address your comments asap and let you know once my changes are up 👍 |
|
Okay, the PR should be updated based on your feedback. Ran both files through rubocop and they checked out. Let me know if there are any other changes you'd like me to make! |
There was a problem hiding this comment.
@ryan-rosenfeld - Looks great!
And just to make sure ... this was tested in your internal auth service?
|
Awesome, thanks @joshcanhelp! Yes I did, I tested by monkeypatching the current version of the gem with the exact changes I made here, and it worked as desired for our usage scenario using the custom issuer. |
|
@ryan-rosenfeld Thank YOU! I'll get this released first thing next week. |
|
hey @joshcanhelp, any luck in cutting a new version to rubygems? any eta you can provide us? we're looking to get this monkeypatch out of production and would love if we could get the native gem in thanks! |
|
So sorry for the delay, this totally slipped through the cracks. Pushing this out right now. |
|
thanks for the update to master. any idea when rubygems cache will pop? seems like it still only has the most recent version |
|
I see it live on RubyGems: |
|
yeah was just able to upgrade. thanks!
…On Thu, Apr 18, 2019 at 11:35 AM Josh Cunningham ***@***.***> wrote:
I see it live on RubyGems:
https://rubygems.org/gems/omniauth-auth0
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#77 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADLK6CAQHJP3HJPEMHB75DPRC5NRANCNFSM4HBF74PA>
.
|
Changes
References
Testing
Unit tests have been added to reflect the added change, and we tested locally with our setup.
Checklist