feat: add new token manager for ICP4D #26

dpopp07 · 2019-05-28T15:35:13Z

Changes:

Introduced a constructor parameter, authentication_type, for specifying the authentication pattern. This will be required for using ICP4D
A new token manager is added, handling ICP4D-style requests
I created a new generic class, called JwtTokenManager for all token-managing needs. This is a base class for the new ICP token manager. I also refactored the IAM token manger to extend the new JWT class too

I still need to write tests but want to start getting feedback on the implementation now, so I don't have to re-write the tests too many times

TODO:

Write tests
Add documentation to the README for using authentication_type
Add a second URL parameter for ICP4D authentication

cc @ehdsouza @mediumTaj @lpatino10 @mamoonraja

Introduce a constructor parameter, `authentication_type`, for specifying the authentication pattern. Required for using ICP4D

Also, make `authentication_type` non-case-sensitive

mkistler

Here's a first batch of comments.

auth/icp-token-manager.ts

auth/jwt-token-manager.ts

auth/icp-token-manager.ts

iam-token-manager/v1.ts

auth/jwt-token-manager.ts

auth/icp-token-manager.ts

lpatino10 · 2019-05-28T18:52:12Z

Should we rename the IcpTokenManagerV1 class to something like Icp4dTokenManagerV1? The name's kind of obnoxious but I think it might be more clear, since ICP and ICP4D are different and use different auth methods. I could see someone getting tripped up by that.

dpopp07 · 2019-05-28T18:55:58Z

It is ICP4D specific, so that's definitely not a bad idea. I'll make that change, unless anyone disagrees. Definitely a bit of an obnoxious name haha

codecov · 2019-05-29T22:48:41Z

Codecov Report

❗ No coverage uploaded for pull request base (master@6d24f0a). Click here to learn what that means.
The diff coverage is 99.26%.

@@            Coverage Diff            @@
##             master      #26   +/-   ##
=========================================
  Coverage          ?   93.26%           
=========================================
  Files             ?       12           
  Lines             ?      505           
  Branches          ?      147           
=========================================
  Hits              ?      471           
  Misses            ?       33           
  Partials          ?        1

Impacted Files	Coverage Δ
lib/requestwrapper.ts	`84.49% <ø> (ø)`
auth/jwt-token-manager-v1.ts	`100% <100%> (ø)`
auth/utils.ts	`100% <100%> (ø)`
iam-token-manager/v1.ts	`100% <100%> (ø)`
auth/icp4d-token-manager-v1.ts	`100% <100%> (ø)`
auth/index.ts	`100% <100%> (ø)`
auth/iam-token-manager-v1.ts	`100% <100%> (ø)`
lib/base_service.ts	`92.3% <96.29%> (ø)`

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 6d24f0a...dc3165d. Read the comment docs.

also, rename JwtTokenManager to JwtTokenManagerV1

dpopp07 · 2019-05-31T19:39:03Z

Okay last minute (small) changes have been made and all tests have been added. Officially ready for final review

@germanattanasio @mkistler @padamstx

mkistler

I left a few comments that I hope you will address, but in any event I think the code here is okay to merge.

mkistler · 2019-06-03T02:26:20Z

auth/iam-token-manager-v1.ts

