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

scope in access token is not space delimited #362

Closed
zhuowang10 opened this issue May 15, 2019 · 11 comments · Fixed by #482
Closed

scope in access token is not space delimited #362

zhuowang10 opened this issue May 15, 2019 · 11 comments · Fixed by #482

Comments

@zhuowang10
Copy link

zhuowang10 commented May 15, 2019

Scope is hard corded as "scp" in access token, and its format is not space delimited string but a string list.
Although scope name and format in access token is not defined in jwt token standard (https://tools.ietf.org/html/rfc7519#page-9), scope in oauth2 request/response is defined as space delimited string (https://tools.ietf.org/html/rfc6749#section-3.3).
Can we expose functions to customize the name and format of scope in access token?

@aeneasr
Copy link
Member

aeneasr commented May 16, 2019

An OIDC related spec lists the scope as scp and as an array which is why we're using that format here. I think it should be straight forward to extend the concrete implementation for that feature if you'd like to have it as scope with space-delimited string.

@zhuowang10
Copy link
Author

Thanks for the quick response.
Do you mind to point out which oidc related spec you mentioned? I'm trying to find the standard to decide which format to use, and didn't find out yet.

@mitar
Copy link
Contributor

mitar commented May 30, 2020

Was it maybe this document? Because that is just a draft. In final standard, scope is used and it is a not an array.

I think the source of this is this PR and comment, see also this comment.

So, this was all based on a draft and now standard is different and this should be fixed.

@aeneasr
Copy link
Member

aeneasr commented May 30, 2020

I don't think we will "fix" the current scope (we could add a new field though!) because it's going to be breaking with pretty hard implications for e.g. access control proxies. Might make it configurable of course but not sure how useful that really is? I'd actually prefer to have just one spec.

@mitar
Copy link
Contributor

mitar commented May 30, 2020

because it's going to be breaking with pretty hard implications

But it will be failing closed. So this should not open any access.

So decide what should be here. Current scp field is not in any spec/standard. scope is. If you want to be standard compliant, you should or:

  • Rename/change.
  • Add a new field.
  • Make it configurable.

Which one? I can try to make a PR, but should know which approach you want.

I'd actually prefer to have just one spec.

So scope then and breaking compatibility?

Is there any other spec which uses scp and array of values?

@aeneasr
Copy link
Member

aeneasr commented May 30, 2020

No, adding scope is fine - I thought that's the case already. I wrote Fosite years ago so I actually don't remember all the details anymore ;)

Adding scope with a space-delimited string is fine from my end!

@mitar
Copy link
Contributor

mitar commented May 30, 2020

No, it is just scp:

if c.Scope != nil {
ret["scp"] = c.Scope
}

@mitar
Copy link
Contributor

mitar commented May 30, 2020

Can we also make a configuration option to not add scp? To keep JWTs small?

@aeneasr
Copy link
Member

aeneasr commented May 30, 2020

We could also just make the scope key configurable

@mitar
Copy link
Contributor

mitar commented May 30, 2020

I mean, the value of scope key is different based on the name. :-)

So I propose configuration option which is enum to select between scp, scope, or both? Not sure how to get that setting to here though.

@aeneasr
Copy link
Member

aeneasr commented Jun 1, 2020

I mean, the value of scope key is different based on the name. :-)

True, but also different per specification ;) But let's go with a simple solution first, I think the enum sounds good!

Hm, maybe a WithScope(key string, value string) function for the JWTClaims? Then set that whereever the JWT is generated?

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 a pull request may close this issue.

3 participants