Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

feat(token): return the uid from the /token endpoint #3000

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vladikoff
Copy link
Contributor

@ghost ghost assigned vladikoff Mar 28, 2019
@ghost ghost added the waffle:active label Mar 28, 2019
@vladikoff
Copy link
Contributor Author

vladikoff commented Mar 28, 2019

PR 3000, wooo

Blocks #2985

@vladikoff vladikoff force-pushed the token-endpoint-user branch 2 times, most recently from 14089fd to 182589b Compare March 28, 2019 13:35
Copy link

@shane-tomlinson shane-tomlinson left a comment

Choose a reason for hiding this comment

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

This makes me uncomfortable because we take care to only return an id_token if the grant contains the openid scope, which contains the same info in sub. With this change we are adding the uid whether requested or not and whether granted or not. While that is acceptable for requests coming from the auth-server, that falls outside of expected behavior for the general case.

Could we somehow restrict this behavior to auth-server originating calls? Or, the auth server adds the openid scope when calling the oauth-server's /token endpoint and then remove the id_token if the RP didn't actually request it?

@@ -119,6 +119,7 @@ module.exports.generateTokens = async function generateTokens(grant) {
const access = await db.generateAccessToken(grant);
const result = {
access_token: access.token.toString('hex'),
user: access.userId.toString('hex'),

Choose a reason for hiding this comment

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

What if we call this sub like we do for an id token. At least it'd be consistent. But I see verify returns user. mmm...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

another idea is to use the fxa-uid field

Copy link

@shane-tomlinson shane-tomlinson left a comment

Choose a reason for hiding this comment

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

Changing to "request changes" to figure out how to only return this parameter when expectations are met.

@vladikoff
Copy link
Contributor Author

With this change we are adding the uid whether requested or not and whether granted or not. While that is acceptable for requests coming from the auth-server, that falls outside of expected behavior for the general case.

UID can be easily obtain by calling verify token

@rfk
Copy link
Contributor

rfk commented Mar 28, 2019

With this change we are adding the uid whether requested or not and whether granted or not.

AFAICT the /verify endpoint will unconditionally return the uid when you present it with a valid access token, regardless of what scopes have been granted to that token:

user: token.userId.toString('hex'),

Which is why we figured it would be OK to return it here as well. But perhaps that's an oversight rather than a deliberate behaviour.

(Edit: lol, mid-air comment collision with vlad's response)

@rfk
Copy link
Contributor

rfk commented Mar 28, 2019

Could we somehow restrict this behavior to auth-server originating calls?

Another option would be to only return it when the scopes explicitly allow it, e.g. when requesting profile or openid scope. I expect this would suffice for the cases where the auth-server wants to know this value without a second db lookup.

@shane-tomlinson
Copy link

Another option would be to only return it when the scopes explicitly allow it, e.g. when requesting profile or openid scope. I expect this would suffice for the cases where the auth-server wants to know this value without a second db lookup.

It could be that we assumed all RPs would at least request profile, though that's not guaranteed. I'd prefer us to do some minimal checking on /verify too to prevent RPs from getting info not explicitly granted.

@shane-tomlinson
Copy link

This repo has been deprecated and migrated to https://github.com/mozill/fxa. Please open this PR against that repo.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants