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

Allow to update the Management API token #141

Merged
merged 3 commits into from
Jul 13, 2018
Merged

Conversation

lbalmaceda
Copy link
Contributor

As I wrote this I thought that good alternative would be to have a function with a callback the user can implement their own logic in. So they can code a token refresher and feed that instead.

Fixes: #131

@lbalmaceda lbalmaceda added this to the v1-Next milestone Jul 12, 2018
README.md Outdated
@@ -268,6 +268,9 @@ ManagementAPI mgmt = new ManagementAPI("{YOUR_DOMAIN}", holder.getAccessToken())

Click [here](https://auth0.com/docs/api/management/v2/tokens) for more information on how to obtain API Tokens.

In the event of token expiration a new one can be set to an existing `ManagementAPI` instance by calling the `setApiToken` method with the new token.
Copy link
Contributor

Choose a reason for hiding this comment

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

Awkward .... consider:

An expired token for an existing ManagementAPI instance can be replaced by calling the setApiToken method with the new token.

@@ -45,6 +46,18 @@ public ManagementAPI(String domain, String apiToken) {
.build();
}

/**
* Update the API token to use on new calls. This is useful when the token is about to expire or it already has.
Copy link
Contributor

Choose a reason for hiding this comment

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

"when the token is about to expire or already has"

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 had doubts on this one 😛

@@ -45,6 +46,18 @@ public ManagementAPI(String domain, String apiToken) {
.build();
}

/**
* Update the API token to use on new calls. This is useful when the token is about to expire or it already has.
* Please note you'll need to obtain the correspondent entity again for this to apply. e.g. call {@link #clients()} again.
Copy link
Contributor

Choose a reason for hiding this comment

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

"corresponding"

* @param apiToken the token to authenticate the calls with.
*/
public void setApiToken(String apiToken) {
Asserts.assertNotNull(apiToken, "api token");
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be wrong but it looks like you've got these params backwards?

http://junit.sourceforge.net/javadoc/org/junit/Assert.html#assertNotNull(java.lang.Object)

I'm not sure if Asserts.assertNotNull() is the same one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm wrong ... Assert is a unit testing thing, Asserts is in this lib

Copy link
Contributor Author

@lbalmaceda lbalmaceda Jul 12, 2018

Choose a reason for hiding this comment

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

Yes! Asserts is a helper class that throws an exception if a value is different than the expected. I think I mentioned this on one of my reviews to your PRs last week. It's basically to avoid boilerplate code.

if (value == null) {
   throw new IllegalArgumentException(String.format("'%s' cannot be null!", name));
}

BTW. At the top of the class you can see the imports and in this case, that I'm importing this lib Asserts class import com.auth0.utils.Asserts. Also in case the class is on the same package, there's no need to explicitly import it.

README.md Outdated
@@ -264,10 +264,13 @@ TokenHolder holder = authRequest.execute();
ManagementAPI mgmt = new ManagementAPI("{YOUR_DOMAIN}", holder.getAccessToken());
```

(Note that the simplified should have error handling, and ideally cache the obtained token until it expires instead of requesting one access token for each Management API v2 invocation).
(Note that the simplified should have error handling, and ideally cache the obtained token until it expires instead of requesting one access token for each Management API v2 invocation).
Copy link
Member

Choose a reason for hiding this comment

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

the simplified sounds weird.

@cocojoe
Copy link
Member

cocojoe commented Jul 13, 2018

This is fine for now, maybe think about this in next major ;)

@lbalmaceda lbalmaceda force-pushed the allow-to-update-token branch from b440133 to 5f855e2 Compare July 13, 2018 18:12
@lbalmaceda lbalmaceda force-pushed the allow-to-update-token branch from 5f855e2 to 6bc0ae1 Compare July 13, 2018 18:15
@lbalmaceda lbalmaceda force-pushed the allow-to-update-token branch from c51e1a9 to b7edcb7 Compare July 13, 2018 19:51
@lbalmaceda lbalmaceda merged commit 9a4890c into master Jul 13, 2018
@lbalmaceda lbalmaceda modified the milestones: v1-Next, 1.8.0 Jul 13, 2018
@damieng damieng deleted the allow-to-update-token branch May 3, 2019 20:57
@@ -15,16 +15,17 @@
public class ManagementAPI {

private final HttpUrl baseUrl;
private final String apiToken;
private String apiToken;

Choose a reason for hiding this comment

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

I don't know if ManagementAPI is supposed to be thread safe, if yes then apiToken should have volatile modifier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants