🌱 Adds support for autoPush url param in poll and verify functions #91

gowthamidommety-okta · 2018-02-05T20:06:35Z

gowthamidommety-okta · 2018-02-06T18:47:03Z

lib/tx.js

      if (rememberDevice) {
-        href += '?rememberDevice=true';
+        opts.rememberDevice = rememberDevice;


Should we explicitly set this to true? Same question for autoPush and updatePhone

I think we should, so we don't change the current way this works for rememberDevice. And if we do the same for the other variables, then we will be consistent everywhere, plus they are supposed to be booleans (I am assuming that is also what the API expects)

gowthamidommety-okta · 2018-02-06T20:06:29Z

Need to change description as this is not only for the poll function

mauriciocastillosilva-okta

one comment in one the changes
and one important comment for the design: if I understand correctly, once we call pol({autoPush: true}) then we won't be able to change the value in all the polling requests. So for the UI that we want to change based on the checkbox, we will need to stop the current poll and then call verify again with the new value.

mauriciocastillosilva-okta · 2018-02-06T23:22:15Z

lib/tx.js

      if (rememberDevice) {
-        href += '?rememberDevice=true';
+        opts.rememberDevice = rememberDevice;


I think we should, so we don't change the current way this works for rememberDevice. And if we do the same for the other variables, then we will be consistent everywhere, plus they are supposed to be booleans (I am assuming that is also what the API expects)

gowthamidommety-okta · 2018-02-12T18:33:06Z

lib/tx.js

@@ -7,7 +7,8 @@ var AuthPollStopError = require('./errors/AuthPollStopError');
 var config            = require('./config');

 function addStateToken(res, options) {
-  var builtArgs = util.clone(options) || {};
+  var builtArgs = {};
+  util.extend(builtArgs, options || {});


Changed this to use extend. clone uses JSON.stringify which makes it difficult to use with functions.

This should work without the || {} check

gowthamidommety-okta · 2018-02-12T18:34:24Z

lib/tx.js

-      var href = pollLink.href;
+      var opts = {};
+      if (autoPush !== undefined && autoPush !== null) {
+        opts.autoPush = (typeof autoPush === 'function') ? autoPush() : !!autoPush;


autoPush now takes a boolean / function. It sets the option on every poll to account for the scenario when the checkbox is clicked during an active poll

Interesting. I wonder why this didn't come up for rememberDevice. It'd have the same problem, right?

Does this need to be a function for the simple verify case, or is the poll case sufficient?

Correct, I don't think the rememberDevice value will change if the checkbox is clicked during an active poll. However, we are implementing it this way for autoPush to keep it consistent with the way the autoPush cookie is set on the front end now.
It doesn't need to be a function for the verify case, I just thought it would be better to keep it consistent. What do you think?

Good point Len, it is interesting cause it definitely has the same problem. I was just checking and when you click verify, we send rememberDevice=true, but for polling we do not send that. And if we change the checkbox, then we don't change the value either. I do not know how the backend works for rememberDevice though... maybe we can have a follow up jira for that?

and yes, for verify it doesn't need to be a function, but my vote goes for the consistency.

re: verify - I'm down with consistency
re: rememberDevice - I'm curious if the server maintains the rememberDevice state for us. If it handles the rememberDevice state, it seems natural that it'd also handle the autoPush state. If it doesn't, we can do what this PR does (send it with every request). Is this a question for @srinivasanagandla-okta ?

@kentonsmith-okta would you know the answer to the above ^ ?

Spoke to @kentonsmith-okta about this. The backend seems to cache the rememberDevice value sent on the first verify and then save it on a successful authentication. If the user were to change the value during a poll, it would have no affect. The only way to clear a saved (true) value is to clear the browser's cookies.

lboyette-okta · 2018-02-12T22:07:40Z

lib/tx.js

-      var href = pollLink.href;
+      var opts = {};
+      if (autoPush !== undefined && autoPush !== null) {
+        opts.autoPush = (typeof autoPush === 'function') ? autoPush() : !!autoPush;


Interesting. I wonder why this didn't come up for rememberDevice. It'd have the same problem, right?

lboyette-okta · 2018-02-12T22:09:24Z

test/spec/mfa-challenge.js

+          return test.trans.verify({
+            passCode: '123456',
+            autoPush: function () {
+              return true;


What should happen if autoPush() doesn't return true or false (let's say they accidentally return a Promise)?

Yea, that was something i was thinking about today. It would probably be better to force whatever the function returns to a boolean.

Yeah, let's force a boolean

lboyette-okta · 2018-02-12T22:10:53Z

test/spec/mfa-challenge.js

+          return test.trans.verify({
+            passCode: '123456',
+            autoPush: function () {
+              return true;


It'd be nice to add a test that throws an error inside of autoPush (particularly in the polling case)

lboyette-okta · 2018-02-12T22:14:00Z

lib/tx.js

@@ -7,7 +7,8 @@ var AuthPollStopError = require('./errors/AuthPollStopError');
 var config            = require('./config');

 function addStateToken(res, options) {
-  var builtArgs = util.clone(options) || {};
+  var builtArgs = {};
+  util.extend(builtArgs, options || {});


This should work without the || {} check

lboyette-okta · 2018-02-12T22:16:24Z

lib/tx.js

-      var href = pollLink.href;
+      var opts = {};
+      if (autoPush !== undefined && autoPush !== null) {
+        opts.autoPush = (typeof autoPush === 'function') ? autoPush() : !!autoPush;


Does this need to be a function for the simple verify case, or is the poll case sufficient?

mauriciocastillosilva-okta

just one comment in the test, the rest looks good.

mauriciocastillosilva-okta · 2018-02-14T00:13:54Z

lib/tx.js

-      var href = pollLink.href;
+      var opts = {};
+      if (autoPush !== undefined && autoPush !== null) {
+        opts.autoPush = (typeof autoPush === 'function') ? autoPush() : !!autoPush;


Good point Len, it is interesting cause it definitely has the same problem. I was just checking and when you click verify, we send rememberDevice=true, but for polling we do not send that. And if we change the checkbox, then we don't change the value either. I do not know how the backend works for rememberDevice though... maybe we can have a follow up jira for that?

and yes, for verify it doesn't need to be a function, but my vote goes for the consistency.

mauriciocastillosilva-okta · 2018-02-14T00:20:19Z

test/spec/mfa-challenge.js

+          return test.trans.poll({
+            delay: 0,
+            autoPush: function () {
+              return true;


might be a good idea to return false in the third response just to test that it actually sends something different if the value is changed after the first poll request.

I second this. We should change one of the polling tests to change between requests

lboyette-okta

One test that changes the response between requests, then 🚢

gowthamidommety-okta · 2018-02-15T18:52:00Z

@lboyette-okta @mauriciocastillosilva-okta Please review again. Made changes and also added the test that changes the response between requests.
@lboyette-okta do we need to add documentation for this?

mauriciocastillosilva-okta

looks good ⛵️

lboyette-okta · 2018-02-15T22:03:29Z

@gowthamidommety-okta if we could add something here: https://github.com/okta/okta-auth-js#verifyoptions-1, that'd be awesome

Resolves: OKTA-155565

lboyette-okta · 2018-02-15T23:42:25Z

README.md


-Poll until factorResult is not WAITING. Throws AuthPollStopError if prev or cancel is called.
+  - `autoPush` - Send a push notification immediately the next time `verify` is called on a push factor


Sweet. Change this to Optional parameter to send a push notification immediately the next time verify is called on a push factor to match the others and 🚢

gowthamidommety-okta force-pushed the gd-OKTA-155565-autoPushHttpOnly branch from 84b035b to df2162d Compare February 6, 2018 18:33

gowthamidommety-okta commented Feb 6, 2018

View reviewed changes

gowthamidommety-okta requested review from lboyette-okta and mauriciocastillosilva-okta February 6, 2018 18:47

mauriciocastillosilva-okta reviewed Feb 6, 2018

View reviewed changes

gowthamidommety-okta force-pushed the gd-OKTA-155565-autoPushHttpOnly branch from 1cebed1 to 1a0724f Compare February 12, 2018 02:04

gowthamidommety-okta commented Feb 12, 2018

View reviewed changes

lboyette-okta reviewed Feb 12, 2018

View reviewed changes

gowthamidommety-okta mentioned this pull request Feb 13, 2018

🌱 Support http only cookie for autoPush okta/okta-signin-widget#395

Merged

mauriciocastillosilva-okta reviewed Feb 14, 2018

View reviewed changes

lboyette-okta approved these changes Feb 15, 2018

View reviewed changes

gowthamidommety-okta force-pushed the gd-OKTA-155565-autoPushHttpOnly branch from 5b920d3 to 23fcc94 Compare February 15, 2018 18:45

mauriciocastillosilva-okta approved these changes Feb 15, 2018

View reviewed changes

🌱 Adds support for autoPush url param in poll and verify functions

5d64acd

Resolves: OKTA-155565

lboyette-okta reviewed Feb 15, 2018

View reviewed changes

gowthamidommety-okta force-pushed the gd-OKTA-155565-autoPushHttpOnly branch 2 times, most recently from f1d9765 to 5d64acd Compare February 15, 2018 23:51

gowthamidommety-okta changed the title ~~🌱 Adds support for autoPush url param in poll function~~ 🌱 Adds support for autoPush url param in poll and verify functions Feb 15, 2018

gowthamidommety-okta merged commit 3d70ed9 into master Feb 16, 2018

jmelberg-okta deleted the gd-OKTA-155565-autoPushHttpOnly branch June 15, 2018 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Adds support for autoPush url param in poll and verify functions #91

🌱 Adds support for autoPush url param in poll and verify functions #91

gowthamidommety-okta commented Feb 5, 2018

gowthamidommety-okta Feb 6, 2018

mauriciocastillosilva-okta Feb 6, 2018

gowthamidommety-okta commented Feb 6, 2018

mauriciocastillosilva-okta left a comment

mauriciocastillosilva-okta Feb 6, 2018

gowthamidommety-okta Feb 12, 2018

lboyette-okta Feb 12, 2018

gowthamidommety-okta Feb 12, 2018

lboyette-okta Feb 12, 2018

lboyette-okta Feb 12, 2018

gowthamidommety-okta Feb 12, 2018

mauriciocastillosilva-okta Feb 14, 2018

lboyette-okta Feb 14, 2018

gowthamidommety-okta Feb 15, 2018

gowthamidommety-okta Feb 15, 2018

lboyette-okta Feb 12, 2018

lboyette-okta Feb 12, 2018

gowthamidommety-okta Feb 12, 2018

lboyette-okta Feb 14, 2018

lboyette-okta Feb 12, 2018

lboyette-okta Feb 12, 2018

lboyette-okta Feb 12, 2018

mauriciocastillosilva-okta left a comment

mauriciocastillosilva-okta Feb 14, 2018

mauriciocastillosilva-okta Feb 14, 2018

lboyette-okta Feb 15, 2018

gowthamidommety-okta Feb 15, 2018

lboyette-okta left a comment

gowthamidommety-okta commented Feb 15, 2018 •

edited

Loading

mauriciocastillosilva-okta left a comment

lboyette-okta commented Feb 15, 2018

lboyette-okta Feb 15, 2018


		Poll until factorResult is not WAITING. Throws AuthPollStopError if prev or cancel is called.
		- `autoPush` - Send a push notification immediately the next time `verify` is called on a push factor

🌱 Adds support for autoPush url param in poll and verify functions #91

🌱 Adds support for autoPush url param in poll and verify functions #91

Conversation

gowthamidommety-okta commented Feb 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gowthamidommety-okta commented Feb 6, 2018

mauriciocastillosilva-okta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciocastillosilva-okta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lboyette-okta left a comment

Choose a reason for hiding this comment

gowthamidommety-okta commented Feb 15, 2018 • edited Loading

mauriciocastillosilva-okta left a comment

Choose a reason for hiding this comment

lboyette-okta commented Feb 15, 2018

Choose a reason for hiding this comment

gowthamidommety-okta commented Feb 15, 2018 •

edited

Loading