Refactor authentication to use Okta authorization server by robertjd · Pull Request #608 · stormpath/express-stormpath

robertjd · 2017-03-09T17:48:14Z

Authentication now happens against an Okta Authentication Server

Token validation is done with the Okta access token, and the application users API.

Logout uses revoke on the Authorization Server.

Begin writing the changelog for this next major version.

Tests will be failing in this PR, as I have not yet addressed the functional differences, or the change to ES6 classes.

This branch still requires you to provide Stormpath configuration somewhere, as I haven't yet removed those dependencies.

Todos:

Example configuration block in changelog should not use literal values, use example values e.g. https://dev-<my org id>.oktapreview.com
Remove ES6 code, we've decided not to force a Node upgrade during the transition.
The authorization server configuration can be discovered through the API, so we should do this so that the developer only needs to specify their API token and app ID.
More help around creating an application and authorization server. I want to create a screencast where I walk through this, as it's a lot of moving parts.
New authentication result from Okta oauth endpoint needs to match OAuthPasswordGrantAuthenticationResult as much as posssible
Normalize base URL trailing slash
Attempt to get some of the IT tests running

Authentication now happens against an Okta Authentication Server Token validation is done with the Okta access token, and the application users API. Logout uses revoke on the Authorization Server. Begin writing the changelog for this next major version.

mraible · 2017-03-09T19:28:24Z

What do I need to change in this file to make this work with Okta?

https://github.com/stormpath/stormpath-angular2-express-example/blob/master/server/server.js

Related: how do I setup my Okta instance to work with this?

robertjd · 2017-03-09T19:46:45Z

Thanks @mraible , great questions. Take a look at the mods to the changelog? If it's not clear let me know and I'll get more detailed. As we go through this migration I want to use the changelog to collect all the notes.

rchild-okta · 2017-03-09T18:32:57Z

+
+- **API Token**: similar to the Stormpath API Key, this is a secret that is used
+  to secure the communication with the Okta platorm.
+- **Application Id**: This is the Okta Application that is connected to yuor


rchild-okta · 2017-03-09T23:24:43Z

+.. code-block:: javascript
+  app.use(stormpath.init(app, {
+    okta: {
+      org: 'https://dev-613050.oktapreview.com/',


nit: Looks like below we're expecting this to not have the / at the end

rchild-okta · 2017-03-09T23:33:51Z

          return next();
        }
-
-        if (account.status !== 'ENABLED') {


Double checking - in Okta, if a user is not active, it'll return an error right (i.e. so we don't need to keep a check like this)?

Right. This is a strange check, I'm not sure why it was here in the first place. In this context the authentication attempt has already happened, which would have failed if the user were not enabled.

rchild-okta · 2017-03-09T23:35:56Z

+  }
+
+  withClientId(clientId) {
+    this.clientId = clientId;


Is this supposed to also be a fluid function (i.e. return this)?

rchild-okta · 2017-03-09T23:40:58Z

+    });
+  }
+
+  jwksResolver(kid, callback) {


Am curious - does njwt cache the keys?

rchild-okta · 2017-03-09T23:42:28Z

+        return callback('Keys collection not found');
+      }
+
+      const matches = res.keys.filter((key) => key.kid === kid);


nit: I like find here instead of filter (will return immediately when it gets the match)

rchild-okta · 2017-03-09T23:44:49Z

+
+function getAppUserById(org, appId, apiToken, uid, callback) {
+  const req = {
+    url: org + '/api/v1/apps/' + appId + '/users/' + uid,


Heh, as an aside, when moving to es6 it's really handy to use template literals for things like this, i.e.

url: `${org}/api/v1/apps/${appId}/users/${uid}`

rchild-okta · 2017-03-09T23:47:20Z

+
+function passwordGrant(config, userFormSubmission, callback) {
+  var req = {
+    url: config.okta.org + 'oauth2/' + config.okta.authorizationServerId + '/v1/token',


Although if we change the example to not show a trailing '/', then this would need to change. I'm okay with either way, but think we should be consistent (or when processing the config, normalize it).

Let's normalize it, I'll add to this PR.

rchild-okta · 2017-03-09T23:51:49Z

+Todo tasks (discovered while implemented Primary goals):
+
+[ ] ensure that the migrated AS configuration will have the right settings for access token timeouts
+[ ] caching of jwks (can use HTTP response from .well-known)


Ah, I see, nm about the caching question

rchild-okta · 2017-03-09T23:55:24Z

          }

-          callback(null, expandedAccount, authResult);
+          callback(null, user, oauthAccessTokenResult);


Just double checking - are all the objects returned to the user of the same format? I.e. in this case is oauthAccessTokenResult the same as authResult in shape (is this even publicly exposed)?

Ah, good catch, thanks! I do need to make this match the OAuthPasswordGrantAuthenticationResult as much as possible.

This is now relying on the `okta` branch in the node SDK and stormpath-config I have decided to attempt using this libraries w/ mods, as they already contain these features that will be helpful to not re-write: - configuration parsing - caching - request executor - some getter methods which will still be useful

coveralls · 2017-03-14T00:24:30Z

Coverage decreased (-53.6%) to 13.643% when pulling 3e8953c on okta-authentication into ecd114b on 4.0.0.

- No longer fetching the app user, using the user resource itself - Login tests should now be passing - Tests are now expecting some static Okta fixture data, passed through the environment. This may change as we bring more tests back online

rorheden added the in progress label Mar 9, 2017

rchild-okta reviewed Mar 9, 2017

View reviewed changes

robertdamphousse-okta added 2 commits March 10, 2017 16:29

Removing ES6 syntax, linter cleanup

69c215b

robertdamphousse-okta added 6 commits March 14, 2017 11:51

Updated changelog examples, depend on okta development branches

e87822b

implement refresh_token cookie

42beadd

update okta-todo.md

c2b3c20

push transformer down to SDK, use SDK for auth result and getAccount()

5a58971

updated changelog

190d8b9

robertjd merged commit aed13fa into 4.0.0 Mar 17, 2017

robertjd deleted the okta-authentication branch March 17, 2017 00:34

rorheden removed the in progress label Mar 17, 2017

robertjd restored the okta-authentication branch March 17, 2017 00:42

Conversation

robertjd commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mraible commented Mar 9, 2017

Uh oh!

robertjd commented Mar 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robertjd Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Mar 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

robertjd commented Mar 9, 2017 •

edited

Loading

robertjd Mar 13, 2017 •

edited

Loading

coveralls commented Mar 14, 2017 •

edited

Loading