Calling `performAuthorizationRequest` multiple times blocks NodeBasedHandler forever

[pixtron]

Expected Behavior

Describe expected behavior

If performAuthorizationRequest is called more than once without completing the flow of the first call, the authorization flow from the first call should still be able to finish.

Describe the problem

Actual Behavior

After two calls to performAuthorizationRequest Server from first call to performAuthorizationRequest is still alive, if it receives a valid authorization response, it resolves the this.authorizationResponse Promise. But with the second call to performAuthorizationRequest the this.authorizationResponse promise from the first call to performAuthorizationRequest gets replaced with another one.

The first call therefore resolves a promise which is not anymore "accessible". The call to completeAuthorizationRequest invoked by completeAuthorizationRequestIfPossible returns the this.authorizationResponse promise, which throws an error or never resolves in the case of #95.

The user could only sign in, after restarting the app.

Steps to reproduce the behavior

1.) Start the example electron app (googlesamples/appauth-js-electron-sample)
2.) Click "Sign in"
3.) Go back to the app without completing consent screen, but keeping the tab open
4.) Click "Sign in" again
5.) Close newly opened tab without completing consent screen
6.) Complete consent screen in the tab opened with the first click on "Sign in"

Environment

• AppAuth-JS version: 1.2.6
• AppAuth-JS Environment : Node

[pixtron]

As i understand the current code base

a.) the purpose of performAuthorizationRequest should be to perform the request. So any error which occurs until opening the consent screen in the browser should be returned by this method.
This could be implemented by returning a promise in: 785b4fe in PR #133.

b.) the purpose of completeAuthorizationRequest should be to check, if there was an incoming authorization response, and if one is detected validate it. If there was a valid authorization response resolve with AuthorizationRequestResponse otherwise resolve with null. And only error if there is an error during the check of a authorization response.

c.) the purpose of completeAuthorizationRequestIfPossible is to invoke the notifier with the authorization response, when completeAuthorizationRequest resolves with a AuthorizationRequestResponse.

Therefore i'd suggest that this.autorizationPromise should be setup by the server, when it receives a authorization response, so it would be possible to avoid overriding it by a subsequent request.

If there is no this.authorizationPromise available in completeAuthorizationRequest it should not reject, but resolve with null, as the absence of this.authorizationPromise at this point tells us that there is no authorization response available yet.

AppAuth-JS/src/node_support/node_request_handler.ts

Lines 122 to 128 in f34322d

	protected completeAuthorizationRequest(): Promise<AuthorizationRequestResponse|null> {
	if (!this.authorizationPromise) {
	return Promise.reject(
	'No pending authorization request. Call performAuthorizationRequest() ?');
	}
	return this.authorizationPromise;

@tikurahul what are your thoughts regarding my suggestion?

[tikurahul]

Thanks for your PR. Please give me some to take a look.

[pixtron]

The PR does not yet fix the issue. Wanted to read your opinion first.

…

Am 14.12.2019 um 18:22 schrieb Rahul Ravikumar ***@***.***>:  Thanks for your PR. Please give me some to take a look. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[tikurahul]

We don't really want to support this.