Update changes for Doorkeeper 5.2 #97
Conversation
…ion_controller of doorkeeper
linhdangduy
force-pushed
the
refactoring_based_on_doorkeeper_5.2
branch
from
1fe2fc1
to
83f8c44
Compare
| pre_auth.client, | ||
| owner.id, | ||
| pre_auth.scopes | ||
| ) |
There was a problem hiding this comment.
Change this to be the same implement with doorkeeper:
https://github.com/doorkeeper-gem/doorkeeper/blob/v5.2.0/app/controllers/doorkeeper/authorizations_controller.rb#L44-L50
If don't have any access_token, it will return nil
There was a problem hiding this comment.
Hmm this is related to a subtle bug which I'm not sure has been fixed in Doorkeeper: doorkeeper-gem/doorkeeper#1197
The spec mentioned in the issue is still there in master / v5.2.2: https://github.com/doorkeeper-gem/doorkeeper/blob/v5.2.2/spec/models/doorkeeper/access_token_spec.rb#L544-L548
Do you know anything about this? I'm not sure if this is still relevant, I'll see if we have a spec for this case or add one in master if necessary.
There was a problem hiding this comment.
@linhdangduy ok I had a look, we don't have a spec for this and I decided not to add one, because it wouldn't make much sense to use a different logic here if Doorkeeper would then end up using the other logic later anyway.
So basically, let's ignore this edge-case and revisit if it actually causes problems for someone 😉
I also saw authorizations_controller.rb defines matching_token? with the same code, so can we replace oidc_consent_required? below with simply:
!skip_authorization && !matching_token?And it looks like matching_token? in Doorkeeper should actually do a .present? check at the end? Currently it can return an empty array, which would be truthy even if no matching token is actually found.
There was a problem hiding this comment.
so can we replace oidc_consent_required? below with simply:
!skip_authorization && !matching_token?
yeah, we can simply use existed matching_token?.
I think about it before, misunderstood that there will be a loop because matching_token? use current_resource_owner, but what doorkeeper-oidc monkey-patched is authenticate_resource_owner!, not current_resource_owner.
So incase we use it, we doesn't need to pass owner to matching_tokens_for_oidc_resource_owner method.
And it looks like matching_token? in Doorkeeper should actually do a .present? check at the end? Currently it can return an empty array, which would be truthy even if no matching token is actually found.
The matching_token? method return nil if matching record was not found, not an empty array, I think. We can see the comment here.
linhdangduy
force-pushed
the
refactoring_based_on_doorkeeper_5.2
branch
from
83f8c44
to
867ed7c
Compare
There was a problem hiding this comment.
@linhdangduy thank you very much for this PR, and please excuse the delay!
This generally LGTM, just some minor notes. I'll get back to you soon after I tested that edge-case about reauthenticating with fewer scopes.
| pre_auth.client, | ||
| owner.id, | ||
| pre_auth.scopes | ||
| ) |
There was a problem hiding this comment.
Hmm this is related to a subtle bug which I'm not sure has been fixed in Doorkeeper: doorkeeper-gem/doorkeeper#1197
The spec mentioned in the issue is still there in master / v5.2.2: https://github.com/doorkeeper-gem/doorkeeper/blob/v5.2.2/spec/models/doorkeeper/access_token_spec.rb#L544-L548
Do you know anything about this? I'm not sure if this is still relevant, I'll see if we have a spec for this case or add one in master if necessary.
|
@toupeira @linhdangduy Do you reckon I can help you move forward this much wanted fix? 😄 |
|
Definitely, this has to be merged and released ASAP. I'm putting my eyes on this pull request, too. If it still has problems, just feedback, i will fix it. |
|
@toupeira can we merge this? I took a look and it LGTM 🚢 Or let me know if I can help with it somehow |
|
@linhdangduy @nbulaj really sorry for dropping the ball on this again 🙈 Will merge now and release a new version soon after going through the other open PRs and issues. |
|
Thanks @toupeira ! Also just as side note - you can ping me anytime if you need some help with Doorkeeper internals or found some strange or breaking changes in it. I'll try to help you with them. Also I reviewed some Doorkeeper extensions (like this one and doorkeeper-jwt) and found that Doorkeeper has some high-coupling in few classes, so I've done some refactoring to allow code reuse for extensions. Like https://github.com/doorkeeper-gem/doorkeeper/pull/1359/files and doorkeeper-gem/doorkeeper@78c822b#diff-30d1c02eb523d3fdbe0ea87cd811bc3fR25 You could re-use Option DSL and mapper now, but it would be available only from Doorkeeper 5.4 :) |
|
@nbulaj thanks, that looks great and I appreciate your help! 👍 I've added a note to the README now that we're looking for maintainers, I don't feel too great about how I keep neglecting it 🤦♂️ If you want I can add you as a gem owner so you can cut releases as well, my only requirement would be that you enable 2FA on rubygems.org 😉 |
|
Yep, it's a great idea @toupeira. I have 2FA both on GitHub and rubygems :) |
|
@nbulaj perfect, you're added now! 👍 The release process is documented at https://github.com/doorkeeper-gem/doorkeeper-openid_connect/blob/master/CONTRIBUTING.md#release-process |
#85 tried to make doorkeeper-openid_connect to be compatible with doorkeeper 5.2, but there are errors to be fixed and some place to refactor. So I create this PR to "completely" make this gem compatible with doorkeeper 5.2.
What PR does
The work is explained as PR's comment below 👇