🐛 Fix concurrent fingerprinting #463
Conversation
src/util/DeviceFingerprint.js
Outdated
|
|
||
| define(['vendor/lib/q', 'okta/jquery'], function (Q, $) { | ||
|
|
||
| return { | ||
| _isMessageFromCorrentIframe: function($iframe, event) { |
There was a problem hiding this comment.
nit: Did you meanCurrent instead of Corrent? Either way, what if we just generalize the method name to _isMessageFromCorrectSource
| @@ -23,6 +26,7 @@ define(['vendor/lib/q', 'okta/jquery'], function (Q, $) { | |||
| } | |||
|
|
|||
| var deferred = Q.defer(); | |||
| var self = this; | |||
There was a problem hiding this comment.
why do you need to do this? Can't you just call this.METHOD or is this a weird js thing?
There was a problem hiding this comment.
weird js thing :) This inside the messageReceived has a different context.
| @@ -25,6 +26,14 @@ function ($, Q, Expect, $sandbox, DeviceFingerprint) { | |||
| navigator.__defineGetter__('userAgent', function () { return userAgent; }); | |||
| } | |||
|
|
|||
| function byPassIframeCheck() { | |||
There was a problem hiding this comment.
nit: bypassMessageSourceCheck. Bypass is one word
| setTimeout(function() { | ||
| // give it time to check if promise resolves or rejects. | ||
| done(); | ||
| }, 1000); |
There was a problem hiding this comment.
does this mean wait for 1 second before succeeding? Is that a guaranteed time for the promise to return? Seems strange to have a wait in a unit test
There was a problem hiding this comment.
no we dont actually wait 1sec. We fake waiting for 1 sec using the mock Util.
src/util/DeviceFingerprint.js
Outdated
|
|
||
| define(['vendor/lib/q', 'okta/jquery'], function (Q, $) { | ||
|
|
||
| return { | ||
| _isMessageFromCorrectSource: function($iframe, event) { |
There was a problem hiding this comment.
we don't really need to return this function right? We can move it above the return statement...
Oh, or are you doing this just for the tests? that is also why you actually added this function instead of just adding the logic inside onMessageReceivedFromOkta right?
There was a problem hiding this comment.
yeah I introduced this to test it. Also I used _ to differentiate it.
magizh-okta
force-pushed
the
mt-okta-172448
branch
2 times, most recently
from
d8a2168
to
95fc5ba
Compare
| navigator.__proto__ || (navigator.__proto__ = _navigator); | ||
| navigator.__defineGetter__('userAgent', function () { return userAgent; }); | ||
| function mockUserAgentCheck(userAgent) { | ||
| spyOn(DeviceFingerprint, '__getUserAgent').and.callFake(function() { |
There was a problem hiding this comment.
@mauriciocastillosilva-okta The prev impl of this was wrong. The reset in afterEach dint happen correctly. Seems like it is not good to modify window.navigator for test and hence moved getting userAgent to a separate function to mock it correctly.
There was a problem hiding this comment.
Glad you find this issue!
| navigator = new Object(); // eslint-disable-line no-global-assign | ||
| navigator.__proto__ || (navigator.__proto__ = _navigator); | ||
| navigator.__defineGetter__('userAgent', function () { return userAgent; }); | ||
| function mockUserAgentCheck(userAgent) { |
There was a problem hiding this comment.
I think we can leave this as mockUserAgent since it is still doing that for the test purposes
| done.fail('Fingerprint promise should not have been rejected. ' + reason); | ||
| }); | ||
| Util.mockSetTimeout(); | ||
| setTimeout(function() { |
There was a problem hiding this comment.
can we use a tick() here instead?
src/util/DeviceFingerprint.js
Outdated
| __getUserAgent: function() { | ||
| return navigator.userAgent; | ||
| }, | ||
| _isMessageFromCorrectSource: function($iframe, event) { |
There was a problem hiding this comment.
either add this one with __ as a prefix or change __getUserAgent to have only one _. I vote for 2 underscores...
Although, now that I think about this, we don't really have a pattern here in the widget repo for naming "internal" or "private" functions. so I will probably just leave the names without any prefix. Plus they are not actually "private", since we are also returning them.
| navigator.__proto__ || (navigator.__proto__ = _navigator); | ||
| navigator.__defineGetter__('userAgent', function () { return userAgent; }); | ||
| function mockUserAgentCheck(userAgent) { | ||
| spyOn(DeviceFingerprint, '__getUserAgent').and.callFake(function() { |
There was a problem hiding this comment.
Glad you find this issue!
The signin widget runs the device fingerprinting code in an iframe and proceeds when the iframe posts back to the parent an event with type `FingerprintAvailable`. The problem is that the widget assumes that one one iframe will exist at a time and therefore only one `FingerprintAvailable` event will occur. This is a bad assumption. It is possible for multiple requests to request fingerprints at the same time but all requests will listen for the same `FingerprintAvailable` event regardless of which iframe generated it. This means that all concurrent fingerprint requests will generate unique fingerprints but all will use the first one that is returned. Because the fingerprints have one time tokens, this will cause failures Checking iframe contentWindow with event sources makes sure that we only process messages received from the right iframe.
magizh-okta
force-pushed
the
mt-okta-172448
branch
from
21bebed
to
c59406b
Compare
The signin widget runs the device fingerprinting code in an iframe and proceeds when the iframe posts back to the parent an event with type
FingerprintAvailable. The problem is that the widget assumes that one one iframe will exist at a time and therefore only oneFingerprintAvailableevent will occur. This is a bad assumption. It is possible for multiple requests to request fingerprints at the same time but all requests will listen for the sameFingerprintAvailableevent regardless of which iframe generated it. This means that all concurrent fingerprint requests will generate unique fingerprints but all will use the first one that is returned. Because the fingerprints have one time tokens, this will cause failuresChecking iframe contentWindow with event sources makes sure that we only process messages received from the right iframe.