feat: Authorization Grants using JWT #546
Conversation
…rizeJwtGrantHandler
…ntHandler, so it matches order in rfc
… AuthorizeJwtGrantHandler
|
|
Xopek
changed the title
There was a problem hiding this comment.
I hope your are not initimidated by the flood of comments. In general this looks very solid already, but I think we can improve it a bit in terms of readability and I also left some security-related ideas :)
Once the next pass is done, I will have another look at the tests, and if nothing serious comes up I think this is pretty much good to go!
| expect: &AccessRequest{ | ||
| GrantTypes: Arguments{"foo"}, | ||
| Request: Request{ | ||
| Client: client, |
There was a problem hiding this comment.
Where does this client come from? Shouldn't the client be nil? What I also don't fully understand is - store := internal.NewMockStorage(ctrl) does not seem to have any EXPECT calls?
There was a problem hiding this comment.
It is default empty client struct. When Request is created default value is used for client, so it is just an empty struct. So we are checking, that even without client auth (and hence storage wont be called, and client struct will stay empty) everything is working.
Considering storage - it is not expected to be called, because we are not passing any client credentials. However i agree that it is not very clear why is that. That's why i will add case, where we WILL pass client credentials and in other cases will add explicit instruction, that we are not expecting storage to be called.
There was a problem hiding this comment.
Also i am suggesting to rewrite the whole tests in this file completely, because each test affects other test, because they share mocks and other vars which results in that test cases are not isolated from each other. That's why new tests that were added in this PR were made using suite package of testify. Only in this file i write new test cases in the same style as old ones.
If you are not against this idea maybe i can rewrite them using suite, but i will made it in separate PR.
No problem at all!) |
|
Are you still up for the changes? :) If you need any help, let us know! |
Yes! Just need to do some work and then i will return to this PR =) |
|
Ok, i addressed all comments and made necessary changes, have a look) |
|
Hello, I wanted to review this today but am too tired to give it the attention it needs. I will try again on Monday :) |
Related issue
#305
Proposed changes
Support for JSON Web Token (JWT) Profile for OAuth 2.0 Client Authentication and Authorization Grants (RFC7523)
Checklist
and signed the CLA.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
appropriate).
Further comments
Implementation details were discussed with @aeneasr beforehand