Add IAM support #929

lpatino10 · 2018-04-19T13:29:11Z

This PR adds in support for IAM in the core of the SDK. With these changes, users have the option of authenticating with Watson services using IAM tokens, which they can manage themselves or let the new IamTokenManager class handle.

Shout out to @dpopp07 for leading the way for this in the Node SDK. Definitely leaned on your changes for my implementation 😉

…rdcoded auth header

…() and refreshToken()

…ken in IamTokenManager

… with IAM

…method

…nstructor

… and add clear warnings

…tant

codecov-io · 2018-04-19T13:40:21Z

Codecov Report

Merging #929 into develop will increase coverage by 0.01%.
The diff coverage is 39.56%.

@@              Coverage Diff              @@
##             develop     #929      +/-   ##
=============================================
+ Coverage      38.77%   38.79%   +0.01%     
- Complexity      1421     1438      +17     
=============================================
  Files            559      562       +3     
  Lines          14261    14398     +137     
  Branches         831      842      +11     
=============================================
+ Hits            5530     5586      +56     
- Misses          8415     8487      +72     
- Partials         316      325       +9

Impacted Files	Coverage Δ	Complexity Δ
...oud/language_translator/v2/LanguageTranslator.java	`38.66% <0%> (-1.62%)`	`8 <0> (ø)`
...guage_classifier/v1/NaturalLanguageClassifier.java	`58.66% <0%> (-2.45%)`	`8 <0> (ø)`
...eveloper_cloud/speech_to_text/v1/SpeechToText.java	`75.43% <0%> (-0.67%)`	`47 <0> (ø)`
...watson/developer_cloud/assistant/v1/Assistant.java	`3.41% <0%> (-0.02%)`	`7 <0> (ø)`
...cloud/visual_recognition/v3/VisualRecognition.java	`63.23% <0%> (-1.43%)`	`12 <0> (ø)`
...developer_cloud/tone_analyzer/v3/ToneAnalyzer.java	`51.21% <0%> (-4.05%)`	`4 <0> (ø)`
...watson/developer_cloud/discovery/v1/Discovery.java	`55.63% <0%> (-0.31%)`	`51 <0> (ø)`
...eveloper_cloud/text_to_speech/v1/TextToSpeech.java	`73.77% <0%> (-1.86%)`	`19 <0> (ø)`
.../developer_cloud/conversation/v1/Conversation.java	`3.41% <0%> (-0.02%)`	`7 <0> (ø)`
...understanding/v1/NaturalLanguageUnderstanding.java	`56% <0%> (-3.58%)`	`8 <0> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55f9868...f44c5af. Read the comment docs.

mamoonraja · 2018-04-19T16:53:29Z

core/src/main/java/com/ibm/watson/developer_cloud/service/security/IamTokenManager.java

+    Double fractionOfTimeToLive = 0.8;
+    Long timeToLive = tokenData.getExpiresIn();
+    Long expirationTime = tokenData.getExpiration();
+    Double refreshTime = expirationTime - (timeToLive * (1.0 - fractionOfTimeToLive));


can you review this one more time before merging?

germanattanasio

We probably want to update the README with examples of how to use IAM.

germanattanasio · 2018-04-19T19:17:51Z

core/src/main/java/com/ibm/watson/developer_cloud/service/WatsonService.java

+    if (iamApiKey != null) {
+      tokenManager = new IamTokenManager(new IamOptions.Builder().apiKey(iamApiKey).build());
+    }
    apiKey = CredentialUtils.getAPIKey(name);


What about the iam_url? When running on stage1 we need to use a different IAM server

We need an CredentialUtils.getIAMUrl(name);

The URL to the IAM endpoint is not present in the VCAP credentials is it? There would be something like "url": "https://gateway-s.watsonplatform.net/assistant/api" but the SDK would have to parse out any info and build the token service URL from there. Is that in the scope of the SDKs? I don't believe I'm doing that in Node.

We need to add the URL to the VCAP services.

My apologies for the confusion, I am doing this in Node. Sounds good 👍

germanattanasio · 2018-04-19T19:18:13Z

core/src/main/java/com/ibm/watson/developer_cloud/service/WatsonService.java

-    if (getApiKey() == null) {
+    if (tokenManager != null) {
+      String accessToken = tokenManager.getToken();
+      builder.addHeader(HttpHeaders.AUTHORIZATION, "Bearer " + accessToken);


"Bearer " should be a constant

germanattanasio · 2018-04-19T19:19:12Z

assistant/src/main/java/com/ibm/watson/developer_cloud/assistant/v1/Assistant.java

+   *          calls from failing when the service introduces breaking changes.
+   * @param iamOptions the options for authenticating through IAM
+   */
+  public Assistant(String versionDate, IamOptions iamOptions) {


They just need to provide iam_api_key. iam_url (optional) and access_token (optional).

germanattanasio · 2018-04-19T19:20:37Z

core/src/main/java/com/ibm/watson/developer_cloud/service/security/IamTokenManager.java

+
+  public IamTokenManager(IamOptions options) {
+    this.apiKey = options.getApiKey();
+    this.url = (options.getUrl() != null) ? options.getUrl() : "https://iam.ng.bluemix.net/identity/token";


use a constant for the URL

germanattanasio · 2018-04-19T19:21:18Z

core/src/main/java/com/ibm/watson/developer_cloud/service/security/IamTokenManager.java

+    builder.header(HttpHeaders.AUTHORIZATION, DEFAULT_AUTHORIZATION);
+
+    FormBody formBody = new FormBody.Builder()
+        .add("grant_type", "urn:ibm:params:oauth:grant-type:apikey")


All the strings here should be constants

germanattanasio

We probably want to update the README with examples of how to use IAM.

dpopp07

@lpatino10 thanks for the shout out 😛

This all looks good to me, as far as the IAM logic goes. Since you're using a builder for the token options, I want to make sure it is clear to the users that they have to manage everything if they use the accessToken() method. It looks like you did a good job of including that in the comments - but I agree with German's suggestion of including examples in the README and maybe reiterating that fact.

dpopp07 · 2018-04-19T19:23:05Z

core/src/test/resources/valid_iam_token.json

+  "refresh_token": "99999999",
+  "token_type": "Bearer",
+  "expires_in": 3600,
+  "expiration": 999999999999999999


If someone is still maintaining this thousands of years from now, I hope they remember to change this expiration time!

dpopp07 · 2018-04-19T20:51:34Z

core/src/main/java/com/ibm/watson/developer_cloud/service/WatsonService.java

    this.name = name;
+    String iamApiKey = CredentialUtils.getIAMKey(name);
+    String iamUrl = CredentialUtils.getIAMUrl(name);
+    if (iamApiKey != null && iamUrl != null) {


Even if the IAM API key is being pulled from VCAP credentials, the IAM URL is still optional, right? I think the tokenManager should still be created if there is an IAM API key but no IAM URL

@lpatino10 will you address this? As far as I can see, that is all that's left to be done

Oops, I missed this one. Making the change now.

dpopp07

Just had a few suggestions on the documentation

dpopp07 · 2018-04-20T16:12:13Z

README.md

+
+### Using IAM
+
+When authenticating with IAM, you have the option of passing in the IAM API key, the IAM URL, and an IAM access token. **Be aware that passing in an access token means that you're assuming responsibility for maintaining that token's lifecycle.** If you instead pass in an IAM API key, the SDK will manage it for you.


This is a bit nit-picky, but I would change this to something like "the IAM API key and optionally the IAM service URL, or an IAM access token. The user would not pass in all three, so "and" may be the wrong word to use. Sorry to sound like grammar 👮

No that's helpful! I was struggling to think of how to convey what was optional or not. I like your idea 👍

dpopp07 · 2018-04-20T16:13:45Z

README.md

+// in the constructor, letting the SDK manage the IAM token
+IamOptions options = new IamOptions.Builder()
+  .apiKey("<iam_api_key>")
+  .url("<iam_url>")


Maybe note here that URL is optional and give the default value

dpopp07 · 2018-04-20T16:15:27Z

README.md

+```
+
+```java
+// in the constructor, taking control of managing IAM token


I think "assuming" is more clear here than "taking". Just my opinion

dpopp07 · 2018-04-20T16:17:07Z

README.md

+// in the constructor, taking control of managing IAM token
+IamOptions options = new IamOptions.Builder()
+  .accessToken("<access_token>")
+  .url("<iam_url>")


If the user passes in an access token, there is no need to provide the URL. They will be making the service requests, so the URL would never be used.

dpopp07 · 2018-04-20T16:17:16Z

README.md

+Discovery service = new Discovery("2017-11-07");
+IamOptions options = new IamOptions.Builder()
+  .accessToken("<access_token>")
+  .url("<iam_url>")


See comments above

dpopp07 · 2018-04-20T16:18:10Z

README.md

+service.setIamCredentials(options);
+```
+
+If at any time you would like to let the SDK take over managing your IAM token, simply override your stored IAM credentials with an IAM API key by calling the `setIamCredentials()` method again.


