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

feat: Added support for Authenticated Datafiles #498

Merged
merged 8 commits into from
Jun 4, 2020

Conversation

zashraf1985
Copy link
Contributor

Summary

Added support for Authenticated Datafiles

Test plan

Will Add Unit Tests.

@zashraf1985 zashraf1985 requested a review from a team as a code owner June 2, 2020 01:38
@zashraf1985 zashraf1985 removed their assignment Jun 2, 2020
@coveralls
Copy link

coveralls commented Jun 2, 2020

Coverage Status

Coverage remained the same at 96.7% when pulling a1dd322 on zeeshan/datafile-auth into cc6a6b6 on master.

Copy link
Contributor

@mjc1283 mjc1283 left a comment

Choose a reason for hiding this comment

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

LGTM. Small comments. Please add tests.

@@ -20,6 +20,8 @@ export const MIN_UPDATE_INTERVAL = 1000;

export const DEFAULT_URL_TEMPLATE = `https://cdn.optimizely.com/datafiles/%s.json`;

export const DEFAULT_AUTHENTICATED_DATAFILE_URL_TEMPLATE = `https://www.optimizely-cdn.com/datafiles/auth/%s.json`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest DEFAULT_AUTHENTICATED_URL_TEMPLATE - everything pertains to datafiles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -43,4 +43,5 @@ export interface DatafileManagerConfig {
updateInterval?: number;
urlTemplate?: string;
cache?: PersistentKeyValueCache;
authDatafileToken?: string
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to isolate this functionality to Node, this shouldn't be added here. DatafileManagerConfig is used by the browser datafile manager. We should create a new interface accepted only by the Node datafile manager.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

protected makeGetRequest(reqUrl: string, headers: Headers): AbortableRequest {
if (this.authToken) {
headers['Authorization'] = `Bearer ${this.authToken}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Let's not mutate the headers object, rather make a shallow copy and add this header.
  • Let's log at debug level, of course NOT included the secret in the log message, but the message should indicate that we added an Authorization: Bearer header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@zashraf1985 zashraf1985 removed their assignment Jun 2, 2020
@@ -44,3 +44,7 @@ export interface DatafileManagerConfig {
urlTemplate?: string;
cache?: PersistentKeyValueCache;
}

export interface NodeDatafileManagerConfig extends DatafileManagerConfig {

Choose a reason for hiding this comment

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

not sure if NodeDatafileManagerConfig is the right name for this.
Also, not sure if it would be easier to make changes in DatafileManagerConfig and HttpPollingDatafileManager directly instead of extending them. It looks clean, but for instance, it does not make sense to do something like that in go-sdk, so maybe we can have language specific implementation.

I would like to have @mjc1283 input on that

Copy link
Contributor

Choose a reason for hiding this comment

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

@pawels-optimizely It is the right name. This class gets pulled in via a specific entry point which is intended for use in Node. We already had these browser/node-specific subclasses as a way of implementing the datafile GET request differently in each environment. This is another behavior that's different for browser vs. Node, so I think it's appropriate. I'm not strongly opposed to putting this in HttpPollingDatafileManager, and just advising against using this in client-side. But since we already have these classes in place, I don't mind having it here either.

Copy link
Contributor

@mjc1283 mjc1283 left a comment

Choose a reason for hiding this comment

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

Found a typo in the CHANGELOG entry

@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
## [Unreleased]
Changes that have landed but are not yet released.

### Changed
- Added support for authenticated datafiles. `NodeDatafileManager` now accepts `datafileAccessToken` to be able to fetch authentication datafiles.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Added support for authenticated datafiles. `NodeDatafileManager` now accepts `datafileAccessToken` to be able to fetch authentication datafiles.
- Added support for authenticated datafiles. `NodeDatafileManager` now accepts `datafileAccessToken` to be able to fetch authenticated datafiles.

@mjc1283 mjc1283 merged commit f6b3a93 into master Jun 4, 2020
@mjc1283 mjc1283 deleted the zeeshan/datafile-auth branch June 4, 2020 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants