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 58 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
44 changes: 40 additions & 4 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 @@ -1367,14 +1392,15 @@ The following configuration options can **only** be included in `token.getWithou
| :-------: | ----------|
| `sessionToken` | Specify an Okta sessionToken to skip reauthentication when the user already authenticated using the Authentication Flow. |
| `responseMode` | Specify how the authorization response should be returned. You will generally not need to set this unless you want to override the default values for `token.getWithRedirect`. See [Parameter Details](https://developer.okta.com/docs/api/resources/oidc#parameter-details) for a list of available modes. |
| `responseType` | Specify the [response type](https://developer.okta.com/docs/api/resources/oidc#request-parameters) for OIDC authentication. Defaults to `id_token`. |
| `responseType` | Specify the [response type](https://developer.okta.com/docs/api/resources/oidc#request-parameters) for OIDC authentication. Defaults to `id_token`. Supported values are `id_token` and `token`. |
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to support code. To kick off the PKCE flow the developer needs to pass in the grant type of authorization_code and the response type is required to be code. The only thing we need to restrict is the combination of response types when using PKCE.

Suggestion:

Supported values are id_token, token, and code. Hybrid flows (code token, code id_token) are not supported.

| | Use an array if specifying multiple response types - in this case, the response will contain both an ID Token and an Access Token. `responseType: ['id_token', 'token']` |
| `scopes` | Specify what information to make available in the returned `id_token` or `access_token`. For OIDC, you must include `openid` as one of the scopes. Defaults to `['openid', 'email']`. For a list of available scopes, see [Scopes and Claims](https://developer.okta.com/docs/api/resources/oidc#access-token-scopes-and-claims). |
| `state` | Specify a state that will be validated in an OAuth response. This is usually only provided during redirect flows to obtain an authorization code. Defaults to a random string. |
| `nonce` | Specify a nonce that will be validated in an `id_token`. This is usually only provided during redirect flows to obtain an authorization code that will be exchanged for an `id_token`. Defaults to a random string. |

For a list of all available parameters that can be passed to the `/authorize` endpoint, see Okta's [Authorize Request API](https://developer.okta.com/docs/api/resources/oidc#request-parameters).


##### Example

```javascript
Expand Down Expand Up @@ -1445,7 +1471,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,8 +1749,13 @@ 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 |
| `yarn start` | Start internal test app |

#### Test App

Implements a simple SPA application to demonstrate functionality and provide for manual testing. [See here for more information](test/app/README.md).

## Contributing

Expand Down
14 changes: 12 additions & 2 deletions fetch/fetchRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,22 @@

var fetch = require('cross-fetch');

/* eslint-disable complexity */
function fetchRequest(method, url, args) {
var body = args.data;
var headers = args.headers || {};
var contentType = (headers['Content-Type'] || headers['content-type'] || '').toLowerCase();
Copy link
Contributor

@jmelberg-okta jmelberg-okta May 31, 2019

Choose a reason for hiding this comment

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

The extra toLowerCase() isn't needed here. We only need to assert that the dictionary key is case sensitive.


// JSON encode body (if appropriate)
if (contentType === 'application/json' && body && typeof body !== 'string') {
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
3 changes: 2 additions & 1 deletion jquery/jqueryRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,15 @@
var $ = require('jquery');

function jqueryRequest(method, url, args) {
// TODO: support content-type
var deferred = $.Deferred();
$.ajax({
type: method,
url: url,
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: {
// 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