Merge pull request #10 from solid/refactor-error-response

Refactor AuthenticationResponse.errorResponse method

Author: dmitrizagidulin

package-lock.json

@@ -14,7 +14,7 @@
     "dist": "webpack --progress",
     "prepare": "npm run build && npm run dist && npm run test",
     "preversion": "npm test",
-    "test": "mocha"
+    "test": "mocha --timeout=10000"
   },
   "repository": {
     "type": "git",
@@ -43,6 +43,7 @@
     "@trust/webcrypto": "0.9.2",
     "base64url": "^3.0.0",
     "node-fetch": "^2.1.2",
+    "standard-http-error": "^2.0.1",
     "text-encoding": "^0.6.4",
     "whatwg-url": "^6.4.1"
   },

package.json

@@ -11,11 +11,29 @@ const FormUrlEncoded = require('./FormUrlEncoded')
 const IDToken = require('./IDToken')
 const Session = require('./Session')
 const onHttpError = require('./onHttpError')
+const HttpError = require('standard-http-error')
 
 /**
  * AuthenticationResponse
  */
 class AuthenticationResponse {
+  /**
+   * @param rp {RelyingParty}
+   * @param [redirect] {string} req.query
+   * @param [body] {string} req.body.text
+   * @param session {Session|Storage} req.session or localStorage or similar
+   * @param params {object} hashmap
+   * @param mode {string} 'query'/'fragment'/'form_post',
+   *   determined in `parseResponse()`
+   */
+  constructor ({rp, redirect, body, session, mode, params = {}}) {
+    this.rp = rp
+    this.redirect = redirect
+    this.body = body
+    this.session = session
+    this.mode = mode
+    this.params = params
+  }
 
   /**
    * validateResponse
@@ -24,14 +42,15 @@ class AuthenticationResponse {
    * Authentication response validation.
    *
    * @param {string|Object} response
-   * @returns {Promise}
+   *
+   * @returns {Promise<Session>}
    */
   static validateResponse (response) {
     return Promise.resolve(response)
       .then(this.parseResponse)
+      .then(this.errorResponse)
       .then(this.matchRequest)
       .then(this.validateStateParam)
-      .then(this.errorResponse)
       .then(this.validateResponseMode)
       .then(this.validateResponseParams)
       .then(this.exchangeAuthorizationCode)
@@ -42,15 +61,16 @@ class AuthenticationResponse {
   /**
    * parseResponse
    *
-   * @param {Object} response
-   * @returns {Promise}
+   * @param {object} response
+   *
+   * @returns {object}
    */
   static parseResponse (response) {
     let {redirect, body} = response
 
     // response must be either a redirect uri or request body, but not both
     if ((redirect && body) || (!redirect && !body)) {
-      throw new Error('Invalid response mode')
+      throw new HttpError(400, 'Invalid response mode')
     }
 
     // parse redirect uri
@@ -59,7 +79,7 @@ class AuthenticationResponse {
       let {search, hash} = url
 
       if ((search && hash) || (!search && !hash)) {
-        throw new Error('Invalid response mode')
+        throw new HttpError(400, 'Invalid response mode')
       }
 
       if (search) {
@@ -82,6 +102,37 @@ class AuthenticationResponse {
     return response
   }
 
+  /**
+   * errorResponse
+   *
+   * @param {AuthenticationResponse} response
+   *
+   * @throws {Error} If response params include the OAuth2 'error' param,
+   *   throws an error based on it.
+   *
+   * @returns {AuthenticationResponse} Chainable
+   *
+   * @todo Figure out HTTP status code (typically 400, 401 or 403)
+   *   based on the OAuth2/OIDC `error` code, probably using an external library
+   */
+  static errorResponse (response) {
+    const errorCode = response.params.error
+
+    if (errorCode) {
+      const errorParams = {}
+      errorParams['error'] = errorCode
+      errorParams['error_description'] = response.params['error_description']
+      errorParams['error_uri'] = response.params['error_uri']
+      errorParams['state'] = response.params['state']
+
+      const error = new Error(`AuthenticationResponse error: ${errorCode}`)
+      error.info = errorParams
+      throw error
+    }
+
+    return response
+  }
+
   /**
    * matchRequest
    *
@@ -130,22 +181,6 @@ class AuthenticationResponse {
     })
   }
 
-  /**
-   * errorResponse
-   *
-   * @param {Object} response
-   * @returns {Promise}
-   */
-  static errorResponse (response) {
-    let error = response.params.error
-
-    if (error) {
-      return Promise.reject(error)
-    }
-
-    return Promise.resolve(response)
-  }
-
   /**
    * validateResponseMode
    *
@@ -321,14 +356,25 @@ class AuthenticationResponse {
   /**
    * decodeIDToken
    *
-   * @param {Object} response
-   * @returns {Promise}
+   * Note: If the `id_token` is not present in params, this method does not
+   * get called (short-circuited in `validateIDToken()`).
+   *
+   * @param response {AuthenticationResponse}
+   * @param response.params {object}
+   * @param [response.params.id_token] {string} IDToken encoded as a JWT
+   *
+   * @returns {AuthenticationResponse} Chainable
    */
   static decodeIDToken (response) {
     let jwt = response.params.id_token
 
-    if (jwt) {
+    try {
       response.decoded = IDToken.decode(jwt)
+    } catch (decodeError) {
+      const error = new HttpError(400, 'Error decoding ID Token')
+      error.cause = decodeError
+      error.info = { id_token: jwt }
+      throw error
     }
 
     return response

src/AuthenticationResponse.js

@@ -233,19 +233,21 @@ class RelyingParty extends JSONDocument {
    *
    * @param response {string} req.query or req.body.text
    * @param session {Session|Storage} req.session or localStorage or similar
-   * @returns {Promise<Object>} Custom response object, with `params` and
-   *   `mode` properties
+   *
+   * @returns {Promise<Session>}
    */
-  validateResponse (response, session) {
-    session = session || this.store
+  validateResponse (response, session = this.store) {
+    let options
 
     if (response.match(/^http(s?):\/\//)) {
-      response = { rp: this, redirect: response, session }
+      options = { rp: this, redirect: response, session }
     } else {
-      response = { rp: this, body: response, session }
+      options = { rp: this, body: response, session }
     }
 
-    return AuthenticationResponse.validateResponse(response)
+    const authResponse = new AuthenticationResponse(options)
+
+    return AuthenticationResponse.validateResponse(authResponse)
   }
 
   /**

src/RelyingParty.js

@@ -45,12 +45,12 @@ class Session {
   /**
    * @param response {AuthenticationResponse}
    *
-   * @returns {Session}
+   * @returns {Session} RelyingParty Session object
    */
   static fromAuthResponse (response) {
     const RelyingParty = require('./RelyingParty')  // import here due to circular dep
 
-    let payload = response.decoded.payload
+    let idClaims = response.decoded && response.decoded.payload || {}
 
     let { rp } = response
 
@@ -65,14 +65,14 @@ class Session {
     let options = {
       credentialType,
       sessionKey,
-      issuer: payload.iss,
+      issuer: idClaims.iss,
+      idClaims,
       authorization: {
         client_id: registration['client_id'],
         access_token: response.params['access_token'],
         id_token: response.params['id_token'],
         refresh_token: response.params['refresh_token']
-      },
-      idClaims: response.decoded && response.decoded.payload,
+      }
     }
 
     return Session.from(options)

src/Session.js

@@ -166,18 +166,29 @@ describe('AuthenticationResponse', () => {
    * errorResponse
    */
   describe('errorResponse', () => {
-    it('should reject with error response', () => {
-      return AuthenticationResponse.errorResponse({
-        params: {
-          error: 'access_denied'
-        }
-      }).should.be.rejectedWith('access_denied')
+    it('should throw an error if error param is present', done => {
+      const errorParams = {
+        error: 'access_denied',
+        error_description: 'Access denied',
+        error_uri: 'https://example.com/123',
+        state: '$tate'
+      }
+      const response = new AuthenticationResponse({ params: {...errorParams} })
+      try {
+        AuthenticationResponse.errorResponse(response)
+      } catch (error) {
+        error.message.should
+          .equal('AuthenticationResponse error: access_denied')
+        error.info.should.eql(errorParams)
+        done()
+      }
     })
 
-    it('should resolve with its argument', () => {
-      let response = { params: {} }
-      return AuthenticationResponse.errorResponse(response)
-        .should.eventually.equal(response)
+    it('should return its argument if no errors', () => {
+      let response = new AuthenticationResponse({})
+
+      AuthenticationResponse.errorResponse(response)
+        .should.equal(response)
     })
   })
 
@@ -477,10 +488,11 @@ describe('AuthenticationResponse', () => {
         .should.be.instanceof(JWT)
     })
 
-    it('should ignore response without id_token', () => {
-      delete response.params.id_token
-      expect(AuthenticationResponse.decodeIDToken(response).decoded)
-        .to.be.undefined
+    it('should throw an error on invalid id_token', () => {
+      response.params.id_token = 'inva1id'
+      expect(() => {
+        AuthenticationResponse.decodeIDToken(response)
+      }).to.throw('Error decoding ID Token')
     })
 
     it('should return its argument', () => {

test/AuthenticationResponseSpec.js

@@ -543,8 +543,8 @@ describe('RelyingParty', () => {
     })
 
     it('should create an AuthenticationResponse instance', () => {
-      let response = {}
-      sinon.stub(AuthenticationResponse, 'validateResponse').resolves(response)
+      const session = {}
+      sinon.stub(AuthenticationResponse, 'validateResponse').resolves(session)
 
       let store = {}
       let rp = new RelyingParty({ store })
@@ -553,9 +553,8 @@ describe('RelyingParty', () => {
 
       return rp.validateResponse(uri)
         .then(res => {
-          expect(res).to.equal(response)
-          expect(AuthenticationResponse.validateResponse).to.have.been
-            .calledWith({ rp, session: store, redirect: uri })
+          expect(res).to.equal(session)
+          expect(AuthenticationResponse.validateResponse).to.have.been.called()
         })
     })
   })