AppAuth-JS icon indicating copy to clipboard operation
AppAuth-JS copied to clipboard

Improved error handling for NodeRequestor

Open pixtron opened this issue 4 years ago • 5 comments

Expected Behavior

Describe expected behavior

Failing requests to /token endpoint (status code 400) should reject with the full error returned by the client in the body of the request. The body contains information for the reason of a failing request (eg. refresh_token expired, client authentication not successful)

Describe the problem

Currently the requestors rejects with new AppAuthError(statusMessage) (FetchRequestor rejects with new AppAuthError(statusCode, statusMessage)). As the app does not receive the error response (see RFC 6749 section 5.2) it can't handle accordingly.

Actual Behavior

NodeRequestor rejects with Bad Request only.

Steps to reproduce the behavior

Issue a Token Request with an invalid authorization code:

const requestor = new NodeRequestor();
const tokenHandler = new BaseTokenRequestHandler(requestor);
const request = new TokenRequest({
  client_id: idpConfig.clientId,
  redirect_uri: idpConfig.redirectUri,
  grant_type: GRANT_TYPE_AUTHORIZATION_CODE,
  code: 'INVALID CODE',
  refresh_token: undefined,
  extras: extras
});

tokenHandler.performTokenRequest(serviceConfiguration, request)
  .then(response => {})
  .catch(err => {
    //err is {message:'Bad Request'},
    //err should be {message: 'Bad Request', code: 400, body: { error: 'invalid_grant', error_description: 'Malformed auth code.' }}
  });

Environment

  • AppAuth-JS version: 1.2.4
  • AppAuth-JS Environment: Node (also applicable for Browser in JQueryRequestor and FetchRequestor )

pixtron avatar Jul 31 '19 11:07 pixtron

Yes. I should get to this soon. TypeScript 3.6 is out too. So will do this as a fast-follow.

tikurahul avatar Aug 30 '19 15:08 tikurahul

Whats the status here?

PeteMac88 avatar Mar 23 '21 14:03 PeteMac88

For me this looks good. My project also relies on openid/AppAuth-JS. And i want to handle different token request errors separatly.

Can we merge the PR??

@tikurahul ?

pommelinho avatar Dec 10 '21 07:12 pommelinho

Whats the status here?

alexsanderp avatar Feb 18 '22 15:02 alexsanderp

@tikurahul can we merge this please?

paulinang avatar Mar 29 '23 01:03 paulinang