-
Notifications
You must be signed in to change notification settings - Fork 309
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
Added tokenProvider functionality #196
Conversation
This looks really, really, really good and exactly what I'm after. I know we need to regenerate Management API Tokens, but it would be so much easier if the library just "took care of it" for you. |
@dctoon is there something that's stopping this PR from being merged? |
@@ -22,6 +22,7 @@ var JobsManager = require('./JobsManager'); | |||
var TicketsManager = require('./TicketsManager'); | |||
var LogsManager = require('./LogsManager'); | |||
var ResourceServersManager = require('./ResourceServersManager'); | |||
var ManagementTokenProvider = require('./ManagementTokenProvider'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need this require
@dctoon this is an amazing PR! Thanks for taking the time to do it! |
@luisrudge any idea when we might see this merged? |
I'll ping the person in charge of this repo. I can't merge it. |
@luisrudge ah okay, cheers. 👍 |
@luisrudge Please tell me this is close to getting merged? 🙏 It's such a killer feature and running a local build is causing havoc with our CI platform. The sooner this is merged the better! |
I'm sorry, I have no ETA for this. The person responsible for this is under a heavy load of work right now 😞 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dctoon also rebase
README.md
Outdated
~~~js | ||
var ManagementClient = require('auth0').ManagementClient; | ||
var ManagementTokenProvider = require('auth0').ManagementTokenProvider; | ||
var auth0 = new ManagementClient({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd do a different API like
new ManagementClient({
domain: "...",
clientId: "...",
clientSecret: "...",
});
to avoid repeating parameters in the existing one, and it won't clash with the other API with a token since it only expects domain and token.
In the case you need to pass additional CC parameters like scopes or audience you can add them in the root
new ManagementClient({
domain: "...",
clientId: "...",
clientSecret: "...",
scopes: "read:users write:users read:payment",
audience: "https://my.app.com/api"
});
For specific provider attributes like cache you can use the tokenProvider namespace
{
tokenProvider: {
cacheTTLInSeconds: 10,
// ... etc
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree - it just confuses things, and goes against the idea of seperation of concerns, the ManagementClient should just accept a TokenProvider instance rather than magically create one based on certain options being set (i.e. make it explicit - the ManagementClient requires either a token or a tokenProvider options).
if you personally want to avoid repeating parameters the write a createManagementClientWithTokenProvider()
factory function - that could even be part of the lib.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't share this concern since node-auth0 is meant mostly for backend usage hence most developers will be using client credentials to get their tokens, and abstracting client credentials will make this library easier to use out of the box.
All that does not imply that we can't share a generic TokenProvider
as part of this library for other usages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, falling over the "options interface" is somewhat of a bike-shedding exercise. the fact is that some kind of token-provider functionality is pretty fundamental to actually using this whole library in a real-world context (without it a long running application is forced to either restart or manually track "stale-token" errors and recreate the "management client" instance ... neither of which makes for a decent experience)
I'm suggesting that the implementation is technically WAY better than it is now (i.e. non-existent) so blocking the PR on the basis of "we need to abtract the client credentials" is a bad idea - such a refinement is better off being done at a later.
Put another way, @dctoon has developed what should have been implemented from the get go, in a module that your company released to support/promote you paid service. take the PR and sweat the details of the "options interface" at a later date.
the side note: I think the Object.getOwnPropertyNames(Client.prototype).forEach(fn => {
if (typeof Client.prototype[fn] === 'function')
Auth0RestClient.prototype[fn] = function(...args) {
return this.wrappedProvider(fn, args);
};
}); |
I think the most notably it needs to accept an "audience" option. I'm currently using a fork of this repo that includes this PR and I have created custom class in my own code that looks like this: const ManagementTokenProvider = require('auth0').ManagementTokenProvider;
module.exports = class TokenProvider extends ManagementTokenProvider
{
/**
* constructor
*
* @param {Object} options
* @return {TokenProvider}
*/
constructor(options)
{
super(options);
const audience = options && options.audience;
Object.defineProperty(this, 'audience', { get : () => audience });
}
clientCredentialsGrant(domain, scope)
{
return this.authenticationClient.clientCredentialsGrant({
audience: this.audience || 'https://' + domain + '/api/v2/',
scope : scope
});
}
}; I am using this inconjunction with the
|
aa83e59
to
8c813cf
Compare
Just pushed an update applying the feedback from @hzalaz. From a library perspective it makes more sense having that functionality embedded as a first class feature thats straight forward to configure.
|
throw new ArgumentError('Must provide a clientSecret'); | ||
} | ||
|
||
if (!params.audience || params.audience.length === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The audience in the client constructor is optional and it should be since it can be build from the domain + /api/v2/
unless its provided.
Also I see that the scope is optional in the constructor but not here so we need to be consistent in the docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe consider having a generic TokenProvider that requires an audience value and making ManagementTokenProvider a sub-class that explicitly sets "audience", in it's constructor options, to "https://" + domain + "/api/v2"
before passing them on to the base class constructor.
P.S. the reason I think it is important to have/provide a generic TokenProvider is because that is what you want if one happens to be interacting with an Extension API (e.g. Auth0's official Authorization Extension) ... having & exposing a generic TokenProvider means users of extension APIs have a first class helper/tool for managing access_token
s
8c813cf
to
5b19669
Compare
When its not provided is inferred from domain's
15935c1
to
4ed84da
Compare
This PR adds the possibility to specify a token provider for the Management API. The
ManagementTokenProvider
fetchesaccess_token
's via the Client Credentials Grant and caches theaccess_token
s by default.An instance of the
ManagementTokenProvider
, is passed via theManagementClient
options propertytokenProvider
.The
ManagementTokenProvider
class will by default cache theaccess_token
for the duration of the propertyexpires_in
returned in the response of the Client Credentials Grant.Example;
A custom Token Provider class only needs to implement the method
getAccessToken
and return a Promise.