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

PKCE implementation #205

Merged
merged 61 commits into from
Jun 3, 2019
Merged
Show file tree
Hide file tree
Changes from 50 commits
Commits
Show all changes
61 commits
Select commit Hold shift + click to select a range
b0624fa
wip
aarongranick-okta Apr 10, 2019
8a4e4c8
cleanup, fix
aarongranick-okta Apr 10, 2019
c140fd0
cleanup / reduce
aarongranick-okta Apr 11, 2019
839139c
simple test app, performs PKCE flow
aarongranick-okta Apr 11, 2019
23857b6
lint
aarongranick-okta Apr 11, 2019
3901d65
fix tests
aarongranick-okta Apr 11, 2019
2e89506
npm start
aarongranick-okta Apr 11, 2019
1f26bfa
save / load code verifier
aarongranick-okta Apr 11, 2019
8421b1c
polyfill webcrypto, TextEncoder to test computeChallenge
aarongranick-okta Apr 12, 2019
6ee3148
Add options to util:
aarongranick-okta Apr 12, 2019
0f3cc7b
test exchangeFortoken, fix minor bug
aarongranick-okta Apr 12, 2019
aa9ccc7
Merge branch 'master' into pkce
aarongranick-okta Apr 15, 2019
06b8dfa
cleanup, fix tests
aarongranick-okta Apr 15, 2019
7650c24
Merge branch 'master' into pkce
aarongranick-okta Apr 15, 2019
d53cc8c
address review feedback
aarongranick-okta Apr 18, 2019
71dc682
changes needed for fetch to handle token post
aarongranick-okta Apr 18, 2019
5d3b428
Merge branch 'master' into pkce
aarongranick-okta Apr 18, 2019
caf4e4e
validate response types (allow 'code' in array)
aarongranick-okta Apr 19, 2019
6958175
Pkce2 (#210)
aarongranick-okta Apr 24, 2019
3739e47
address review feedback
aarongranick-okta Apr 24, 2019
173e9af
Merge branch 'master' into pkce
aarongranick-okta Apr 24, 2019
5d68ca5
Add karma test for crypto/pkce which needs webcrypto
aarongranick-okta Apr 25, 2019
3e41320
browser tests for complete login flow, implicit & pkce
aarongranick-okta Apr 26, 2019
ec2b368
fix karma test
aarongranick-okta Apr 29, 2019
a6ebafa
update README from review feedback
aarongranick-okta Apr 29, 2019
bbcf753
Adds tests for "validateOptions"
aarongranick-okta Apr 29, 2019
5cc5c98
remove package-lock.json from test/app
aarongranick-okta May 1, 2019
4f46c08
use --prefix (easier to understand)
aarongranick-okta May 1, 2019
a9931f2
do not export pkce interface
aarongranick-okta May 1, 2019
6beae1d
fix test app
aarongranick-okta May 1, 2019
cd5e5da
validate code_challenge_method against well-known configuration
aarongranick-okta May 1, 2019
0746ffc
verify functionality of getWithPopup() for PKCE and implicit
aarongranick-okta May 2, 2019
8a92988
fix tests
aarongranick-okta May 2, 2019
1cbfa75
support PKCE directly in getToken (for popup, frame, etc.)
aarongranick-okta May 3, 2019
b078905
Use crypto to generate super random string for verifier
aarongranick-okta May 6, 2019
9c308b1
remove breaking change: responseMode
aarongranick-okta May 8, 2019
ce798ce
Add tests for renew token
aarongranick-okta May 22, 2019
1c99d55
add test for error/iframe offline_access
aarongranick-okta May 22, 2019
6e3c197
nits
aarongranick-okta May 22, 2019
9ddc284
throw if using undefined storage name key
aarongranick-okta May 23, 2019
82e1ca9
Merge branch 'master' into pkce
aarongranick-okta May 23, 2019
5589c7b
add method to test for PKCE support (+tests for features)
aarongranick-okta May 23, 2019
85e95eb
Add PKCE paragraph to README
aarongranick-okta May 23, 2019
365d129
Merge branch 'pkce' of github.com:okta/okta-auth-js into pkce
aarongranick-okta May 23, 2019
0322b0c
withCredentials for reqwest / jquery httpRequestClients
aarongranick-okta May 23, 2019
e386714
remove features from server test
aarongranick-okta May 23, 2019
dc1adca
Throw error if trying to use PKCE on unsupported browser
aarongranick-okta May 29, 2019
4f11f53
Update README.md
aarongranick-okta May 29, 2019
ce15188
PKCE supported: only throw in constructor
aarongranick-okta May 29, 2019
d384a71
lint nits
aarongranick-okta May 29, 2019
d145903
Accept more query params in test app
aarongranick-okta May 31, 2019
0ecc4fc
nit
aarongranick-okta May 31, 2019
e13491e
small fix for testApp
aarongranick-okta May 31, 2019
1cd3306
set values for scopes, responseType in test
aarongranick-okta May 31, 2019
d8cfe0a
udpate test app readme
aarongranick-okta May 31, 2019
d702a20
disallow "code" responseType
aarongranick-okta May 31, 2019
29c1974
add tests for base64 utils
aarongranick-okta May 31, 2019
b438da1
review feedback
aarongranick-okta May 31, 2019
24deee5
expect responseType=code when grantType=authorization_code
aarongranick-okta Jun 1, 2019
c2f1eb1
nits
aarongranick-okta Jun 3, 2019
be807b7
getToken*: throw if PKCE not supported
aarongranick-okta Jun 3, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .eslintignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@
/buildtools
/dist
/target
/node_modules
node_modules
*.config.js
/lib/config.js
36 changes: 33 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -162,8 +162,10 @@ tokenManager: {
| `issuer` | Specify a custom issuer to perform the OIDC flow. Defaults to the base url parameter if not provided. |
| `clientId` | Client Id pre-registered with Okta for the OIDC authentication flow. |
| `redirectUri` | The url that is redirected to when using `token.getWithRedirect`. This must be pre-registered as part of client registration. If no `redirectUri` is provided, defaults to the current origin. |
| `grantType` | Specify grantType for this Application. Supported types are `implicit` and `authorization_code`. Defaults to `implicit` |
Copy link
Contributor

Choose a reason for hiding this comment

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

style-nit: Adhere to the existing style of surrounding grantType in the description with back-ticks.

| `authorizeUrl` | Specify a custom authorizeUrl to perform the OIDC flow. Defaults to the issuer plus "/v1/authorize". |
| `userinfoUrl` | Specify a custom userinfoUrl. Defaults to the issuer plus "/v1/userinfo". |
| `tokenUrl` | Specify a custom tokenUrl. Defaults to the issuer plus "/v1/token". |
| `ignoreSignature` | ID token signatures are validated by default when `token.getWithoutPrompt`, `token.getWithPopup`, `token.getWithRedirect`, and `token.verify` are called. To disable ID token signature validation for these methods, set this value to `true`. |
| | This option should be used only for browser support and testing purposes. |

Expand Down Expand Up @@ -191,6 +193,28 @@ var config = {
var authClient = new OktaAuth(config);
```

##### PKCE OAuth flow
Copy link
Contributor

Choose a reason for hiding this comment

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

OAuth -> OAuth 2.0

We don't need to replace all other occurrences of this, but we should make sure we're referencing the correct protocol in our headings.


By default the `implicit` OAuth flow will be used. It is widely supported by most browsers. PKCE is a newer flow which is more secure, but does require certain capabilities from the browser.

To use PKCE flow, set `grantType` to `authorization_code` in your config.

```javascript

var config = {
grantType: 'authorization_code',

// other config
issuer: 'https://{yourOktaDomain}/oauth2/default',
};

var authClient = new OktaAuth(config);
```

If the user's browser does not support PKCE, an exception will be thrown. You can test if a browser supports PKCE before construction with this static method:

`OktaAuth.features.isPKCESupported()`

### Optional configuration options

### `httpRequestClient`
Expand All @@ -212,7 +236,8 @@ var config = {
// headers: {
// headerName: headerValue
// },
// data: postBodyData
// data: postBodyData,
// withCredentials: true|false,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Maybe throw a line to explain why they might need withCredentials set to false or true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If they are providing the httpRequest agent, they just need to honor the setting. It is a common flag for ajax libraries.

// }
return Promise.resolve(/* a raw XMLHttpRequest response */);
}
Expand Down Expand Up @@ -1445,7 +1470,12 @@ authClient.token.getWithRedirect(oauthOptions);

#### `token.parseFromUrl(options)`

Parses the access or ID Tokens from the url after a successful authentication redirect. If an ID token is present, it will be [verified and validated](https://github.com/okta/okta-auth-js/blob/master/lib/token.js#L186-L190) before available for use.
Parses the authorization code, access, or ID Tokens from the URL after a successful authentication redirect.

If an authorization code is present, it will be exchanged for token(s) by posting to the `tokenUrl` endpoint.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify - the resolved return value from token.parseFromUrl will be an accessToken and/or idToken. The authorization_code will never be available to the developer, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct


The ID token will be [verified and validated](https://github.com/okta/okta-auth-js/blob/master/lib/token.js#L186-L190) before available for use.
aarongranick-okta marked this conversation as resolved.
Show resolved Hide resolved


```javascript
authClient.token.parseFromUrl()
Expand Down Expand Up @@ -1718,7 +1748,7 @@ yarn install
| Command | Description |
| --------------------- | ------------------------------ |
| `yarn build` | Build the SDK with a sourcemap |
| `yarn test` | Run unit tests using Jest |
| `yarn test` | Run unit tests |
| `yarn lint` | Run eslint linting |

## Contributing
Expand Down
11 changes: 9 additions & 2 deletions fetch/fetchRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,18 @@
var fetch = require('cross-fetch');

function fetchRequest(method, url, args) {
var body = args.data;

// JSON encode body (if appropriate)
if (body && args.headers && args.headers['Content-Type'] === 'application/json' && typeof body !== 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure it will be 'Content-Type' and never 'content-type'? IIRC the spec is not case sensitive.

Copy link
Contributor

Choose a reason for hiding this comment

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

The header can be either, but looking up the key 'Content-Type' in this object is case sensitive. We should probably check 'Content-Type'.toLower() too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Bump ^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can definitely make this change. But in reality we are the only users of this code. We use it as a pluggable utility wrapper around another xhr lib. Anything we do here, we must duplicate that logic in our jqueryRequest and reqwestRequest objects as well. Previously EVERYTHING was JSON encoded. I am creating a small pinhole for the token POST to succeed

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we check these values to be safe? If our dependency on cross-fetch, an application and/or plugin modifies the headers to be lowercase, this check will fail.

body = JSON.stringify(body);
}

var fetchPromise = fetch(url, {
method: method,
headers: args.headers,
body: JSON.stringify(args.data),
credentials: 'include'
body: body,
credentials: args.withCredentials === false ? 'omit' : 'include'
})
.then(function(response) {
var error = !response.ok;
Expand Down
4 changes: 3 additions & 1 deletion jest.server.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ module.exports = {
'./test/spec/oauthUtil.js',
'./test/spec/token.js',
'./test/spec/tokenManager.js',
'./test/spec/webfinger.js'
'./test/spec/webfinger.js',
'./test/spec/pkce.js',
'./test/spec/features.js'
],
'reporters': [
'default',
Expand Down
2 changes: 1 addition & 1 deletion jquery/jqueryRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ function jqueryRequest(method, url, args) {
headers: args.headers,
data: JSON.stringify(args.data),
xhrFields: {
withCredentials: true
withCredentials: args.withCredentials
}
})
.then(function(data, textStatus, jqXHR) {
Expand Down
88 changes: 88 additions & 0 deletions karma.conf.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*!
* Copyright (c) 2019-Present, Okta, Inc. and/or its affiliates. All rights reserved.
* The Okta software accompanied by this notice is provided pursuant to the Apache License, Version 2.0 (the "License.")
*
* You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0.
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
*
* See the License for the specific language governing permissions and limitations under the License.
*/

// Karma configuration file, see link for more information
// http://karma-runner.github.io/3.0/config/configuration-file.html

/* global __dirname */
var path = require('path');
var REPORTS_DIR = path.join(__dirname, 'build2', 'reports', 'karma');

var webpackConf = {
devtool: 'inline-source-map',
resolve: {
alias: {
'@okta/okta-auth-js': path.join(__dirname, 'lib/browser/browserIndex.js')
}
},
module: {
rules: [
{
test: /\.js$/,
use: { loader: 'istanbul-instrumenter-loader' },
enforce: 'post',
include: [
path.resolve(__dirname, 'lib')
]
}
]
}
};

module.exports = function (config) {
config.set({
basePath: '',
frameworks: ['jasmine', 'jquery-3.3.1'],
plugins: [
'karma-jasmine',
'karma-chrome-launcher',
'karma-coverage-istanbul-reporter',
'karma-webpack',
'karma-jquery',
'karma-sourcemap-loader'
],
files: [
{ pattern: './test/karma/main.js', watched: false }
],
preprocessors: {
'test/karma/main.js': ['webpack', 'sourcemap']
},
webpack: webpackConf,
webpackMiddleware: {
stats: 'normal',
},
client:{
Copy link
Contributor

Choose a reason for hiding this comment

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

style-nit: Space after :

// Passing specific test to run
// but this works only with `karma start`, not `karma run`.
test: config.test,
clearContext: false // leave Jasmine Spec Runner output visible in browser
},
coverageIstanbulReporter: {
dir: REPORTS_DIR,
reports: [ 'html', 'lcovonly' ],
fixWebpackSourcePaths: true
},
reporters: ['progress', 'coverage-istanbul'],
port: 9876,
colors: true,
logLevel: config.LOG_INFO,
autoWatch: true,
browsers: ['ChromeHeadlessNoSandbox'],
customLaunchers: {
ChromeHeadlessNoSandbox: {
base: 'ChromeHeadless',
flags: ['--no-sandbox']
}
},
singleRun: false
});
};
10 changes: 10 additions & 0 deletions lib/browser/browser.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,19 @@ function OktaAuthBuilder(args) {
issuer: util.removeTrailingSlash(args.issuer),
authorizeUrl: util.removeTrailingSlash(args.authorizeUrl),
userinfoUrl: util.removeTrailingSlash(args.userinfoUrl),
tokenUrl: util.removeTrailingSlash(args.tokenUrl),
grantType: args.grantType,
redirectUri: args.redirectUri,
httpRequestClient: args.httpRequestClient,
storageUtil: args.storageUtil,
transformErrorXHR: args.transformErrorXHR,
headers: args.headers
};

if (this.options.grantType === 'authorization_code' && !sdk.features.isPKCESupported()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we also need this statement whenever token.getWith* methods are called? Developers should be able to init their authClient with some config but override it when calling getWith*.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A developer CAN override grantType in these calls, but we are not checking for PKCE support. I did have this logic + tests, but after talking with @robertdamphousse-okta we decided to leave it out until requested, as we want to encourage setting grantType up front so that token auto-renew works properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be trivial to bring it back: dc1adca#diff-2c8402d8ca2c5c319eb971f5bdf5ac4dR37

Copy link
Contributor Author

Choose a reason for hiding this comment

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

adding this back in.

throw new AuthSdkError('This browser doesn\'t support PKCE');
}

this.userAgent = 'okta-auth-js-' + config.SDK_VERSION;

// Digital clocks will drift over time, so the server
Expand Down Expand Up @@ -151,6 +157,10 @@ proto.features.isTokenVerifySupported = function() {
return typeof crypto !== 'undefined' && crypto.subtle && typeof Uint8Array !== 'undefined';
};

proto.features.isPKCESupported = function() {
return proto.features.isTokenVerifySupported();
};

// { username, password, (relayState), (context) }
proto.signIn = function (opts) {
var sdk = this;
Expand Down
10 changes: 10 additions & 0 deletions lib/browser/browserStorage.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@ storageUtil.browserHasSessionStorage = function() {
}
};

storageUtil.getPKCEStorage = function() {
aarongranick-okta marked this conversation as resolved.
Show resolved Hide resolved
if (storageUtil.browserHasLocalStorage()) {
return storageBuilder(storageUtil.getLocalStorage(), config.PKCE_STORAGE_NAME);
aarongranick-okta marked this conversation as resolved.
Show resolved Hide resolved
} else if (storageUtil.browserHasSessionStorage()) {
return storageBuilder(storageUtil.getSessionStorage(), config.PKCE_STORAGE_NAME);
} else {
return storageBuilder(storageUtil.getCookieStorage(), config.PKCE_STORAGE_NAME);
}
};

aarongranick-okta marked this conversation as resolved.
Show resolved Hide resolved
storageUtil.getHttpCache = function() {
if (storageUtil.browserHasLocalStorage()) {
return storageBuilder(storageUtil.getLocalStorage(), config.CACHE_STORAGE_NAME);
Expand Down
3 changes: 3 additions & 0 deletions lib/builderUtil.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ function buildOktaAuth(OktaAuthBuilder) {
OktaAuth.prototype = OktaAuthBuilder.prototype;
OktaAuth.prototype.constructor = OktaAuth;

// Hoist feature detection functions to static type
OktaAuth.features = OktaAuthBuilder.prototype.features;
aarongranick-okta marked this conversation as resolved.
Show resolved Hide resolved

return OktaAuth;
};
}
Expand Down
4 changes: 3 additions & 1 deletion lib/http.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ function httpRequest(sdk, options) {
args = options.args,
saveAuthnState = options.saveAuthnState,
accessToken = options.accessToken,
withCredentials = options.withCredentials !== false, // default value is true
storageUtil = sdk.options.storageUtil,
storage = storageUtil.storage,
httpCache = storageUtil.getHttpCache();
Expand All @@ -49,7 +50,8 @@ function httpRequest(sdk, options) {

var ajaxOptions = {
headers: headers,
data: args || undefined
data: args || undefined,
withCredentials: withCredentials
};

var err, res;
Expand Down
11 changes: 9 additions & 2 deletions lib/oauthUtil.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ function getOAuthUrls(sdk, oauthParams, options) {
var authorizeUrl = util.removeTrailingSlash(options.authorizeUrl) || sdk.options.authorizeUrl;
var issuer = util.removeTrailingSlash(options.issuer) || sdk.options.issuer;
var userinfoUrl = util.removeTrailingSlash(options.userinfoUrl) || sdk.options.userinfoUrl;
var tokenUrl = util.removeTrailingSlash(options.tokenUrl) || sdk.options.tokenUrl;

// If an issuer exists but it's not a url, assume it's an authServerId
if (issuer && !(/^https?:/.test(issuer))) {
Expand Down Expand Up @@ -202,7 +203,9 @@ function getOAuthUrls(sdk, oauthParams, options) {
// Shared resource server userinfoUrls look like:
// https://example.okta.com/oauth2/aus8aus76q8iphupD0h7/v1/userinfo
userinfoUrl = userinfoUrl || issuer + '/v1/userinfo';

// Shared resource server tokenUrls look like:
// https://example.okta.com/oauth2/aus8aus76q8iphupD0h7/v1/token
tokenUrl = tokenUrl || issuer + '/v1/token';
// Normally looks like:
// https://example.okta.com
} else {
Expand All @@ -212,12 +215,16 @@ function getOAuthUrls(sdk, oauthParams, options) {
// Normal userinfoUrls look like:
// https://example.okta.com/oauth2/v1/userinfo
userinfoUrl = userinfoUrl || issuer + '/oauth2/v1/userinfo';
// Normal tokenUrls look like:
// https://example.okta.com/oauth2/v1/token
tokenUrl = tokenUrl || issuer + '/oauth2/v1/token';
}

return {
issuer: issuer,
authorizeUrl: authorizeUrl,
userinfoUrl: userinfoUrl
userinfoUrl: userinfoUrl,
tokenUrl: tokenUrl
};
}

Expand Down
Loading