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

PKCE implementation #205

merged 61 commits into from
Jun 3, 2019

Conversation

aarongranick-okta
Copy link
Contributor

@aarongranick-okta aarongranick-okta commented Apr 15, 2019

  • adds unit tests
  • includes a simple app (test/app) which performs the full PKCE flow
  • supports all getToken flows including auto renew
  • adds karma/webpack tests for in browser testing of crypto & login flows

TODO:

  • this has only been tested with the cross-fetch httpRequestClient. There were specific changes involving content type that may need to be duplicated in the other clients (jquery / reqwest). NEEDS to be tested with jquery/reqwest!

reqwest/reqwestRequest.js Outdated Show resolved Hide resolved
lib/http.js Outdated Show resolved Hide resolved
lib/pkce.js Outdated Show resolved Hide resolved
lib/pkce.js Outdated Show resolved Hide resolved
lib/pkce.js Outdated Show resolved Hide resolved
lib/pkce.js Outdated Show resolved Hide resolved
lib/token.js Show resolved Hide resolved
test/app/src/testApp.js Outdated Show resolved Hide resolved
test/app/src/testApp.js Outdated Show resolved Hide resolved
test/app/src/testApp.js Outdated Show resolved Hide resolved
Copy link
Contributor

@jmelberg-okta jmelberg-okta left a comment

Choose a reason for hiding this comment

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

Took a quick first pass - but primarily need to address a few things:

  1. The crypo library used should be revisited. This will need to be approved by our security team before this gets merged.
  2. We need to account for basic validation for the code_verifier. We should have tests that assert it has sufficient entropy and meets the size boundary.

lib/pkce.js Outdated Show resolved Hide resolved
lib/pkce.js Outdated Show resolved Hide resolved
lib/pkce.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
test/app/README.md Outdated Show resolved Hide resolved
test/app/src/testApp.js Outdated Show resolved Hide resolved
lib/token.js Outdated Show resolved Hide resolved
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.

test/app/src/testApp.js Outdated Show resolved Hide resolved
@@ -3,7 +3,7 @@
* anywhere without any dependencies. It packages the SDK
* with reqwest and Q. It also preserves license comments.
*/

/* global __dirname */
Copy link
Contributor

Choose a reason for hiding this comment

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

stale comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

our eslint is setup for client-side. I did not want to set node: true for the whole folder so just a single case here

Copy link
Contributor

@swiftone swiftone left a comment

Choose a reason for hiding this comment

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

A few nits but nothing blocking

Copy link
Contributor

@jmelberg-okta jmelberg-okta left a comment

Choose a reason for hiding this comment

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

Overall looking really good. Few nits and questions.

Only real blocker is whether or not to encourage devs to fallback to implicit versus throwing an error.

README.md Outdated Show resolved Hide resolved
lib/token.js Show resolved Hide resolved
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

lib/builderUtil.js Show resolved Hide resolved
README.md Outdated

var config = {
// use PKCE or implicit flow based on browser support
grantType: OktaAuth.features.isPKCESupported() ? 'authorization_code' : 'implicit',
Copy link
Contributor

Choose a reason for hiding this comment

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

From our conversation yesterday, I thought we didn't want to fall back to implicit if the browser doesn't support crypto.subtle, as PKCE is the safer approach.

Suggestion: Run the OktaAuth.features.isPKCESupported() function before crypto.subtle is called, so we can return an error message if the user is using an unsupported browser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now being tested / will throw in the OktaAuth constructor

test/karma/spec/features.js Show resolved Hide resolved
});


});
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: A few files are missing newlines at the end

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've updated the eslint rules for the karma stuff

.then(function(computed) {
expect(encodeURIComponent(computed)).toBe(computed);
});
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Is eslint configured for the karma specs? We're missing ; in a few places.

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 is, but is using more "modern" settings which doesn't require. I'll add the "semi" rule in

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 also aded the eol rule

test/karma/spec/renewToken.js Show resolved Hide resolved
test/karma/spec/renewToken.js Outdated Show resolved Hide resolved
Copy link
Contributor

@manueltanzi-okta manueltanzi-okta left a comment

Choose a reason for hiding this comment

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

Looks great! Thank for addressing my comments.

Please add the error message for when PKCE is not supported, then 🚀

README.md Outdated Show resolved Hide resolved
@@ -212,7 +234,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.

Copy link
Contributor

@haishengwu-okta haishengwu-okta left a comment

Choose a reason for hiding this comment

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

couple of questions and looks good to me, plus echo Jordan's comments.

