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

Add support for providing clientSecret as a function #476

merged 1 commit into from
Mar 12, 2021

Conversation

Ginden
Copy link
Contributor

@Ginden Ginden commented Jan 22, 2021

Partially resolves #462

@@ -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.

@@ -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.

@Ginden Ginden requested a review from Nargonath February 3, 2021 08:44
@Ginden
Copy link
Contributor Author

Ginden commented Feb 10, 2021

This blocks our application development. @Nargonath

@Ginden
Copy link
Contributor Author

Ginden commented Mar 10, 2021

Any update on this? @Nargonath

@Nargonath
Copy link
Member

Sorry for the lack of udpate on this @Ginden. I was waiting for others to chime in. I'll try to get some feedbacks and we'll see. 😃

@Nargonath Nargonath added this to the 12.2.0 milestone Mar 12, 2021
@Nargonath Nargonath merged commit feb15b6 into hapijs:master Mar 12, 2021
@Nargonath
Copy link
Member

Nargonath commented Mar 15, 2021

It has been published as v12.2.0. Thanks @Ginden !

https://github.com/hapijs/bell/releases/tag/v12.2.0

@Nargonath Nargonath mentioned this pull request Apr 2, 2021
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.

Sign in with apple
3 participants