I like that you point this out to the user!

…_SERVICES

dpopp07

Nice! Looks good to me

lpatino10 added 15 commits April 17, 2018 13:00

feat(core): Add initial changes for IAM support

9b27f08

feat(assistant): Add testing lines for IAM support

da47bdc

refactor(core): Use proper form to make IAM API call and switch to ha…

3a841e4

…rdcoded auth header

style(core): Add comments, style changes for clarity

a671643

refactor(core): Return just the access token string from requestToken…

5f8cf38

…() and refreshToken()

refactor(core): Use constructor instead of builder for IamToken

8aaae7c

refactor(core): Change options model for IAM and keep user-managed to…

d39dbdf

…ken in IamTokenManager

refactor(core): Don't accept refresh token when instantiating service…

1bc675e

… with IAM

refactor(core): Replace IAM ServiceCall implementation with a single …

9b49c96

…method

feat(core): Add method to set user-managed access token outside of co…

9b621ad

…nstructor

refactor(core): Tweak manner of setting IAM credentials from user POV…

3dffb61

… and add clear warnings

feat(core): Get IAM API key from VCAP_SERVICES if available

0c86ade

test(core): Add test for getting IAM API key from VCAP_SERVICES

9e60eeb

test(core): Add remaining IAM unit tests

06f61dd