@@ -0,0 +1,158 @@
+/**
+ * Copyright 2015 IBM Corp. All Rights Reserved.


Need to check copyright date on this file.

Yes, that's certainly wrong. The IAM token manager was originally written in 2018 but this specific file in this repo was added in 2019. So - should it be 2018, 2019 or 2019?

auth/jwt-token-manager-v1.ts

lib/base_service.ts

mediumTaj

👍 looks good! There are a few references to refresh tokens - Are we still refreshing tokens or are all of these meant for compatibility?

dpopp07 · 2019-06-04T16:29:40Z

We're not using refresh tokens anymore. We are requesting a new token when the token is expired and I might have referred to that as a "refresh" but I agree that may be misleading. I'll update that

lib/base_service.ts

germanattanasio

left a few comments but looks OK

padamstx

LGTM

# [0.3.0](v0.2.8...v0.3.0) (2019-06-05) ### Features * add `IcpTokenManagerV1` as a top-level export of the package ([cfa3e1b](cfa3e1b)) * add new token manager for ICP4D ([ee1ddad](ee1ddad)) * add new token manager for ICP4D ([#26](#26)) ([2097a64](2097a64)) * carry `disable_ssl_verification` through to token managers ([4f2f789](4f2f789))

ibm-devx-automation · 2019-06-05T18:29:14Z

🎉 This PR is included in version 0.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

germanattanasio · 2019-06-06T00:29:03Z

@dpopp07 can you add an icon to ibm-devx-automation 😛 ?

esminerva · 2019-06-06T01:20:47Z

no

dpopp07 · 2019-06-06T16:18:34Z

Haha I would if I could 🤷‍♂

dpopp07 · 2019-06-06T16:48:35Z

@germanattanasio Oh wait, it looks like it does have an icon!

feat: add new token manager for ICP4D

ee1ddad

Introduce a constructor parameter, `authentication_type`, for specifying the authentication pattern. Required for using ICP4D

dpopp07 requested review from germanattanasio, esminerva and padamstx May 28, 2019 15:35

dpopp07 changed the title ~~feat: add new token manager for ICP4D~~ feat: add new token manager for ICP4D - DO NOT MERGE May 28, 2019

dpopp07 changed the title ~~feat: add new token manager for ICP4D - DO NOT MERGE~~ feat: add new token manager for ICP4D - IN PROGRESS May 28, 2019

dpopp07 added 2 commits May 28, 2019 11:25

feat: add IcpTokenManagerV1 as a top-level export of the package

cfa3e1b

Also, make `authentication_type` non-case-sensitive

docs: add some documentation on authentication_type in the README

de711e7

mkistler reviewed May 28, 2019

View reviewed changes

germanattanasio reviewed May 28, 2019

View reviewed changes

auth/icp-token-manager.ts Outdated Show resolved Hide resolved

germanattanasio reviewed May 28, 2019

View reviewed changes

auth/icp-token-manager.ts Outdated Show resolved Hide resolved

germanattanasio reviewed May 28, 2019

View reviewed changes

auth/icp-token-manager.ts Outdated Show resolved Hide resolved

germanattanasio reviewed May 28, 2019

View reviewed changes

auth/icp-token-manager.ts Outdated Show resolved Hide resolved

dpopp07 added 4 commits May 28, 2019 15:05

chore: address pr comments

bd902eb

feat: carry disable_ssl_verification through to token managers

4f2f789

refactor: re-organize auth folder in sdk

70cfc23

refactor: remove disable_ssl_verification support in iam

d91cd06

dpopp07 added 5 commits May 30, 2019 16:38

refactor: refactor the token expiration time calculation

4ba77ca

also, rename JwtTokenManager to JwtTokenManagerV1

test: add unit tests for JwtTokenManagerV1

81a5da2

Merge branch 'master' into icp4d-authentication

f06beb5

test: add unit tests for icp token manager

6d718fa

test: add unit tests for base service changes

3adbe10

dpopp07 changed the title ~~feat: add new token manager for ICP4D - IN PROGRESS~~ feat: add new token manager for ICP4D May 31, 2019

dpopp07 added the ready for review label May 31, 2019

mkistler approved these changes Jun 3, 2019

View reviewed changes

refactor: move computeBasicAuthHeader to its own module

12028fc

dpopp07 added 4 commits June 3, 2019 11:26

refactor: rename icp_access_token to icp4d_access_token

799a876

docs: update copyright date in iam-token-manager-v1.js

0f4ece7

build: ignore linting .js files under auth directory

1fc9fb1

test: update base service test to use icp4d_access_token

91cbaf0

mediumTaj approved these changes Jun 4, 2019

View reviewed changes

docs: remove references to "refreshing" tokens

9ac1cee

dpopp07 changed the title ~~feat: add new token manager for ICP4D~~ WIP: feat: add new token manager for ICP4D Jun 5, 2019

dpopp07 added in progress and removed ready for review labels Jun 5, 2019

germanattanasio changed the title ~~WIP: feat: add new token manager for ICP4D~~ feat: add new token manager for ICP4D Jun 5, 2019

germanattanasio reviewed Jun 5, 2019

View reviewed changes

lib/base_service.ts Outdated Show resolved Hide resolved

germanattanasio approved these changes Jun 5, 2019

View reviewed changes

refactor: add parameter icp4d_url

dc3165d

dpopp07 removed the in progress label Jun 5, 2019

padamstx approved these changes Jun 5, 2019

View reviewed changes

germanattanasio merged commit 2097a64 into master Jun 5, 2019

ibm-devx-automation added the released label Jun 5, 2019

dpopp07 deleted the icp4d-authentication branch June 5, 2019 21:27

JurajNyiri pushed a commit to JurajNyiri/node-sdk-core that referenced this pull request Aug 22, 2024

docs: Add error handling reference to README (IBM#26)

98f3d96

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add new token manager for ICP4D #26

feat: add new token manager for ICP4D #26

dpopp07 commented May 28, 2019 •

edited

Loading

mkistler left a comment

lpatino10 commented May 28, 2019

dpopp07 commented May 28, 2019 •

edited

Loading

codecov bot commented May 29, 2019 •

edited

Loading

dpopp07 commented May 31, 2019

mkistler left a comment

mkistler Jun 3, 2019

dpopp07 Jun 3, 2019

mediumTaj left a comment

dpopp07 commented Jun 4, 2019

germanattanasio left a comment

padamstx left a comment

ibm-devx-automation commented Jun 5, 2019

germanattanasio commented Jun 6, 2019

esminerva commented Jun 6, 2019

dpopp07 commented Jun 6, 2019

dpopp07 commented Jun 6, 2019

		@@ -0,0 +1,158 @@
		/**
		* Copyright 2015 IBM Corp. All Rights Reserved.

feat: add new token manager for ICP4D #26

feat: add new token manager for ICP4D #26

Conversation

dpopp07 commented May 28, 2019 • edited Loading

mkistler left a comment

Choose a reason for hiding this comment

lpatino10 commented May 28, 2019

dpopp07 commented May 28, 2019 • edited Loading

codecov bot commented May 29, 2019 • edited Loading

Codecov Report

dpopp07 commented May 31, 2019

mkistler left a comment

Choose a reason for hiding this comment

mkistler Jun 3, 2019

Choose a reason for hiding this comment

dpopp07 Jun 3, 2019

Choose a reason for hiding this comment

mediumTaj left a comment

Choose a reason for hiding this comment

dpopp07 commented Jun 4, 2019

germanattanasio left a comment

Choose a reason for hiding this comment

padamstx left a comment

Choose a reason for hiding this comment

ibm-devx-automation commented Jun 5, 2019

germanattanasio commented Jun 6, 2019

esminerva commented Jun 6, 2019

dpopp07 commented Jun 6, 2019

dpopp07 commented Jun 6, 2019

dpopp07 commented May 28, 2019 •

edited

Loading

dpopp07 commented May 28, 2019 •

edited

Loading

codecov bot commented May 29, 2019 •

edited

Loading