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 option to pass attributes to unauthorized replies #46

Merged
merged 1 commit into from Nov 17, 2015

Conversation

@maxbeatty
Copy link
Contributor

maxbeatty commented Nov 15, 2015

This addition allows someone to optionally specify an attributes object to pass to Boom.unauthorized.

Some backstory: AWS SNS allows subscriptions from HTTPS endpoints with basic authentication. Their confirmation flow involves an initial unauthenticated request to the endpoint followed by an authenticated request. I wasn't receiving the second authenticated request because the WWW-Authentication header didn't specify a realm (e.g. WWW-Authentication: Basic realm="hapi"). Thankfully, Boom.unauthorized made this easy to add.

@@ -741,6 +741,42 @@ it('accepts request object in validateFunc', (done) => {
});
});

it('includes additional attributes in WWW-Authenticate header', (done) => {
const server = new Hapi.Server();

This comment has been minimized.

Copy link
@mtharrison

mtharrison Nov 15, 2015

Member

Missing blank line here

This comment has been minimized.

Copy link
@maxbeatty

maxbeatty Nov 15, 2015

Author Contributor

👍

off topic: what do you think about a separate PR to add .eslintrc with hapi config and .editorconfig to match?

This comment has been minimized.

Copy link
@mtharrison

mtharrison Nov 15, 2015

Member

This is already built into lab. npm test runs the linter too.

This comment has been minimized.

Copy link
@maxbeatty

maxbeatty Nov 15, 2015

Author Contributor

I was hoping lab defaulted to that. Doesn't look like it's catching the missing new line for the arrow function but correctly spots it for traditional function declaration. I'll open an issue with hapi-scope-start

This comment has been minimized.

Copy link
@mtharrison

mtharrison Nov 15, 2015

Member

It caught it for me (and travis). That's how I knew about it ;)

This comment has been minimized.

Copy link
@maxbeatty

maxbeatty Nov 15, 2015

Author Contributor

Local issue 😳 rm -r node_modules/ && npm i && npm test caught it like it should have. found that hapi-scope-start has a nice test for exactly this

});

const request = { method: 'POST', url: '/', headers: {
// intentionally empty

This comment has been minimized.

Copy link
@mtharrison

mtharrison Nov 15, 2015

Member

Why include this empty object?

This comment has been minimized.

Copy link
@maxbeatty

maxbeatty Nov 15, 2015

Author Contributor

just to make sure the authorization header is not defined so it definitely goes to the first Boom.unauthorized. I can remove it if you prefer

This comment has been minimized.

Copy link
@mtharrison

mtharrison Nov 15, 2015

Member

Yes please.

@mtharrison

This comment has been minimized.

Copy link
Member

mtharrison commented Nov 15, 2015

Thanks for the contribution. Looks good apart from those couple of comments. Part of me wonders though if this will ever be used for anything apart from setting the realm and if it'd be clearer to just have a realm option. We'll probably just go with this though for now, it's simple enough to use and the README mentions realm.

@mtharrison mtharrison added the feature label Nov 15, 2015
@mtharrison mtharrison self-assigned this Nov 15, 2015
@mtharrison mtharrison added this to the 4.1.0 milestone Nov 15, 2015
@mtharrison

This comment has been minimized.

Copy link
Member

mtharrison commented Nov 16, 2015

@maxbeatty would you be able to squash this into a single commit?

@maxbeatty

This comment has been minimized.

Copy link
Contributor Author

maxbeatty commented Nov 16, 2015

Sure thing. What's the best way to do that? My git-foo is lacking

@maxbeatty maxbeatty force-pushed the maxbeatty:unauthorized-attributes branch from 647d497 to 57b59bb Nov 16, 2015
@maxbeatty

This comment has been minimized.

Copy link
Contributor Author

maxbeatty commented Nov 16, 2015

Thanks! That was much easier than the answers I was finding on SO

mtharrison added a commit that referenced this pull request Nov 17, 2015
add option to pass attributes to unauthorized replies
@mtharrison mtharrison merged commit 548566d into hapijs:master Nov 17, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mtharrison

This comment has been minimized.

Copy link
Member

mtharrison commented Nov 17, 2015

Thanks, published in v4.1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.