Add option to manually checking transaction state #27

dafortune · 2017-03-03T04:00:10Z

Also

Serialize enrollment and verification step
Add option to get the step

dafortune · 2017-03-04T01:10:30Z

lib/transaction/index.js

    callback(null, verificationStep);
  });
 };

+transaction.prototype.getAuthVerficationStep = function getAuthVerficationStep() {


This can be easily converted in @nikolaseu proposal of adding the method verifyAuth to the transaction, just call getAuthVerficationStep().verify(...) on that method.

Typo in "getAuthVerficationStep()", missing "i" in verification

dafortune · 2017-03-04T01:14:57Z

lib/transaction/index.js

+  return self.authVerificationStep;
+};
+
+transaction.prototype.getEnrollmentConfirmationStep = function getEnrollmentConfirmationStep() {


This can be easily converted in @nikolaseu proposal of adding the method confirmEnrollment to the transaction, just call getEnrollmentConfirmationStep().confirm(...) on that method.

dafortune · 2017-03-04T01:15:32Z

lib/utils/client_factory.js

@@ -13,6 +14,8 @@ exports.create = function create(options) {

  if (transport === 'polling') {


Need to replace this by @thoper proposed name.

We support both, internally I keep using transport not to spread the changes.

dafortune · 2017-03-04T01:16:50Z

lib/transaction/index.js

+      throw new errors.UnexpectedInputError('Expected data to be an object');
+    }
+
+    if (typeof VALID_METHODS.indexOf(data.method) < 0) {


Add test for invalid method in the serialized transaction

dafortune · 2017-03-04T01:17:08Z

lib/transaction/index.js

+    throw new errors.UnexpectedInputError('Expected data to be an object');
+  }
+
+  if (typeof VALID_METHODS.indexOf(data.method) < 0) {


Add test for invalid method in the serialized transaction

dafortune · 2017-03-04T01:19:34Z

lib/transaction/enrollment_confirmation_step.js

@@ -44,7 +55,10 @@ enrollmentConfirmationStep.prototype.confirm = function confirm(data) {
  };

  var confirmTask = function confirmTask(done) {
-    self.strategy.confirm(data, done);
+    self.strategy.confirm(data, function confirmationDataAccepted(err, result) {
+      acceptedCallback(err, { recoveryCode: self.enrollmentAttempt.getRecoveryCode() });


passing the recovery code here don't quite convince me, but, how are you going to get the recovery code otherwise... making self.enrollmentAttempt.getRecoveryCode() public seems like opening the door too much considering that we are going to change how enrollmentAttempt works.

topherMarie · 2017-03-06T17:36:57Z

lib/transaction/index.js

+  var self = this;
+
+  if (!self.authVerificationStep) {
+    throw new errors.InvalidStateError('cannot get enrollment confirmation ' +


Consistent capitalization -- "Cannot get..."

nikolaseu · 2017-03-06T19:36:14Z

lib/transaction/auth_verification_step.js

@@ -57,14 +67,27 @@ authVerificationStep.prototype.verify = function verify(data) {
      verificationPayload = verificationPayload || {};

      if (err) {
+        if (acceptedCallback) {


can this ever be false? I see this before

acceptedCallback = acceptedCallback || function noop() {};

Good point forgot to remove that

nikolaseu · 2017-03-06T19:37:24Z

lib/transaction/auth_verification_step.js

+      // accepted by the service provider, on the other hand
+      // we still need to wait for (loginOrRejectTask) to trigger the event.
+      // loginOrRejectTask might never be triggered in case of transport=manual
+      acceptedCallback(null, payload);


Seems like this actually needs to be always there, so the previous if is not required ?

nikolaseu · 2017-03-06T19:39:01Z

lib/transaction/index.js

    callback(null, verificationStep);
  });
 };

+transaction.prototype.getAuthVerficationStep = function getAuthVerficationStep() {


Typo in "getAuthVerficationStep()", missing "i" in verification

fiddur

I think it could be good to put the examples in some example-dir and run them in a test (mocking all external calls), if it's not too much work. Broken examples is a really bad thing.

fiddur · 2017-03-07T07:19:26Z

docs/server-side.md

+
+```js
+// WARNING 1: This is an advanced example, before using it consider if the
+// hosted-page option does not match you use case, they are easier to implement


s/you/your/

It is your environment not yours enrollment. Good catch :)

fiddur · 2017-03-07T07:19:45Z

docs/server-side.md

+```js
+// WARNING 1: This is an advanced example, before using it consider if the
+// hosted-page option does not match you use case, they are easier to implement
+// and better suitted for most use cases.


s/suitted/suited/

fiddur · 2017-03-07T07:21:05Z

docs/server-side.md

+// WARNING 2: POST Methods require CSRF protection, don't forget to add them or
+// you will be at risk of CSRF attacks, this example left them out for simplicity
+// reasons and because there are many different ways to add them that
+// are really specific to your enrollment.


I think this warning is superfluous; the default for a browser is to block XHR posts to other domains, so anyone adding a header for allowed origins do so at their own risk.

I'm not sure if the warning is superfluos: cors is not necessary to do csrf, adding a form that triggers itself is enough (assuming there is a body parser that allows it), in this case the attack is quite difficult because you can trigger only one call, but some attackers tend to be smart and I think it is better to close as many doors as posible, even if they are not easy to exploit.

fiddur · 2017-03-07T07:28:14Z

lib/errors/invalid_state_error.js

+}
+
+InvalidStateError.prototype = object.create(GuardianError.prototype);
+InvalidStateError.prototype.contructor = InvalidStateError;


contructor...

fiddur · 2017-03-07T07:28:36Z

lib/errors/unexpected_input_error.js

+}
+
+UnexpectedInputError.prototype = object.create(GuardianError.prototype);
+UnexpectedInputError.prototype.contructor = UnexpectedInputError;


contructor

fiddur · 2017-03-07T07:30:39Z

lib/index.js

@@ -44,7 +53,7 @@ function auth0GuardianJS(options) {
  self.httpClient = object.get(options, 'dependencies.httpClient',
    httpClient(self.serviceUrl, globalTrackingId));

-  self.transport = options.transport || 'socket';
+  self.transport = options.transport || options.state_checking_mechanism || 'socket';


The jsdoc says stateCheckingMechanism; I don't see anything convering it to snake_case...

dafortune · 2017-03-07T13:21:08Z

https://www.pivotaltracker.com/story/show/141217503

dafortune · 2017-03-07T13:25:41Z

#28

joseluisdiaz · 2017-03-07T14:52:22Z

lib/index.js

+  if (transactionTokenObject.isExpired()) {
+    asyncHelpers.setImmediate(callback, new errors.CredentialsExpiredError());
+    return;
+  }


joseluisdiaz · 2017-03-07T14:58:35Z

lib/transaction/index.js

+    ? self.authVerificationStep.serialize() : null;
+  data.enrollmentConfirmationStep = self.enrollmentConfirmationStep
+    ? self.enrollmentConfirmationStep.serialize() : null;
+


move 236, to:

data.enrollmentAttempt: this.enrollmentAttempt ? this.enrollmentAttempt.serialize() : undefined

dafortune added 2 commits March 2, 2017 19:26

Added null client

588d883

Add serialization and behavior changes

a241a1d

dafortune force-pushed the allow-manual-transaction-checking branch from 8063ad0 to a241a1d Compare March 3, 2017 04:02

dafortune added 3 commits March 3, 2017 18:10

Add tests for callbacks

e5bab1a

Add tests for resume and enrollmentCompletion/VerificationStep

b29b3c2

Add tests for serialization

06c4ce7

dafortune commented Mar 4, 2017

View reviewed changes

Fix issue with enrollment

d77033d

topherMarie reviewed Mar 6, 2017

View reviewed changes

dafortune added 2 commits March 6, 2017 16:09

Add emitted error ignored

8fed2bf

Rename stateCheckingMechanism

2e3b535

nikolaseu reviewed Mar 6, 2017

View reviewed changes

dafortune added 3 commits March 6, 2017 18:49

Add docs

e1f0822

Fix noop issue

e649bff

Add tests

8294dc7

fiddur reviewed Mar 7, 2017

View reviewed changes

dafortune added 3 commits March 7, 2017 09:30

Fix casing

0015778

Fix typo

6a3832f

Fix docs typo

2291036

Fix doc typo

0d6d662

joseluisdiaz approved these changes Mar 7, 2017

View reviewed changes

Style fixes

6d9b19e

dafortune merged commit 42e9dcc into auth0:master Mar 7, 2017

dafortune deleted the allow-manual-transaction-checking branch March 7, 2017 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to manually checking transaction state #27

Add option to manually checking transaction state #27

dafortune commented Mar 3, 2017

dafortune Mar 4, 2017 •

edited

Loading

nikolaseu Mar 6, 2017

dafortune Mar 6, 2017

dafortune Mar 4, 2017

dafortune Mar 4, 2017

dafortune Mar 6, 2017

dafortune Mar 4, 2017

dafortune Mar 4, 2017

dafortune Mar 4, 2017

topherMarie Mar 6, 2017

dafortune Mar 6, 2017

nikolaseu Mar 6, 2017

dafortune Mar 6, 2017

dafortune Mar 6, 2017

nikolaseu Mar 6, 2017

nikolaseu Mar 6, 2017

fiddur left a comment

fiddur Mar 7, 2017

dafortune Mar 7, 2017

fiddur Mar 7, 2017

fiddur Mar 7, 2017

dafortune Mar 7, 2017 •

edited

Loading

fiddur Mar 7, 2017

dafortune Mar 7, 2017

fiddur Mar 7, 2017

dafortune Mar 7, 2017

fiddur Mar 7, 2017

dafortune Mar 7, 2017

dafortune commented Mar 7, 2017

dafortune commented Mar 7, 2017

joseluisdiaz Mar 7, 2017

joseluisdiaz Mar 7, 2017

		@@ -13,6 +14,8 @@ exports.create = function create(options) {

		if (transport === 'polling') {

Add option to manually checking transaction state #27

Add option to manually checking transaction state #27

Conversation

dafortune commented Mar 3, 2017

dafortune Mar 4, 2017 • edited Loading

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

fiddur 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

dafortune Mar 7, 2017 • edited Loading

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

dafortune commented Mar 7, 2017

dafortune commented Mar 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dafortune Mar 4, 2017 •

edited

Loading

dafortune Mar 7, 2017 •

edited

Loading