-
Notifications
You must be signed in to change notification settings - Fork 55
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
Support Client Assertion authentication on authentication client #260
Conversation
af0e295
to
cefca23
Compare
break | ||
} | ||
|
||
if required && (body.Get("client_secret") == "" && body.Get("client_assertion") == "") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the &&
in (body.Get("client_secret") == "" && body.Get("client_assertion") == "")
be a ||
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to error here if both values are empty as the they can be used independently of each other, so if client_secret
is empty but client_assertion
is not then we can continue
JwtID(uuid.New().String()). | ||
Issuer(clientID). | ||
Audience([]string{domain}). | ||
Expiration(time.Now().Add(2 * time.Minute)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of curiosity, where does this 2 minute exp come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was aligning with the value set by the node-auth0 v4 beta here, I think there's not really a specific reason for picking 2 minutes, the only requirement is that exp
must be a value no more than 5 minutes after the iat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense for this exp value to be configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we currently support configuring this on any other SDK and I haven't seen any request to do so yet, so I'd maybe defer to adding if we get a request
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #260 +/- ##
==========================================
- Coverage 94.89% 94.66% -0.24%
==========================================
Files 46 46
Lines 8441 8523 +82
==========================================
+ Hits 8010 8068 +58
- Misses 337 353 +16
- Partials 94 102 +8
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just very small suggestions, overall things look good 👍🏻
authentication/authentication.go
Outdated
@@ -155,6 +158,7 @@ func New(ctx context.Context, domain string, options ...Option) (*Authentication | |||
auth0ClientInfo: client.DefaultAuth0ClientInfo, | |||
idTokenSigningAlg: "RS256", | |||
retryStrategy: client.DefaultRetryOptions, | |||
domain: domain + "/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use url.JoinPath
here instead? If the domain already contains "/"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good point, on reflection it's probably a bit wasteful to have a separate domain here (just for the added /
) when we already have url
@@ -70,7 +79,11 @@ func (p *Passwordless) SendSMS(ctx context.Context, params passwordless.SendSMSR | |||
// | |||
// See: https://auth0.com/docs/api/authentication?http#authenticate-user | |||
func (p *Passwordless) LoginWithSMS(ctx context.Context, params passwordless.LoginWithSMSRequest, validationOptions oauth.IDTokenValidationOptions, opts ...RequestOption) (t *oauth.TokenSet, err error) { | |||
p.addClientAuthentication(¶ms.ClientAuthentication) | |||
err = p.addClientAuthentication(¶ms.ClientAuthentication) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Sergiu Ghitea <[email protected]>
6b67632
to
84b9185
Compare
🔧 Changes
Introduces support for using Client Assertion to the authentication client, this is for use when the Authentication Method is set to
Private Ket JWT
. This is supported using theWithClientAssertion
option that can be passed instead of the existingWithClientSecret
📚 References
🔬 Testing
📝 Checklist