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.

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

Conversation

lboyette-okta
Copy link
Contributor

@lboyette-okta lboyette-okta commented Jan 11, 2018

Add sendFingerprint option to attach a fingerprint to the signIn call

@lboyette-okta lboyette-okta changed the title 🌱 Add sendFingerprint option on signIn 🌱 Add sendFingerprint option to signIn Jan 11, 2018
README.md Outdated
@@ -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 [email protected])
- `password` - The password of the user
- `sendFingerprint` - Enabling this will send a `X-Device-Fingerprint` header. Defaults to `false`

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.

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

Copy link

Choose a reason for hiding this comment

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

a couple of comments, but in general is looking good

return tx.postToTransaction(this, '/api/v1/authn', opts);
var sdk = this;
var body = opts || {};
return new Q(!body.sendFingerprint ? {} :

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

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

return new Q(!body.sendFingerprint ? {} :
sdk.fingerprint()
.then(function(fingerprint) {
return {

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

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;

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 👍

@dannavarro-okta
Copy link

Copy link

Choose a reason for hiding this comment

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

minor nits, and just one thing left that I didn't see the first time: if we have a test for sendFingerprint: undefined already, then we are good, if not, it might be worth adding one.

README.md Outdated
@@ -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 [email protected])
- `password` - The password of the user
- `sendFingerprint` - Enabling this will send a `X-Device-Fingerprint` header. Defaults to `false`

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

lib/tx.js Outdated
@@ -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) {

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 setup({ authClient: test.oa }).signIn({
username: 'not',
password: 'real',
sendFingerprint: true

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

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

Copy link

Choose a reason for hiding this comment

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

looks good to me ⛵️

@lboyette-okta lboyette-okta merged commit aa82bc4 into master Jan 17, 2018
@jmelberg-okta jmelberg-okta deleted the lb-enable-fingerprint branch June 15, 2018 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants