Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add and export InvalidHeaderError #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ricardogama
Copy link

@ricardogama ricardogama commented Dec 5, 2016

This PR adds the InvalidHeaderError which inherits from TypeError. Throwing a custom error and exporting it allows a more precise error handling, since it's possible to know that the error was thrown by parse. For example:

const { TokenExpiredError, verify } = require('jsonwebtoken');
const { InvalidHeaderError, parse } = require('auth-header');
const { UnauthorizedError } = require('src/errors');
const { userManager } = require('src/user');

function getUserByAuthorizationToken(header) {
  try {
    const { token } = parse(header);

    verify(token);

    return userManager.getUserByToken(token);
  } catch (e) {
    if (e instanceof InvalidHeaderError) {
      throw new UnauthorizedError('Invalid header');
    }

    if (e instanceof TokenExpiredError) {
      throw new UnauthorizedError('Expired token');
    }

    throw e;
  }
}

So I replaced all TypeError throws by InvalidHeaderError on the parse module and modified its tests accordingly, adding an assert for its message.

The InvalidHeaderError implementation was based on the Custom Error Types section of the MDN Error documentation.

@ricardogama ricardogama force-pushed the feature/invalid-header-error branch from 2990601 to c6aedc2 Compare December 5, 2016 11:42
@ricardogama
Copy link
Author

@izaakschroeder Any thoughts?

@izaakschroeder
Copy link
Owner

Thinking of maybe using https://www.npmjs.com/package/es6-error

@izaakschroeder
Copy link
Owner

Thanks for the contribution so far 😄 Will look at this in more detail tonight after I get some rest.

@ricardogama ricardogama force-pushed the feature/invalid-header-error branch from bd61193 to c6aedc2 Compare December 12, 2016 17:19
@ricardogama
Copy link
Author

ricardogama commented Dec 12, 2016

@izaakschroeder If you're ok to add it as dependency that's awesome, just let me know.

BTW, any reason for babel-core being a dependency? Seems duplicated.

@ricardogama
Copy link
Author

@izaakschroeder ping

@izaakschroeder
Copy link
Owner

Sorry for the delay, super busy as of late. Feel free to add the dependency 😄 And babel-cli should be a devDependency for generating dist, but that's it I think. If it's not like that, feel free to fix it up! 🎉 (Or I can get to it soon™)

@ricardogama ricardogama force-pushed the feature/invalid-header-error branch from c6aedc2 to 81ad6cc Compare December 21, 2016 14:04
@ricardogama ricardogama force-pushed the feature/invalid-header-error branch from 81ad6cc to ab4b8ef Compare December 21, 2016 14:06
@ricardogama
Copy link
Author

@izaakschroeder Refactored in favour of es6-error and removed babel-core as dependency, please review.

@ricardogama
Copy link
Author

@izaakschroeder ping

1 similar comment
@ricardogama
Copy link
Author

@izaakschroeder ping

@ricardogama
Copy link
Author

@izaakschroeder ping?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants