Introduce a token-manager class for token handling #89

vmatyus · 2020-04-28T12:45:24Z

The new TokenManager class will be responsible only for the token handling specifically.
Changed some private fields to protected: tokenInfo, expireTime, refreshTime.
Placed getCurrentTime into the utils module to be globally usable.

Checklist

npm test passes (tip: npm run lint-fix can correct most style issues)
tests are included
documentation is changed or added

token-manager class will be responsible only for the token handling specifically and change some private fields to protected: tokenInfo, expireTime, refreshTime.

CLAassistant · 2020-04-28T12:45:30Z

All committers have signed the CLA.

mkistler

This looks good but there a few small fixups and minor improvements I'd like you to make.

mkistler · 2020-04-28T23:37:45Z

auth/token-managers/token-manager.ts

+ * A class for shared functionality for parsing, storing, and requesting
+ * JWT tokens. Intended to be used as a parent to be extended for token
+ * request management. Child classes should implement `requestToken()`
+ * to retrieve the bearer token from intended sources.


This comment needs work. First, it should not refer to "JWT tokens" since this class is about generic "tokens". Also, I think child classes also need to implement saveTokenInfo.

Fixed in e92c3de

mkistler · 2020-04-28T23:51:59Z

auth/token-managers/token-manager.ts

+import { RequestWrapper } from '../../lib/request-wrapper';
+import {getCurrentTime} from "../utils";
+
+/** Configuration options for JWT token retrieval. */


Suggested change

/** Configuration options for JWT token retrieval. */

/** Configuration options for token retrieval. */

Fixed in e92c3de

mkistler · 2020-04-28T23:53:15Z

auth/token-managers/token-manager.ts

