-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
0.6.0 #293
0.6.0 #293
Conversation
The |
@andrewmcnamara this branch is wip :) |
d72e407
to
0c930c0
Compare
@janekolszak check out 1d20c6e |
ID string `db:"id"` | ||
Version int `db:"version"` | ||
Client []byte `db:"client"` | ||
ID string `db:"id"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it looks much simpler and better.
Plain SQL will be a great advantage. It's fast and predictable.
ResponseTypes string `db:"response_types"` | ||
Scope string `db:"scope"` | ||
Owner string `db:"owner"` | ||
PolicyURI string `db:"policy_uri"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd divide this table into two - one with OIDC specific data and one with the ClientInfo.
I'm not sure Hydra should handle ClientInfo part at all. It looks IdP specific and I know I will have to add some fields in my service and not use others. I might as well store my own version of ClientInfo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the default data that makes hydra compliant with https://tools.ietf.org/html/rfc7592 - you can still use your own metadata storage
Our use case is preventing issues with backwards compatibility based on what we've implemented. The existing version uses aud, not client_id as they are practically the same thing. And it's the way we identify a client credentials token from a standard auth one, because these fields match. And what spec? If you're going to use the JWT fields of iat, exp, etc. Mixing in the non-JWT client_id when aud represents the same thing is very strange. |
https://tools.ietf.org/html/rfc7662#section-2.2
We could include both, though. Btw BC breaks are part of the minor versioning and the reason why Hydra is not marked as stable (major version 1) yet. :) |
But you use the Client ID as the AUD in the OpenID token now right? So the behaviour is inconsistent. And you really need some way of marking an access token as a client credentials (subject-free) version. |
I don't think that the subject of a client credentials token should be empty. It should be the client id. In which case it's pretty easy to figure out by checking if client_id == subject.
Yes, this is why I offered two times to include the aud claim as well! |
Adding the Sorry, didn't notice that above. |
Our use case is preventing issues with backwards compatibility based on The existing version uses aud, not client_id as they are practically the And it's the way we identify a client credentials token from a standard On Monday, 24 October 2016, Aeneas notifications@github.com wrote:
|
And what spec? If you're going to use the JWT fields of iat, exp, etc. Mixing in the On Monday, 24 October 2016, Wayne Robinson wayne.robinson@gmail.com wrote:
|
Is the scope/s key also different? On Monday, 24 October 2016, Andrew McNamara notifications@github.com
|
?parseTime=true
mysql requirementbc breaks: