feat: syncs time with Authorization Server date header#16
feat: syncs time with Authorization Server date header#16jaredperreault-okta wants to merge 5 commits into0.5from
Conversation
| nonce | ||
| }; | ||
|
|
||
| console.log('claims', claims); |
There was a problem hiding this comment.
Yes, I'll remove
| if (!( | ||
| OAuth2Client.isDPoPProofClockSkewError(json) && // proper error is returned from AS | ||
| request.canRetry() && // request hasn't been retried too many times previously | ||
| Math.abs(Date.now() - TimeCoordinator.clockSkew) >= 150 // the TimeCoordinator updated with a meaningful time difference (~2.5 mintues) |
There was a problem hiding this comment.
You might want to put this clock skew value into a const stored elsewhere.
| if (!( | ||
| OAuth2Client.isDPoPProofClockSkewError(json) && // proper error is returned from AS | ||
| request.canRetry() && // request hasn't been retried too many times previously |
There was a problem hiding this comment.
It looks like you're trying to do an early return to return the JSON response to the caller, but the !(expression && expression) logic makes my brain go fuzzy reading it. It's not clear to me what conditions need to be met to consider the need to retry the response.
It might be clearer to break the expression up a bit, or to perform the JWT sign and retry process at the same time as checking for a time-skew error.
| // this will align the client's clock with the Authorization Server's | ||
| await this.signTokenRequestWithDPoP(request); // re-sign request (with new `TimeCoordinator.now()`) | ||
| const retryReponse = await this.retry(request); // trigger retry | ||
| json = await retryReponse.json(); |
There was a problem hiding this comment.
You're not returning the json returned from the retry here.
There was a problem hiding this comment.
Yea this block is a little confusing but the gist is:
if (isOAuth2ErrorResponse(json) {
// if dpop proof clock skew error
retry() // this is return a response other than `OAuth2ErrorResponse`
//else
return json;
}| export function isDPoPProofClockSkewError (error: OAuth2ErrorResponse) { | ||
| return error.error === 'invalid_dpop_proof' && ( | ||
| error.error_description === 'The DPoP proof JWT is issued in the future.' || | ||
| error.error_description === 'The DPoP proof JWT is issued more than five minutes in the past.' | ||
| ); |
There was a problem hiding this comment.
I believe so, I didn't them mentioned in the spec
| authentication?: ClientAuthentication; | ||
| allowHTTP?: boolean; | ||
| } | ||
| syncClockWithAuthorizationServer?: boolean; |
There was a problem hiding this comment.
Do you anticipate any occasions where a developer wouldn't want to adjust clock skew? In the other SDKs that implement clock skew, the default implementation just does it automatically since it's almost always what the developer would want. If clock skew is not wanted, then it's simpler to assign a different TimeCoordinator instance which doesn't perform clock correction, this way the clock behavior is consistent across the whole SDK.
There was a problem hiding this comment.
Oh, and implementing this here, at the OAuth2 configuration level, doesn't help with clcok skew corrections in other parts of the SDK where they may be necessary.
2 participants
No description provided.