lib/http.js Outdated Show resolved Hide resolved
@@ -464,53 +511,81 @@ function getWithoutPrompt(sdk, oauthOptions, options) {
function getWithPopup(sdk, oauthOptions, options) {
var oauthParams = util.clone(oauthOptions) || {};
util.extend(oauthParams, {
display: 'popup'
display: 'popup',
responseMode: 'okta_post_message'
Copy link
Contributor

Choose a reason for hiding this comment

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

why you have to add responseMode: 'okta_post_message'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In an earlier version of this PR, I was changing the default responseMode, so this line was necessary. That breaking change has been removed. Also I have discovered the true reason okta_post_message is the default, is because token.renew() cannot take any parameters, so must use defaults or values set in the constructor.

@aarongranick-okta
Copy link
Contributor Author

@jmelberg-okta Please take another look when you have a chance. I believe I have addressed all feedback / requests. Thanks!

Copy link
Contributor

@jmelberg-okta jmelberg-okta left a comment

Choose a reason for hiding this comment

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

Couple more questions related to older comments. Can you please ensure all comments have been acknowledged and answered?

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.

Bump ^

lib/token.js Outdated Show resolved Hide resolved
test/karma/spec/renewToken.js Show resolved Hide resolved
README.md Outdated
@@ -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.

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.

lib/token.js Outdated
if (typeof responseType === 'string') {
responseType = [responseType];
}
if (responseType.includes('code')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Left a comment in the README, but we need to remove this check. Our documentation mentions that you must have this value when using this grant.

Instead - when we obtain values for the grant_type and response_type, we should assert that if grant_type is authorization_code then response_type is code. If it isn't, we can add that for them as the default value.

Therefore:

  • grant_type=authorization_code && response_type=token: Fails
  • grant_type=authorization_code && response_type=id_token: Fails
  • grant_type=authorization_code && response_type=code: Succeeds
  • grant_type=authorization_code && response_type=undefined: Succeeds as we populate this value for them.

Copy link
Contributor Author

@aarongranick-okta aarongranick-okta Jun 1, 2019

Choose a reason for hiding this comment

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

Yes this definitely makes sense. The refactor is here: 24deee5

Copy link
Contributor

@jmelberg-okta jmelberg-okta left a comment

Choose a reason for hiding this comment

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

Once feedback is addressed - 🚀

My primary ask is since we've refactored a bit of our existing getToken method and token renewal functions, can we please test this against the Widget inside of the monolith? Assuming those tests pass, I think this is safe to release.

// renewed token, we verify the promise key doesn't exist and return.
return;
.then(function(freshTokens) {
// We may receive more tokens than we requested
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is necessarily correct - developers configuring the scope and grant_type should always know what tokens will be returned.

For example, when scope=openid: The response should contain an access and identity token.
When scope=foobar (non-OIDC scope): The response should contain only an access token.

While scope validation can infer which token types will be minted, one option could be to make the change there. However this is probably the right way to handle this.

I'd suggest removing this comment and keeping your logic below.

Copy link
Contributor Author

@aarongranick-okta aarongranick-okta Jun 3, 2019

Choose a reason for hiding this comment

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

What's happening here is the caller is saying something like:

var token = tokenManager.get('accessToken')

This triggers the renew token logic. If we are using authorization code then we will receive all tokens (as defined by the scope). This logic is (trying) to allow all tokens to be refreshed but return only the one requested.

I think, as written, it may not work for token types other than idToken and accessToken (these are the only we have defined in the token manager). - does this library support other token types?

throw new AuthSdkError('A clientId must be specified in the OktaAuth constructor to get a token');
}

if (!oauthOptions.redirectUri) {
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: According to the spec this value should be checked against the original redirect URI:

redirect_uri
         REQUIRED, if the "redirect_uri" parameter was included in the
         authorization request as described in Section 4.1.1, and their
         values MUST be identical.

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 believe the server is doing this check. This validation here is around the API call - it is saying redirectUri is a required parameter to the api (the server will fail the call if it is not present).

return (responseType.indexOf(key) !== -1);
}).forEach(function(key) {
if (!tokenDict[key]) {
throw new AuthSdkError('Unable to parse OAuth flow response: ' + key + ' was not returned.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably OK for now, but ideally we'll concat an error message with both all keys instead of throwing on the first encounter.

README.md Outdated
@@ -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.

README.md Outdated
@@ -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.

@@ -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. The default value is based on the configured `grantType`. If `grantType` is `implicit` (the default setting), `responseType` will have a default value of `id_token`. If `grantType` is `authorization_code`, the default value will be `code`. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Great description! 👍

var inferredKey = freshToken.idToken ? 'idToken' : freshToken.accessToken ? 'accessToken' : key;
map[inferredKey] = freshToken;

var oldToken = get(storage, inferredKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This is a duplicate call. You can use the token variable above instead.

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 you look at the code being replaced, this extra call was there before, I believe it is to prevent an error where the token existed before refresh attempt but was cleared while the request was in flight. In the case that the token may have changed while the request was in flight, I am taking the freshest "old" token

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.

@aarongranick-okta aarongranick-okta merged commit 0a8a4e1 into master Jun 3, 2019
@aarongranick-okta aarongranick-okta deleted the pkce branch June 3, 2019 18:44
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.

7 participants