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 fxa uid and device_id to tokens #81

Merged
merged 1 commit into from Feb 25, 2016

Conversation

dannycoates
Copy link
Contributor

This change adds fxa_uid and device_id to the generated token as well as logging them for metrics.

For device_id it relies on the new fxa-deviceId claim from FxA coming in v1.57. When not present it will be a hash of fxa_uid + 'none'.

@rfk
Copy link
Contributor

rfk commented Feb 24, 2016

Is it odd that one has an "fxa" prefix and the other doesn't..? shrug

email = request.validated['assertion']['email']
id_key = request.registry.settings.get("fxa.metrics_uid_secret_key")
if id_key is None:
id_key = 'insecure'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should at least log a warning here if the key is not configured.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better to log once at startup than every request, yeah?

@rfk
Copy link
Contributor

rfk commented Feb 24, 2016

But generally 👍 on tunneling this all the way through

if id_key is None:
logger.warning(
'fxa.metrics_uid_secret_key is not set. '
'This will allow PII to be more easily identified')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rfk better?

Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ 👍

@rfk
Copy link
Contributor

rfk commented Feb 25, 2016

r+, although I guess you need #83 to get the green tick from CircleCI?

Let's keep an eye out when QA'ing this to just check that the increased token size doesn't break anything. I can't imagine it will, but there may be e.g. header size limitations that we accidentally bump up against. And for completeness we'll have to check that the loadtests are accurately reflecting the new token size.

@dannycoates
Copy link
Contributor Author

apparently I don't have merge privileges on this or server-syncstorage

@rfk
Copy link
Contributor

rfk commented Feb 25, 2016

you're now a member of "sagrada-devs" which owns these repos; oh man, that's a project code-name I haven't heard in a while...

dannycoates added a commit that referenced this pull request Feb 25, 2016
add fxa uid and device_id to tokens
@dannycoates dannycoates merged commit d13d88e into mozilla-services:master Feb 25, 2016
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.

None yet

2 participants