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.

Sign up for GitHub

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

🌱 Add sendFingerprint option to signIn #82

Merged
merged 3 commits into from
Jan 17, 2018
Merged

🌱 Add sendFingerprint option to signIn #82

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9ec3fca
:seedling: Add sendFingerprint option on signIn
lboyette-okta Jan 11, 2018
a3f0bd6
Address feedback
lboyette-okta Jan 12, 2018
171368a
Address nits
lboyette-okta Jan 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ Read our [contributing guidelines](./CONTRIBUTING.md) if you wish to contribute.
* [unlockAccount](#unlockaccountoptions)
* [verifyRecoveryToken](#verifyrecoverytokenoptions)
* [webfinger](#webfingeroptions)
* [fingerprint] (#fingerprintoptions)
* [fingerprint](#fingerprintoptions)
* [tx.resume](#txresume)
* [tx.exists](#txexists)
* [transaction.status](#transactionstatus)
Expand Down Expand Up @@ -132,6 +132,7 @@ The goal of an authentication flow is to [set an Okta session cookie on the user

- `username` - User’s non-qualified short-name (e.g. dade.murphy) or unique fully-qualified login (e.g dade.murphy@example.com)
- `password` - The password of the user
- `sendFingerprint` - Enabling this will send a `X-Device-Fingerprint` header. Defaults to `false`
Copy link

Choose a reason for hiding this comment

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

it should probably be noted that this is only honored with certain features. We would be able to get a fingerprint without any features but from a usability standpoint, it is only used in conjunction with certain FFs. In fact I think we ignore the header if the flags are not enabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We occasionally include features behind FF (OIDC is an example). As long as sending the header doesn't break anything, I'm not too worried about it.

Copy link

Choose a reason for hiding this comment

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

nit: I believe there is an extra space in - Enabling


```javascript
authClient.signIn({
Expand Down
18 changes: 17 additions & 1 deletion lib/clientBuilder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,23 @@ proto.features.isTokenVerifySupported = function() {

// { username, password, (relayState), (context) }
proto.signIn = function (opts) {
return tx.postToTransaction(this, '/api/v1/authn', opts);
var sdk = this;
var body = opts || {};
return new Q(!body.sendFingerprint ? {} :
Copy link

Choose a reason for hiding this comment

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

would it be better that if we don't have the sendFingerprint option we just do what we were doing before?

if (!body.sendFingerprint) {
  return tx.postToTransaction(this, '/api/v1/authn', opts);
} else {
  return new Q(sdk.fingerprint()
...
}

Copy link
Contributor Author

@lboyette-okta lboyette-okta Jan 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Doing it the way we have it now prevents me from having tx.postToTransaction twice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right though, I think it'd be cleaner your way

sdk.fingerprint()
.then(function(fingerprint) {
return {
Copy link

Choose a reason for hiding this comment

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

another option would be to just add it to opts directly, and that way you don't need to create body nor add it to tx.postToTransacton... just an idea to save a variable, but either works

opts['headers']['X-Device-Fingerprint'] = fingerprint;

Copy link
Contributor Author

@lboyette-okta lboyette-okta Jan 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I could remove the body variable declaration above, but I'll still need to do it below to delete the sendFingerprint property without affecting the opts object they passed in.

var body = util.clone(opts || {});
delete body.sendFingerprint;

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 we're really opposed to the variable declaration, it could be:

opts = util.clone(opts || {});
delete opts.sendFingerprint;

Copy link

Choose a reason for hiding this comment

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

not opposed, was just throwing an idea... but you are right, changing opts directly is not a good practice, much better to just pass the extra argument to http.post and let it deal with merging the options. So all good 👍

headers: {
'X-Device-Fingerprint': fingerprint
}
};
})
)
.then(function(options) {
body = util.clone(body);
delete body.sendFingerprint;
return tx.postToTransaction(sdk, '/api/v1/authn', body, options);
});
};

proto.signOut = function () {
Expand Down
4 changes: 2 additions & 2 deletions lib/tx.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ function transactionExists(sdk) {
return !!sdk.tx.exists._getCookie(config.STATE_TOKEN_COOKIE_NAME);
}

function postToTransaction(sdk, url, options) {
return http.post(sdk, url, options)
function postToTransaction(sdk, url, body, options) {
Copy link

Choose a reason for hiding this comment

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

nit: we could name it args instead of body just for sake of consistency with http.post signature

return http.post(sdk, url, body, options)
.then(function(res) {
return new AuthTransaction(sdk, res);
});
Expand Down
63 changes: 55 additions & 8 deletions test/spec/fingerprint.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
define(function(require) {
var OktaAuth = require('OktaAuth');
var util = require('../util/util');
var packageJson = require('../../package.json');

describe('fingerprint', function() {
function setup(options) {
Expand Down Expand Up @@ -59,17 +60,17 @@ define(function(require) {
});
});

var authClient = new OktaAuth({
var authClient = options.authClient || new OktaAuth({
url: 'http://example.okta.com'
});
if (typeof options.userAgent !== 'undefined') {
util.mockUserAgent(authClient, options.userAgent);
}
return authClient.fingerprint({ timeout: options.timeout });
return authClient;
}

it('iframe is created with the right src and it is hidden', function (done) {
return setup()
return setup().fingerprint()
.catch(function(err) {
expect(err).toBeUndefined();
})
Expand All @@ -88,7 +89,7 @@ define(function(require) {
});

it('allows non-Okta postMessages', function (done) {
return setup({ sendOtherMessage: true })
return setup({ sendOtherMessage: true }).fingerprint()
.catch(function(err) {
expect(err).toBeUndefined();
})
Expand All @@ -101,7 +102,7 @@ define(function(require) {
});

it('fails if the iframe sends invalid message content', function (done) {
return setup({ firstMessage: 'invalidMessageContent' })
return setup({ firstMessage: 'invalidMessageContent' }).fingerprint()
.then(function() {
done.fail('Fingerprint promise should have been rejected');
})
Expand All @@ -112,7 +113,7 @@ define(function(require) {
});

it('fails if user agent is not defined', function (done) {
return setup({ userAgent: '' })
return setup({ userAgent: '' }).fingerprint()
.then(function() {
done.fail('Fingerprint promise should have been rejected');
})
Expand All @@ -125,7 +126,7 @@ define(function(require) {
it('fails if it is called from a Windows phone', function (done) {
return setup({
userAgent: 'Mozilla/5.0 (compatible; MSIE 9.0; Windows Phone OS 7.5; Trident/5.0;)'
})
}).fingerprint()
.then(function() {
done.fail('Fingerprint promise should have been rejected');
})
Expand All @@ -136,7 +137,7 @@ define(function(require) {
});

it('fails after a timeout period', function (done) {
return setup({ timeout: 5 })
return setup({ timeout: true }).fingerprint({ timeout: 5 })
.then(function() {
done.fail('Fingerprint promise should have been rejected');
})
Expand All @@ -145,5 +146,51 @@ define(function(require) {
done();
});
});

util.itMakesCorrectRequestResponse({
title: 'attaches fingerprint to signIn requests if addFingerprint is true',
setup: {
uri: 'http://example.okta.com',
calls: [
{
request: {
method: 'post',
uri: '/api/v1/authn',
data: { username: 'not', password: 'real' },
headers: {
'Accept': 'application/json',
'Content-Type': 'application/json',
'X-Okta-User-Agent-Extended': 'okta-auth-js-' + packageJson.version,
'X-Device-Fingerprint': 'ABCD'
}
},
response: 'success'
}
]
},
execute: function (test) {
return setup({ authClient: test.oa }).signIn({
username: 'not',
password: 'real',
sendFingerprint: true
Copy link

Choose a reason for hiding this comment

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

not sure if we have one test already, but if not maybe we should add one with sendFingerprint: false or sendFingerptint: undefined

Copy link
Contributor Author

@lboyette-okta lboyette-okta Jan 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We have tests for signIn that don't pass sendFingerprint, but none that send false

});
}
});

it('fails signIn request if fingerprinting fails', function(done) {
return setup({ firstMessage: 'invalidMessageContent' })
.signIn({
username: 'not',
password: 'real',
sendFingerprint: true
})
.then(function() {
done.fail('signIn promise should have been rejected');
})
.catch(function(err) {
util.assertAuthSdkError(err, 'Unable to parse iframe response');
done();
});
});
});
});