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

DO NOT MERGE: Fix redirectable with blank uris #1

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

Conversation

urnf
Copy link

@urnf urnf commented Aug 6, 2019

NOTE: This PR starts from Doorkeeper 5.0.2 as a base. This is intended to serve as a temporary source for our gem in the interim while we upgrade to Ruby 2.4+. This branch is pointed to from our gemfile, do not merge this PR!

Summary

We are fixing redirectable? returning the wrong result as a consequence of a different error name type. This should function correctly independently of error name type.

In base Doorkeeper, there is a PR and commit that the maintainer is looking to push into the latest 5.2 that fixes the issue in one way doorkeeper-gem@444fdbe where their intent is that validations (and the error response generated) need to happen in a very specific order and would avoid running into this issue.

- Calling public method redirectable should do the right thing regardless of validation state

Signed-off-by: Kevin Lai <[email protected]>
Co-authored-by: Kevin Lai <[email protected]>
@urnf
Copy link
Author

urnf commented Sep 24, 2019

Thinking about closing this PR, but we are actively using it in our bigmaven gemfile and I cannot risk someone accidentally coming back here to delete it.

@urnf
Copy link
Author

urnf commented Mar 10, 2020

That commit looks like it's been rolled up into this PR and comment https://github.com/doorkeeper-gem/doorkeeper/pull/1277/files#r310095549

As soon as we get to Ruby 2.4/2.7 we should consider upgrading.

@jhmoore Is there anything we can do to limit access to this fork or prevent merging while this gets worked on?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants