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 support for providing clientSecret as a function #476

Merged
merged 1 commit into from
Mar 12, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 2 additions & 0 deletions API.md
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,8 @@ The `server.auth.strategy()` method requires the following strategy options:
object will be merged with the Wreck request object used to call the token endpoint. Such an
object can contain custom HTTP headers or TLS options (e.g.
`{ agent: new Https.Agent({ cert: myClientCert, key: myClientKey}) }`).
To allow dynamically updating secret, this option can be passed as a *function* returning string
Copy link
Member

Choose a reason for hiding this comment

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

We should mention that the fonction is called synchronously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think function returning string is equivalent to () => string in TypeScript, not () => string | Promise<string>. Though, we can just add await in lib/oauth.js:267

Copy link
Member

Choose a reason for hiding this comment

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

I agree but we didn't express any type definition here. This was just to prevent people from using async function and then wonder why it does not work the way they intended to. I don't expect every user to look at the code. I'm in favor of leaving sync for now especially you don't seem to need it. We can always reassess later if someone needs it.

to be used as `clientSecret`.
- `forceHttps` - A boolean indicating whether or not you want the redirect_uri to be forced to
https. Useful if your hapi application runs as http, but is accessed through https.
- `location` - Set the base redirect_uri manually if it cannot be inferred properly from server
Expand Down
1 change: 1 addition & 0 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ internals.schema = Joi.object({

clientSecret: Joi.alternatives()
.try(Joi.string().allow(''))
.try(Joi.function())
.conditional('provider.protocol', {
not: 'oauth',
then: Joi.object()
Expand Down
3 changes: 3 additions & 0 deletions lib/oauth.js
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,9 @@ exports.v2 = function (settings) {
if (typeof settings.clientSecret === 'string') {
query.client_secret = settings.clientSecret;
}
else if (typeof settings.clientSecret === 'function') {
query.client_secret = settings.clientSecret();
Copy link
Member

Choose a reason for hiding this comment

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

Is there any parameters that could be interesting to pass on here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

settings?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not much of a bell user myself that's just a question that crossed my mind. I just wondered if that's something you considered on your end. If you don't need it let's keep it that way then.

}
}

const requestOptions = {
Expand Down
42 changes: 42 additions & 0 deletions test/oauth.js
Original file line number Diff line number Diff line change
Expand Up @@ -928,6 +928,48 @@ describe('Bell', () => {
expect(res.headers.location).to.contain(mock.uri + '/auth?special=true&client_id=test&response_type=code&redirect_uri=http%3A%2F%2Flocalhost%3A8080%2Flogin&state=');
});

it('uses clientSecret() function', async (flags) => {

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

let callCount = 0;
const secretGetter = () => {

callCount += 1;
return 'secret';
};

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

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

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

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

const res2 = await mock.server.inject(res1.headers.location);

await server.inject({ url: res2.headers.location, headers: { cookie } });
expect(callCount, 'callCount').to.equal(1);
});

it('forces https in redirect_uri when set in options', async (flags) => {

const mock = await Mock.v2(flags);
Expand Down