Native app callback support #351

Closed
wants to merge 4 commits into
from

2 participants

@jasl

My project using doorkeeper to build OAuth provider, and provides it for native app

According to https://developers.google.com/accounts/docs/OAuth2InstalledApp#choosingredirecturi (it appears here), I think doorkeep misunderstand why provide urn:ietf:wg:oauth:2.0:oob, It's designed for native app not for test

So I move test_redirect_uri to urn:ietf:wg:oauth:2.0:oob:test and add native_redirect_uri use urn:ietf:wg:oauth:2.0:oob, and let it return JSON, easy to parse

As issue #349 I commented, I think test_redirect_uri is no need, maybe we can remove it.

In addition, ErrorResponse, TokenResponse and CodeResponse without a unified interface made many bugs, seems need a refactor, so do their request classes.

@jasl

ping @tute

@tute
doorkeeper rubygem member

In addition, ErrorResponse, TokenResponse and CodeResponse without a
unified interface made many bugs, seems need a refactor, so do their
request classes.

Agreed, been doing little refactors this week in master. Please rebase on
top of it, and I'm happy to discuss better API/organization.


From http://www.ietf.org/mail-archive/web/oauth/current/msg09988.html:

Currently Google OAuth2 implementation uses the special urn:ietf:wg:oauth:2.0:oob
to signal the Authorization Endpoint to return an HTML page with the code,
instead of a redirect. At first sight, it seems a good idea, however it
isn't in the OAuth 2 RFC.

From https://github.com/doorkeeper-gem/doorkeeper/wiki/Authorization-Code-Flow:

(...) you should fill in the redirect URI field with
urn:ietf:wg:oauth:2.0:oob. This will tell doorkeeper to display the
authorization code instead of redirecting to a client application (...).

The parts I take out (...) speak about testing.

From this PR's description:

As issue #349 I commented, I think test_redirect_uri is no need, maybe we
can remove it.

It seems that doorkeeper and Google have the same semantics, but doorkeeper confusingly calls it "testing". I agree there seems to be no need of a specific test_redirect_uri, and I believe doorkeeper already implements it for native clients. The naming seems wrong.

If all of this is true, we can deprecate test_redirect_uri and call it native_redirect_uri? And if so we could say Use <code><%= Doorkeeper.configuration.native_redirect_uri %></code> for native apps or local tests.

@tute
doorkeeper rubygem member

Ping @jasl. Do you agree that behavior is correct but naming is wrong? Do you think the name should be updated through a deprecation? Thanks for your input.

@jasl
@tute
doorkeeper rubygem member

No rush, I completely understand you. :) Good to be in sync, you are free to close this PR and open a new one later, or work over this same one. Enjoy!

@tute
doorkeeper rubygem member

Ping @jasl (cleaning up/working on open issues here :) ).

@jasl
@jasl

see PR #409

@jasl jasl closed this Jun 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment