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

Support functions for provider.auth and provider.token #463

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions API.md
Original file line number Diff line number Diff line change
Expand Up @@ -191,10 +191,13 @@ The `server.auth.strategy()` method requires the following strategy options:
- `'HMAC-SHA1'` - default
- `'RSA-SHA1'` - in that case, the `clientSecret` is your RSA private key
- `temporary` - the temporary credentials (request token) endpoint (OAuth 1.0a only).
It may be passed either a string, or a function which takes the client's `request` and returns a string.
- `useParamsAuth` - boolean that determines if OAuth client id and client secret will be sent
as parameters as opposed to an Authorization header (OAuth 2.0 only). Defaults to `false`.
- `auth` - the authorization endpoint URI.
It may be passed either a string, or a function which takes the client's `request` and returns a string.
- `token` - the access token endpoint URI.
It may be passed either a string, or a function which takes the client's `request` and returns a string.
- `scope` - an array of scope strings (OAuth 2.0 only).
- `scopeSeparator` - the scope separator character (OAuth 2.0 only). Only required when a
provider has a broken OAuth 2.0 implementation. Defaults to space (Facebook and GitHub
Expand Down
6 changes: 3 additions & 3 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,16 +39,16 @@ exports.plugin = {
internals.provider = Joi.object({
name: Joi.string().optional().default('custom'),
protocol: ['oauth', 'oauth2'],
auth: Joi.string().required(),
token: Joi.string().required(),
auth: Joi.alternatives(Joi.string(), Joi.func()).required(),
token: Joi.alternatives(Joi.string(), Joi.func()).required(),
headers: Joi.object(),
profile: Joi.func(),
profileMethod: Joi.valid('get', 'post').default('get')
})
.when('.protocol', {
is: 'oauth',
then: Joi.object({
temporary: Joi.string().required(),
temporary: Joi.alternatives(Joi.string(), Joi.func()).required(),
Copy link
Author

@charlesbjohnson charlesbjohnson Jun 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My interest is only in auth and token, but I thought it might be odd to leave this out.

signatureMethod: Joi.valid('HMAC-SHA1', 'RSA-SHA1').default('HMAC-SHA1')
}),
otherwise: Joi.object({
Expand Down
32 changes: 21 additions & 11 deletions lib/oauth.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,10 @@ exports.v1 = function (settings) {

// Obtain temporary OAuth credentials

const temporaryEndpoint = internals.resolveProviderEndpoint(request, settings.provider.temporary);
const oauth_callback = internals.location(request, protocol, settings.location);
try {
var { payload: temp } = await client.temporary(oauth_callback);
var { payload: temp } = await client.temporary(temporaryEndpoint, oauth_callback);
}
catch (err) {
return h.unauthenticated(err, { credentials });
Expand All @@ -79,7 +80,8 @@ exports.v1 = function (settings) {
Hoek.merge(authQuery, request.query);
}

return h.redirect(settings.provider.auth + '?' + internals.queryString(authQuery)).takeover();
const authEndpoint = internals.resolveProviderEndpoint(request, settings.provider.auth);
return h.redirect(authEndpoint + '?' + internals.queryString(authQuery)).takeover();
}

// Authorization callback
Expand All @@ -102,8 +104,9 @@ exports.v1 = function (settings) {

// Obtain token OAuth credentials

const tokenEndpoint = internals.resolveProviderEndpoint(request, settings.provider.token);
try {
var { payload: token } = await client.token(state.token, request.query.oauth_verifier, state.secret);
var { payload: token } = await client.token(tokenEndpoint, state.token, request.query.oauth_verifier, state.secret);
}
catch (err) {
return h.unauthenticated(err, { credentials });
Expand Down Expand Up @@ -230,7 +233,9 @@ exports.v2 = function (settings) {
}

h.state(cookie, state);
return h.redirect(settings.provider.auth + '?' + internals.queryString(query)).takeover();

const authEndpoint = internals.resolveProviderEndpoint(request, settings.provider.auth);
return h.redirect(authEndpoint + '?' + internals.queryString(query)).takeover();
}

// Authorization callback
Expand Down Expand Up @@ -286,8 +291,9 @@ exports.v2 = function (settings) {

// Obtain token

const tokenEndpoint = internals.resolveProviderEndpoint(request, settings.provider.token);
try {
var { res: tokenRes, payload } = await Wreck.post(settings.provider.token, requestOptions);
var { res: tokenRes, payload } = await Wreck.post(tokenEndpoint, requestOptions);
}
catch (err) {
return h.unauthenticated(Boom.internal('Failed obtaining ' + name + ' access token', err), { credentials });
Expand Down Expand Up @@ -387,28 +393,26 @@ exports.Client = internals.Client = function (options) {

this.provider = options.name;
this.settings = {
temporary: internals.Client.baseUri(options.provider.temporary),
token: internals.Client.baseUri(options.provider.token),
clientId: options.clientId,
clientSecret: options.provider.signatureMethod === 'RSA-SHA1' ? options.clientSecret : internals.encode(options.clientSecret || '') + '&',
signatureMethod: options.provider.signatureMethod
};
};


internals.Client.prototype.temporary = function (oauth_callback) {
internals.Client.prototype.temporary = function (uri, oauth_callback) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change. The Client is a public interface. Can't change it.


// Temporary Credentials (2.1)

const oauth = {
oauth_callback
};

return this._request('post', this.settings.temporary, null, oauth, { desc: 'temporary credentials' });
return this._request('post', internals.Client.baseUri(uri), null, oauth, { desc: 'temporary credentials' });
};


internals.Client.prototype.token = function (oauthToken, oauthVerifier, tokenSecret) {
internals.Client.prototype.token = function (uri, oauthToken, oauthVerifier, tokenSecret) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same on every Client endpoint... I thought that was implicit... :-)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted the changes to these method signatures but kept their ability to resolve endpoints from a function, if a function is provided. It's a bit ugly since it potentially requires setting client.settings.token and client.settings.temporary in two passes, but I think it only applies to those who choose to upgrade to using a function instead of a string.

I didn't catch that Client was exported before (d'oh 😅). I think this should resolve the breaking change, right? Is there something else that I'm missing?


// Token Credentials (2.3)

Expand All @@ -417,7 +421,7 @@ internals.Client.prototype.token = function (oauthToken, oauthVerifier, tokenSec
oauth_verifier: oauthVerifier
};

return this._request('post', this.settings.token, null, oauth, { secret: tokenSecret, desc: 'token credentials' });
return this._request('post', internals.Client.baseUri(uri), null, oauth, { secret: tokenSecret, desc: 'token credentials' });
};


Expand Down Expand Up @@ -723,6 +727,12 @@ internals.getProtocol = function (request, settings) {
};


internals.resolveProviderEndpoint = function (request, endpoint) {

return typeof endpoint === 'function' ? endpoint(request) : endpoint;
};


internals.resolveProviderParams = function (request, params) {

const obj = typeof params === 'function' ? params(request) : params;
Expand Down
87 changes: 87 additions & 0 deletions test/oauth.js
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,47 @@ describe('Bell', () => {
expect(res.headers.location).to.equal(mock.uri + '/auth?oauth_token=1&runtime=true');
});

it('authenticates an endpoint via oauth with auth provider (function)', async (flags) => {

const mock = await Mock.v1(flags);
const server = Hapi.server({ host: 'localhost', port: 8080 });
await server.register(Bell);

const provider = Hoek.merge(mock.provider, {
temporary: (request) => request.query.host + '/temporary',
auth: (request) => request.query.host + '/auth',
token: (request) => request.query.host + '/token'
});

server.auth.strategy('custom', 'bell', {
password: 'cookie_encryption_password_secure',
isSecure: false,
clientId: 'test',
clientSecret: 'secret',
provider
});

server.route({
method: '*',
path: '/login',
options: {
auth: 'custom',
handler: function (request, h) {

return request.auth.credentials;
}
}
});

const res1 = await server.inject('/login?host=' + mock.uri);

const cookie = res1.headers['set-cookie'][0].split(';')[0] + ';';
const res2 = await mock.server.inject(res1.headers.location + '&host=' + mock.uri);

const res3 = await server.inject({ url: res2.headers.location + '&host=' + mock.uri, headers: { cookie } });
expect(res3.statusCode).to.equal(200);
});

it('authenticates an endpoint via oauth with auth provider parameters', async (flags) => {

const mock = await Mock.v1(flags);
Expand Down Expand Up @@ -897,6 +938,46 @@ describe('Bell', () => {

describe('v2()', () => {

it('authenticates an endpoint with provider (function)', async (flags) => {

const mock = await Mock.v2(flags);
const server = Hapi.server({ host: 'localhost', port: 8080 });
await server.register(Bell);

const provider = Hoek.merge(mock.provider, {
auth: (request) => request.query.host + '/auth',
token: (request) => request.query.host + '/token'
});

server.auth.strategy('custom', 'bell', {
password: 'cookie_encryption_password_secure',
isSecure: false,
clientId: 'test',
clientSecret: 'secret',
provider
});

server.route({
method: '*',
path: '/login',
options: {
auth: 'custom',
handler: function (request, h) {

return request.auth.credentials;
}
}
});

const res1 = await server.inject('/login?host=' + mock.uri);
const cookie = res1.headers['set-cookie'][0].split(';')[0] + ';';

const res2 = await mock.server.inject(res1.headers.location + '&host=' + mock.uri);

const res3 = await server.inject({ url: res2.headers.location + '&host=' + mock.uri, headers: { cookie } });
expect(res3.statusCode).to.equal(200);
});

it('authenticates an endpoint with provider parameters', async (flags) => {

const mock = await Mock.v2(flags);
Expand Down Expand Up @@ -2154,6 +2235,12 @@ describe('Bell', () => {
expect(OAuth.Client.baseUri('http://example.com:8080/x')).to.equal('http://example.com:8080/x');
expect(OAuth.Client.baseUri('https://example.com:8080/x')).to.equal('https://example.com:8080/x');
});

it('passes through without port', () => {

expect(OAuth.Client.baseUri('http://example.com/x')).to.equal('http://example.com/x');
expect(OAuth.Client.baseUri('https://example.com/x')).to.equal('https://example.com/x');
});
Comment on lines +2239 to +2243
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated, but after my changes we lost coverage on this case. This adds it back.

});

describe('signature()', () => {
Expand Down