refactor(assistant): Remove changes for testing specifically in Assis…

170b94e

…tant

lpatino10 requested review from dpopp07 and germanattanasio April 19, 2018 13:29

Merge branch 'develop' into iam-support

c0ba4aa

lpatino10 added the in progress label Apr 19, 2018

lpatino10 added 4 commits April 19, 2018 10:12

feat(service-constructors): Add constructors for IAM authentication

722ca5c

refactor(services): Fix merge conflicts

235f273

fix(conversation): Add back method that accidentally got deleted

3341a8d

feat(core): Add check for expired refresh token

34577e1

lpatino10 added ready for review and removed in progress labels Apr 19, 2018

style(nlu): Fix checkstyle complaint

33ab80b

lpatino10 requested a review from mamoonraja April 19, 2018 14:58

mamoonraja reviewed Apr 19, 2018

View reviewed changes

fix(core): Fix null check in IAM logic

25c7a7d

germanattanasio approved these changes Apr 19, 2018

View reviewed changes

germanattanasio suggested changes Apr 19, 2018

View reviewed changes

dpopp07 approved these changes Apr 19, 2018

View reviewed changes

lpatino10 added 2 commits April 19, 2018 15:51

feat(core): Pull IAM URL from VCAP_SERVICES

f61a9a3

style(core): Use string constants for IAM logic

772ae6a

dpopp07 reviewed Apr 19, 2018

View reviewed changes

lpatino10 added 2 commits April 20, 2018 11:03

refactor(core): Only expose getToken method in IamTokenManager

66773bd

docs(main README): Add documentation about using IAM

6f80223

dpopp07 suggested changes Apr 20, 2018

View reviewed changes

lpatino10 added 2 commits April 20, 2018 13:54

docs(main README): Address review comments

31c42de

refactor(core): Allow tokenManager creation if IAM URL is not in VCAP…

f44c5af

…_SERVICES

dpopp07 approved these changes Apr 20, 2018

View reviewed changes

mamoonraja approved these changes Apr 20, 2018

View reviewed changes

lpatino10 merged commit 2a1f307 into develop Apr 20, 2018

lpatino10 deleted the iam-support branch April 20, 2018 21:41


		### Using IAM

		When authenticating with IAM, you have the option of passing in the IAM API key, the IAM URL, and an IAM access token. Be aware that passing in an access token means that you're assuming responsibility for maintaining that token's lifecycle. If you instead pass in an IAM API key, the SDK will manage it for you.

Add IAM support #929

Add IAM support #929

Uh oh!

Conversation

lpatino10 commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

germanattanasio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

germanattanasio left a comment

Choose a reason for hiding this comment

Uh oh!

dpopp07 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpopp07 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpopp07 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

lpatino10 commented Apr 19, 2018 •

edited

Loading

codecov-io commented Apr 19, 2018 •

edited

Loading

dpopp07 left a comment •

edited

Loading