port updateExtensionAuthenticationRequest #61
Conversation
cranberyxl
force-pushed
the
feature/updateExtensionAuthenticationRequest
branch
3 times, most recently
from
17f2d5d to
316cbaa
Compare
cranberyxl
force-pushed
the
feature/updateExtensionAuthenticationRequest
branch
from
316cbaa to
fbf15b7
Compare
wparad
left a comment
wparad
left a comment
There was a problem hiding this comment.
I'm marking this as request changes, but realistically, these changes are something I expect us to do before merging on top of this PR. I'm not expecting you to go through and make these updates.
| * @param {Object} [connectionProperties] Connection specific properties to pass to the identity provider. Can be used to override default scopes for example. | ||
| * @return {Promise<AuthenticateResponse>} The authentication response. | ||
| */ | ||
| async updateExtensionAuthenticationRequest({ state, connectionId, tenantLookupIdentifier, connectionProperties, hint }) { |
There was a problem hiding this comment.
Will likely be a new method in:
loginApi.updateAuthenticationRequest(authenticationRequestId, { hint, tenantLookupIdentifier, connection });| connectionProperties | ||
| }); | ||
|
|
||
| return requestOptions.data; |
There was a problem hiding this comment.
I'm thinking a dedicated response object that only includes the authenticationUrl to be used by the caller, rather than the full object.
|
|
||
| return requestOptions.data; | ||
| } catch (error) { | ||
| if (error.status && error.status >= 400 && error.status < 500) { |
There was a problem hiding this comment.
We'll want this pattern to match the other APIs error response pattern.
| } | ||
| } | ||
|
|
||
| async calculateAntiAbuseHash(props) { |
There was a problem hiding this comment.
Make sure to take in the explicit properties by name in the expected order, validate there aren't any additional properties, before creating the valueString.
| let fineTuner = 0; | ||
| let hash = null; | ||
| while (++fineTuner) { | ||
| hash = base64url.encode(await crypto.subtle.digest('SHA-256', new TextEncoder().encode(`${timestamp};${fineTuner};${valueString}`))); |
There was a problem hiding this comment.
Will probably want the crypto.createHash version which avoids the need for the text encoder, I actually don't know if the async version is better, since this is CPU intensive, using the async method in subtle is less consistent with the other createHash crypto usages we have.
| try { | ||
| const resolvedTenantLookupIdentifier = hint || tenantLookupIdentifier; | ||
| const antiAbuseHash = await jwtManager.calculateAntiAbuseHash({ connectionId, tenantLookupIdentifier: resolvedTenantLookupIdentifier, authenticationRequestId }); | ||
| const requestOptions = await this.httpClient.patch(`/api/authentication/${authenticationRequestId}`, { |
There was a problem hiding this comment.
Reminder: Add the origin header.
2 participants
From: https://github.com/Authress/authress-login.js/blob/release/2.6/src/index.js#L441
Allows this redirection to happen on the server and not be dependent on browser javascript.