-
Notifications
You must be signed in to change notification settings - Fork 10
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
Initial implementation #1
Changes from 8 commits
b907192
211c01f
8f25d14
9fb6c05
7a4236b
79ab1ad
af42f0e
2d563b4
952e1c2
35ba205
6598de8
458ec47
3637777
8f8357b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
{ | ||
presets: ["es2015"], | ||
plugins: ["transform-object-rest-spread"], | ||
sourceMaps: true | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
# http://editorconfig.org | ||
root = true | ||
|
||
# All files | ||
[*] | ||
insert_final_newline = true | ||
trim_trailing_whitespace = true | ||
charset = utf-8 | ||
|
||
# JS files | ||
[*.js] | ||
indent_style = space | ||
indent_size = 2 | ||
|
||
# JSON files | ||
[*.json] | ||
indent_style = space | ||
indent_size = 2 | ||
|
||
[*.md] | ||
trim_trailing_whitespace = false |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
test |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
{ | ||
"parser" : "babel-eslint", | ||
"extends": "airbnb", | ||
"parserOptions": { | ||
"ecmaVersion": 6, | ||
"sourceType": "module", | ||
"ecmaFeatures": { | ||
"jsx": false | ||
} | ||
}, | ||
"plugins" : [ | ||
"flow-vars" | ||
], | ||
"env" : { | ||
"browser" : true, | ||
"mocha": true, | ||
}, | ||
"rules": { | ||
"no-empty-label": 0, | ||
"space-before-keywords": 0, | ||
"space-after-keywords": 0, | ||
"space-return-throw-case": 0, | ||
"no-iterator": 0 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
BSD License | ||
|
||
For @shoutem/fetch-token-intercept software | ||
|
||
Copyright (c) 2017-present, Shoutem. All rights reserved. | ||
|
||
Redistribution and use in source and binary forms, with or without | ||
modification, are permitted provided that the following conditions are met: | ||
|
||
* Redistributions of source code must retain the above copyright | ||
notice, this list of conditions and the following disclaimer. | ||
|
||
* Redistributions in binary form must reproduce the above copyright | ||
notice, this list of conditions and the following disclaimer in the | ||
documentation and/or other materials provided with the distribution. | ||
|
||
* Neither the name of the Shoutem nor the | ||
names of its contributors may be used to endorse or promote products | ||
derived from this software without specific prior written permission. | ||
|
||
THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND | ||
ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED | ||
WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE | ||
DISCLAIMED. IN NO EVENT SHALL COPYRIGHT HOLDER BE LIABLE FOR ANY | ||
DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES | ||
(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; | ||
LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND | ||
ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT | ||
(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS | ||
SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
require('babel-register')({ | ||
presets: ['es2015'], | ||
plugins: ['transform-object-rest-spread'], | ||
sourceMaps: 'both', | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
{ | ||
"name": "@shoutem/fetch-token-intercept", | ||
"version": "0.0.1-alpha.1", | ||
"description": "Fetch interceptor for managing refresh token flow.", | ||
"main": "lib/index.js", | ||
"files": [ | ||
"lib" | ||
], | ||
"scripts": { | ||
"lint": "eslint src test", | ||
"test": "mocha --require babelTestSetup --reporter spec --recursive test", | ||
"coverage": "babel-node node_modules/isparta/bin/isparta cover --report text --report html node_modules/mocha/bin/_mocha -- -R spec --recursive test", | ||
"build": "babel src --out-dir lib" | ||
}, | ||
"repository": { | ||
"type": "git", | ||
"url": "" | ||
}, | ||
"keywords": [ | ||
"fetch", | ||
"intercept", | ||
"refresh", | ||
"token", | ||
"access" | ||
], | ||
"author": "Shoutem", | ||
"license": "MIT", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BSD? custom? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note, this should probably be updated on redux io repo too. |
||
"bugs": { | ||
"url": "" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. link to github issues page? |
||
}, | ||
"homepage": "", | ||
"devDependencies": { | ||
"babel-cli": "^6.9.0", | ||
"babel-core": "^6.9.1", | ||
"babel-eslint": "^6.0.0", | ||
"babel-preset-es2015": "^6.9.0", | ||
"babel-preset-stage-0": "^6.3.13", | ||
"babel-register": "^6.9.0", | ||
"chai": "^3.5.0", | ||
"chai-shallow-deep-equal": "^1.4.0", | ||
"deep-freeze": "0.0.1", | ||
"es6-promise": "^4.0.5", | ||
"eslint": "^2.11.1", | ||
"eslint-config-airbnb": "^9.0.1", | ||
"eslint-plugin-flow-vars": "^0.4.0", | ||
"eslint-plugin-import": "^1.8.1", | ||
"eslint-plugin-jsx-a11y": "^1.3.0", | ||
"eslint-plugin-react": "^5.1.1", | ||
"estraverse-fb": "^1.3.1", | ||
"express": "^4.14.1", | ||
"fetch-everywhere": "^1.0.5", | ||
"isparta": "^4.0.0", | ||
"istanbul": "0.4.4", | ||
"mocha": "^2.5.3", | ||
"nock": "^8.0.0", | ||
"sinon": "^1.17.4" | ||
}, | ||
"dependencies": {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,111 @@ | ||
import { | ||
isResponseUnauthorized, | ||
} from './services/http'; | ||
|
||
export class AccessTokenProvider { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add nice jsdoc comment /** about this class, explaining it to others There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Export default |
||
constructor(fetch, config) { | ||
this.fetch = fetch; | ||
|
||
this.config = config; | ||
this.refreshAccessTokenPromise = null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
this.tokens = { | ||
refreshToken: null, | ||
accessToken: null, | ||
}; | ||
|
||
this.isAuthorized = this.isAuthorized.bind(this); | ||
this.refresh = this.refresh.bind(this); | ||
this.clear = this.clear.bind(this); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You divided public from private? Be free to add that comment. Maybe we should use some convention. |
||
|
||
this.resolveAccessToken = this.resolveAccessToken.bind(this); | ||
this.fetchToken = this.fetchToken.bind(this); | ||
this.handleFetchResolved = this.handleFetchResolved.bind(this); | ||
this.handleTokenResolved = this.handleTokenResolved.bind(this); | ||
this.handleError = this.handleError.bind(this); | ||
} | ||
|
||
/** | ||
* Refreshes current access token with provided refresh token | ||
*/ | ||
refresh() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
// if token resolver is not authorized it should just resolve | ||
if (!this.isAuthorized()) { | ||
return Promise.resolve(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should it reject? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it rejects it will cause a chain reaction which will cause fetchIntercept chain to reject also, which in turn will reject an initial fetch request which is undesirable as we don't want to break the fetch spec which currently states that fetch will only reject on network errors and this is not a network error. But indeed it could be potentially problematic if you call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking from outside, I would want for my requests to fail with 40x error with equal behavior as in case that I don't use fetch-token-intercept and didn't add proper Auth header. Also it would be useful to see additional error or warning from fetch-token-intercept about need to call |
||
} | ||
|
||
// if we are not running token promise, start it | ||
if (!this.refreshAccessTokenPromise) { | ||
this.refreshAccessTokenPromise = new Promise(this.resolveAccessToken); | ||
} | ||
|
||
// otherwise just return existing promise | ||
return this.refreshAccessTokenPromise; | ||
} | ||
|
||
/** | ||
* Authorizes intercept library with given refresh token | ||
* @param refreshToken | ||
* @param accessToken | ||
*/ | ||
authorize(refreshToken, accessToken) { | ||
this.tokens = { ...this.tokens, refreshToken, accessToken }; | ||
} | ||
|
||
/** | ||
* Returns current authorization for fetch fetchInterceptor | ||
* @returns {{accessToken: string, refreshToken: string}} | ||
*/ | ||
getAuthorization() { | ||
return this.tokens; | ||
} | ||
|
||
clear() { | ||
this.tokens.accessToken = null; | ||
this.tokens.refreshToken = null; | ||
} | ||
|
||
isAuthorized() { | ||
return this.tokens.refreshToken !== null; | ||
} | ||
|
||
fetchToken(tokenRequest) { | ||
const { fetch } = this; | ||
return fetch(tokenRequest); | ||
} | ||
|
||
handleFetchResolved(response) { | ||
this.refreshAccessTokenPromise = null; | ||
|
||
if (isResponseUnauthorized(response)) { | ||
this.clear(); | ||
return null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should you throw an exception? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same reasoning as on the comment above. This is not exceptional behavior because it's not network related, we've successfully fetched a response, and it's status causes provider to reset itself. Promises waiting for it to resolve will carry on without a new token and fail with 401 which is expected. The gist here is 'try to reauthorize to get new token, if it fails, it should fail the same manner as without fetch interceptor'. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, we could throw, but fetchIntercept would have to handle it and resolve fetch requests normally. We can discuss this live. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with your gist. I wasn't sure, so I rather ask to see what is your reasoning. Thanks. |
||
} | ||
|
||
return this.config.parseAccessToken(response); | ||
} | ||
|
||
handleTokenResolved(token, resolve) { | ||
this.tokens.accessToken = token; | ||
|
||
if (this.config.onAccessTokenChange) { | ||
this.config.onAccessTokenChange(token); | ||
} | ||
|
||
resolve(token); | ||
} | ||
|
||
handleError(error, reject) { | ||
this.refreshAccessTokenPromise = null; | ||
this.clear(); | ||
|
||
reject(error); | ||
} | ||
|
||
resolveAccessToken(resolve, reject) { | ||
return Promise.resolve(this.config.createAccessTokenRequest(this.tokens.refreshToken)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. move this nesting above
|
||
.then(this.fetchToken) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fetchAccessToken |
||
.then(this.handleFetchResolved) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handleFetchAccessTokenResponse |
||
.then(token => this.handleTokenResolved(token, resolve)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handleAccessToken |
||
.catch(error => this.handleError(error, reject)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove line |
||
export const ERROR_INVALID_CONFIG = 'invalid-config'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
import { | ||
isReactNative, | ||
isWorker, | ||
isWeb, | ||
isNode, | ||
} from './services/environment'; | ||
import { | ||
FetchInterceptor, | ||
} from './fetchInterceptor'; | ||
|
||
const interceptors = []; | ||
|
||
function init() { | ||
if (isReactNative()) { | ||
attach(global); | ||
} else if (isWorker()) { | ||
attach(self); | ||
} else if (isWeb()) { | ||
attach(window); | ||
} else if (isNode()) { | ||
attach(global); | ||
} else { | ||
throw new Error('Unsupported environment for fetch-token-intercept'); | ||
} | ||
} | ||
|
||
function attach(env) { | ||
if (!env.fetch) { | ||
throw Error('No fetch available. Unable to register fetch-token-intercept'); | ||
} | ||
|
||
// for now add default interceptor | ||
interceptors.push(new FetchInterceptor(env.fetch)); | ||
|
||
// monkey patch fetch | ||
const fetchWrapper = fetch => (...args) => interceptors[0].intercept(...args); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a bit confusing to work with array[0]. What is idea behind multiple interceptors? Are they all going to intercept in parallel or in series requests? I would suggest that if you have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Below functions should also change, but we should first decide about interface for adding multiple interceptors. As I see for know we will have only one class There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we discussed, we're at a point that this could be implemented easily when needed, and we don't really need it. I believe it goes to 'nice to have' list when we settle on implementation. |
||
env.fetch = fetchWrapper(env.fetch); | ||
} | ||
|
||
function configure(config) { | ||
interceptors[0].configure(config); | ||
} | ||
|
||
function authorize(...args) { | ||
interceptors[0].authorize(...args); | ||
} | ||
|
||
function getAuthorization() { | ||
return interceptors[0].getAuthorization(); | ||
} | ||
|
||
function clear() { | ||
return interceptors[0].clear(); | ||
} | ||
|
||
export { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should export direct on function signature. |
||
init, | ||
clear, | ||
configure, | ||
authorize, | ||
getAuthorization, | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this file could be called index.js. Currently, index.js is just re-exporting it. Ok, it should also call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't ahve README file, add it. I know you don't have time to write everything. But maybe you could just add short description about what this lib solves and add link to somethnig like this:
https://auth0.com/blog/refresh-tokens-what-are-they-and-when-to-use-them/