+   * @returns {Promise}
+   */
+  protected requestToken(): Promise<any> {
+    const errMsg = '`requestToken` MUST be overridden by a subclass of JwtTokenManagerV1.';


Suggested change

const errMsg = '`requestToken` MUST be overridden by a subclass of JwtTokenManagerV1.';

const errMsg = '`requestToken` MUST be overridden by a subclass of TokenManagerV1.';

Fixed in e92c3de

mkistler · 2020-04-29T00:16:02Z

auth/token-managers/token-manager.ts

+  protected disableSslVerification: boolean;
+  protected headers: OutgoingHttpHeaders;
+  protected requestWrapperInstance: RequestWrapper;
+  protected tokenInfo: any;


While we're abstracting, I think we should delegate tokenInfo and tokenName to subclasses of TokenManager, with only accessToken visible at this level.

Sorry, but I am not catching your point. TokenInfo currently in use to check that a token already is stored in the object. Here is a reference to a line where there properties are used:
https://github.com/vmatyusGitHub/node-sdk-core/blob/2269660441b93d426932ec9b0dfb51ac1dc0159a/auth/token-managers/token-manager.ts#L98
In the constructor TokenInfo is initialised to an empty object and tokenName shall be initialised in a subclass.
Are you suggesting to have only accesToken property with type string, this way I shall remove tokenInfo and tokenName from the object's property list?

Since tokenInfo is used in the logic contained in this file, I'm fine with it existing here.

Here was my thinking: This class requires its subclasses to implement saveTokenInfo, which is passed the response of the token request. saveTokenInfo has to set some member variables ... specifically expireTime, refreshTime, and tokenInfo.

But what is tokenInfo? It is just an object that must contain a field named tokenName that contains the access token. From the point of view of a generic TokenManager, these are extraneous details. All it needs is the accessToken. So why not simplify the TokenManager class (i.e. make it more general), and simply use accessToken set by saveTokenInfo, and push the complexity of varying token property names into the subclass, JWTTokenManager.

I can live with the code the way it is, but I think it would be better if tokenInfo and tokenName was pushed down into JWTTokenManager and TokenManager only dealt with accessToken.

Now, I understand your request. Thank you for the more detailed description. I created a modification that would use accessToken. In this commit the change can be reviewed: 064b92f

I was just wondering why it is important to use TokenInfo inside JwtTokenManager? Will there be a feature in the future that will be built upon the content of this object?

Are you referring to the type TokenInfo? I don't know that we have a specific feature in mind for the future so it is not truly important. However, I think it is worth leaving in as at the very least it provides a developer with knowledge of what they can expect the response to look like.

mkistler · 2020-04-29T00:35:33Z

auth/token-managers/token-manager.ts

+  }
+
+  /**
+   * Save the token from the response and the calculated expiration time to the object's state.


We should be explicit. This method must set accessToken (see earlier comment), expireTime, and refreshTime based on the tokenResponse.

Half fixed in e92c3de

Remove all jwt reference from the class and function descriptions. Extend child classes implementation description with `saveTokenInfo` implementation requirement. Add more explicit description to `saveTokenInfo`

dpopp07

I think the change looks good. I did leave a couple stylistic comments to address before merging. I will also wait to merge until Mike approves as well

dpopp07 · 2020-04-29T21:28:17Z

auth/token-managers/jwt-token-manager.ts

-function getCurrentTime(): number {
-  return Math.floor(Date.now() / 1000);
-}
+import {TokenManager, TokenManagerOptions} from "./token-manager";


Style suggestion:

Suggested change

import {TokenManager, TokenManagerOptions} from "./token-manager";

import { TokenManager, TokenManagerOptions } from "./token-manager";

Fixed in fc4e9b6

dpopp07 · 2020-04-29T21:30:09Z

auth/token-managers/token-manager.ts

+import { OutgoingHttpHeaders } from 'http';
+import logger from '../../lib/logger';
+import { RequestWrapper } from '../../lib/request-wrapper';
+import {getCurrentTime} from "../utils";


Style suggestion:

Suggested change

import {getCurrentTime} from "../utils";

import { getCurrentTime } from "../utils";

Fixed in fc4e9b6

dpopp07 · 2020-04-29T22:50:27Z

auth/token-managers/token-manager.ts

+  protected disableSslVerification: boolean;
+  protected headers: OutgoingHttpHeaders;
+  protected requestWrapperInstance: RequestWrapper;
+  protected tokenInfo: any;


Since tokenInfo is used in the logic contained in this file, I'm fine with it existing here.

mkistler

The code as-is is fine but I think it could be marginally better with a slightly different design. I'll leave it to you to decide whether to include that in this PR or leave it for future work.

mkistler · 2020-05-01T02:26:58Z

auth/token-managers/token-manager.ts

+  protected disableSslVerification: boolean;
+  protected headers: OutgoingHttpHeaders;
+  protected requestWrapperInstance: RequestWrapper;
+  protected tokenInfo: any;


Here was my thinking: This class requires its subclasses to implement saveTokenInfo, which is passed the response of the token request. saveTokenInfo has to set some member variables ... specifically expireTime, refreshTime, and tokenInfo.

But what is tokenInfo? It is just an object that must contain a field named tokenName that contains the access token. From the point of view of a generic TokenManager, these are extraneous details. All it needs is the accessToken. So why not simplify the TokenManager class (i.e. make it more general), and simply use accessToken set by saveTokenInfo, and push the complexity of varying token property names into the subclass, JWTTokenManager.

I can live with the code the way it is, but I think it would be better if tokenInfo and tokenName was pushed down into JWTTokenManager and TokenManager only dealt with accessToken.

dpopp07 · 2020-05-01T18:47:23Z

I second @mkistler - I think the suggestion is good but I can live with the current code. @vmatyusGitHub what do you think?

vmatyus · 2020-05-04T12:12:26Z

In my custom authentication case also only 1 access token is in use, so I also think it is a valid request. I am open to implement it.

Store token in accessToken and move varying token property names into subclass JWT. Update tests to use accessToken field.

mkistler

Looks Great! 👍

Thanks for making the accessToken change!

vmatyus · 2020-05-05T08:53:57Z

@mkistler and @dpopp07 Thanks for answering my concerns and reviewing my changes.

vmatyus · 2020-05-05T09:01:15Z

Can I ask to merge this PR?

# [2.4.0](v2.3.2...v2.4.0) (2020-05-05) ### Features * **token-manager:** Introduce a token-manager class for token handling ([#89](#89)) ([23c5f3f](23c5f3f))

ibm-devx-automation · 2020-05-05T14:46:28Z

🎉 This PR is included in version 2.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Bumps [@types/node](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node) from 14.14.16 to 14.14.19. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/node) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

feat(token-manager): introduce a token-manager class

18698d5

token-manager class will be responsible only for the token handling specifically and change some private fields to protected: tokenInfo, expireTime, refreshTime.

feat(helper): Add getCurrentTime function from JwtTokenManager

2269660

mkistler requested review from mkistler and dpopp07 April 28, 2020 22:37

mkistler suggested changes Apr 29, 2020

View reviewed changes

fix(token-manager): Code comments specifications

e92c3de

Remove all jwt reference from the class and function descriptions. Extend child classes implementation description with `saveTokenInfo` implementation requirement. Add more explicit description to `saveTokenInfo`

dpopp07 approved these changes Apr 29, 2020

View reviewed changes

fix(token-manager): Add spaces to import statements

fc4e9b6

vmatyus requested a review from mkistler April 30, 2020 12:11

mkistler approved these changes May 1, 2020

View reviewed changes

feat(token-manager): Introduce accessToken

064b92f

Store token in accessToken and move varying token property names into subclass JWT. Update tests to use accessToken field.

vmatyus force-pushed the master branch from bf0672e to 064b92f Compare May 4, 2020 12:50

mkistler approved these changes May 4, 2020

View reviewed changes

dpopp07 merged commit 23c5f3f into IBM:master May 5, 2020

ibm-devx-automation pushed a commit that referenced this pull request May 5, 2020

chore(release): 2.4.0 [skip ci]

6745315

# [2.4.0](v2.3.2...v2.4.0) (2020-05-05) ### Features * **token-manager:** Introduce a token-manager class for token handling ([#89](#89)) ([23c5f3f](23c5f3f))

ibm-devx-automation added the released label May 5, 2020

bessbd mentioned this pull request May 22, 2020

refactor: 💡 Refactor JWTTokenManager to introduce TokenManager IBM/python-sdk-core#72

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a token-manager class for token handling #89

Introduce a token-manager class for token handling #89

vmatyus commented Apr 28, 2020 •

edited

Loading

CLAassistant commented Apr 28, 2020 •

edited

Loading

mkistler left a comment

mkistler Apr 28, 2020

vmatyus Apr 29, 2020

mkistler Apr 28, 2020

vmatyus Apr 29, 2020

mkistler Apr 28, 2020

vmatyus Apr 29, 2020

mkistler Apr 29, 2020

vmatyus Apr 29, 2020

dpopp07 Apr 29, 2020

mkistler May 1, 2020

vmatyus May 4, 2020 •

edited

Loading

vmatyus May 4, 2020

dpopp07 May 4, 2020

mkistler Apr 29, 2020

vmatyus Apr 29, 2020

dpopp07 left a comment

dpopp07 Apr 29, 2020

vmatyus Apr 30, 2020

dpopp07 Apr 29, 2020

vmatyus Apr 30, 2020

dpopp07 Apr 29, 2020

mkistler left a comment

mkistler May 1, 2020

dpopp07 commented May 1, 2020

vmatyus commented May 4, 2020

mkistler left a comment

vmatyus commented May 5, 2020

vmatyus commented May 5, 2020 •

edited

Loading

ibm-devx-automation commented May 5, 2020

	/** Configuration options for JWT token retrieval. */
	/** Configuration options for token retrieval. */

	const errMsg = '`requestToken` MUST be overridden by a subclass of JwtTokenManagerV1.';
	const errMsg = '`requestToken` MUST be overridden by a subclass of TokenManagerV1.';

	import {TokenManager, TokenManagerOptions} from "./token-manager";
	import { TokenManager, TokenManagerOptions } from "./token-manager";

	import {getCurrentTime} from "../utils";
	import { getCurrentTime } from "../utils";

Introduce a token-manager class for token handling #89

Introduce a token-manager class for token handling #89

Conversation

vmatyus commented Apr 28, 2020 • edited Loading

Checklist

CLAassistant commented Apr 28, 2020 • edited Loading

mkistler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmatyus May 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpopp07 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkistler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpopp07 commented May 1, 2020

vmatyus commented May 4, 2020

mkistler left a comment

Choose a reason for hiding this comment

vmatyus commented May 5, 2020

vmatyus commented May 5, 2020 • edited Loading

ibm-devx-automation commented May 5, 2020

vmatyus commented Apr 28, 2020 •

edited

Loading

CLAassistant commented Apr 28, 2020 •

edited

Loading

vmatyus May 4, 2020 •

edited

Loading

vmatyus commented May 5, 2020 •

edited

Loading