Better support for native apps

[montdidier]

This PR relates to the discussion here and here

I have loosened the validation to allow native apps to present a custom scheme. The old Google style OOB method is still supported (backwards compatible).

I have removed the native_redirect_url functionality but I don't know how you would like to deprecate it formally. My opinion is to keep the config for a while but print a deprecation warning (so upgraders do not break their app keeping their configuration from previously). The configuration has no impact on the workings of doorkeeper in this PR.

[jasl]

You can run rubocop -a to fix all style issues

[felipeelias]

I think a simple warn "[DOORKEEPER] ... if the setting is present would be enough to notify the users. We should then remove it on a major version (?)

[montdidier]

Sorry was just making sure these changes actually work against my app. They do seem to work.

[montdidier]

Alright, how do we feel about what we see here now? Any more feedback or desires?

[felipeelias]

I will double check this today evening, thanks for the work so far!

[montdidier]

@felipeelias Ok. How we do we feel about this current state? I think this does everything we want?

[felipeelias]

@montdidier thanks for the follow up, indeed looks exactly like we spoke.

One question though: why the logic between registration and code validation now differs? I mean, I was expecting this validation to be the same on both registration and authorization code flow. Am I missing something?

One problem I see with the above is:

I tested your branch against the provider app and I was able to register with the non-scheme uris, but then, as I attempt the authorization, I receive the error page saying The requested redirect uri is malformed or doesn't match client redirect URI.

I believe that can be confusing for users, what do you think?

[montdidier]

@felipeelias I must have missed something. I'll try to fix it. What was the URI you tried to use?

[nbulaj]

See comments please

[nbulaj]

Why do we removed scenario "Authorization Code Flow with hashing ?

[montdidier]

I didn't really remove the scenario per se, more added two variants and changed the name(s), testing oob and oob:auto. Since these scenarios happen within the feature 'Authorization Code Flow' and context 'with grant hashing enabled' I didn't think the scenario as named was saying much. The two new scenarios I guess would read

1. Authorization Code Flow with grant hashing enabled using redirect_url urn:ietf:wg:oauth:2.0:oob.
2. Authorization Code Flow with grant hashing enabled using redirect_url urn:ietf:wg:oauth:2.0:oob:auto.

If expanded out.

Happy to change to whatever folks would prefer.

[felipeelias]

@montdidier app.co:80 and variations of it

[montdidier]

@montdidier app.co:80 and variations of it

Ok. I've changed the validation code, so now it should be consistent across both validators. Let me know what you think.

[felipeelias]

@montdidier this looks good! I tested against the provider app and we're good to go, just make sure that the specs are passing and from my side it's good to merge

[nbulaj]

LGTM 👍

Just minor comments about the failing specs. And don't forget please to squash your commits 🙏 and I think we can finish with the PR

[montdidier]

@nbulaj Are you sure you want me to squash or will somebody squash when it comes to merge time in the UI?

[felipeelias]

@montdidier please do, I think there is a reason why it's there

[montdidier]

@felipeelias there we go. let me know if anything else needs to happen.

[nbulaj]

Small comments

[houndci-bot]

Some files could not be reviewed due to errors:

cannot load such file -- rubocop-performance

cannot load such file -- rubocop-performance
/home/linters/.bundle/gems/rubocop-0.64.0/lib/rubocop/config_loader_resolver.rb:15:in `require'
/home/linters/.bundle/gems/rubocop-0.64.0/lib/rubocop/config_loader_resolver.rb:15:in `block in resolve_requires'
/home/linters/.bundle/gems/rubocop-0.64.0/lib/rubocop/config_loader_resolver.rb:11:in `each'
/home/linters/.bundle/gems/rubocop-0.64.0/lib/rubocop/config_loader_resolver.rb:11:in `resolve_requires'
/home/linters/.bundle/gems/rubocop-0.64.0/lib/rubocop/config_loader.rb:44:in `load_file'
/home/linters/.bundle/gems/rubocop-0.64.0/lib/rubocop/config_loader.rb:82:in `configuration_from_file'
/home/linters/.bundle/gems/rubocop-0.64.0/lib/rubocop/config_store.rb:44:in `for'
/home/linters/.bundle/gems/rubocop-0.64.0/lib/rubocop/cli.rb:206:in `apply_default_formatter'
/home/linters/.bundle/gems/rubocop-0.64.0/lib/rubocop/cli.rb:46:in `run'
/home/linters/.bundle/gems/rubocop-0.64.0/exe/rubocop:13:in `block in '
/usr/local/lib/ruby/2.6.0/benchmark.rb:308:in `realtime'
/home/linters/.bundle/gems/rubocop-0.64.0/exe/rubocop:12:in `'
/home/linters/.bundle/bin/rubocop:23:in `load'
/home/linters/.bundle/bin/rubocop:23:in `'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:74:in `load'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:74:in `kernel_load'
/usr/local/lib/ruby/2.6.0/bundler/cli/exec.rb:28:in `run'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:463:in `exec'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:27:in `dispatch'
/usr/local/lib/ruby/2.6.0/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
/usr/local/lib/ruby/2.6.0/bundler/cli.rb:18:in `start'
/usr/local/lib/ruby/gems/2.6.0/gems/bundler-1.17.2/exe/bundle:30:in `block in '
/usr/local/lib/ruby/2.6.0/bundler/friendly_errors.rb:124:in `with_friendly_errors'
/usr/local/lib/ruby/gems/2.6.0/gems/bundler-1.17.2/exe/bundle:22:in `'
/usr/local/bin/bundle:23:in `load'
/usr/local/bin/bundle:23:in `'

[nbulaj]

@montdidier could you please rebase with the current master? And fix conflicts please?

I think this PR is ready to be merged, I have no objections. So you can squash it and we'll continue.

[montdidier]

@nbulaj done.

[nbulaj]

Last @montdidier . Do we need to specify in our upgrade guides any breaking changes for this PR? I mean if something previously worked and now is broken - we need to define it.

https://github.com/doorkeeper-gem/doorkeeper/wiki/Migration-from-old-versions

[montdidier]

@nbulaj It should not have changed for people using the old method of native support. It might be worth noting in the changes guide that the configuration setting native_redirect_url is deprecated. It just does nothing now but having it won't break anything having it.

[felipeelias]

@montdidier thank you for your contribution ❤️

[montdidier]

@felipeelias No problem! Thank you, for your feedback and patience.

[ghiculescu]

@montdidier we currently support creating apps with a local redirect URI (using native_redirect_uri)

Am I right in assuming doorkeeper no longer supports this functionality? Should we remove our support for it?

I don't mind removing it but I'm confused by your comments that "It should not have changed for people using the old method of native support." As far as I can tell, support for that method was entirely removed here.

[montdidier]

@ghiculescu if I remember correctly it still works but when you request the redirect url in the admin page you use urn:ietf:wg:oauth:2.0:oob to signal you want to use this method. It was a method that was never formally ratified and I would recommend moving to a better method if you can, but it should still work if used in this way. What did you have in your native_redirect_uri ?

[ghiculescu]

@montdidier this is what our /oauth/applications/new page looks like atm:

[montdidier]

My memory is foggy on this topic because I have not looked at it since; so definitely double check what I am saying.

So depending on how your app works you have a few options.

1. You can use the original method which was a never ratified proposal from the working group which you can use by putting urn:ietf:wg:oauth:2.0:oob or urn:ietf:wg:oauth:2.0:oob:autoin the redirect URL. I believeurn:ietf:wg:oauth:2.0:ooboriginally was intended for a copy and paste by the user andurn:ietf:wg:oauth:2.0:oob:auto` used what ever programatic extraction from within the app was possible. If you are using this currently and it works you can probably stick with it, but I would recommend switching to the one of the options below.

This basically puts the authorization token directly into a web view or the browser title view and the mobile app retrieves it from there. The native_redirect_uri was some belt and suspenders way for checking what was put in the redirect_uri. When the app is registered the redirect URL is registered there so the native_redirect_uri was not flexible enough to support all method simultaneously.

2. You can use custom scheme redirection. Which looks like com.example.app:redirect_uri_path and you build a special custom scheme handler in the app. This is better. Use authorization code flow with PKCE extension for maximum best practice.

3. You can use loopback redirection which looks like http://127.0.0.1:9871. In this case you need to open the a port to receive an HTTP request when waiting for the result to redirect back from the local webview or securewebview or whatever.

What did you app have for native_redirect_uri currently?

[ghiculescu]

What did you app have for native_redirect_uri currently?

We had this:

I guess we were using it as a nicer looking version of urn:ietf:wg:oauth:2.0:oob.

This basically puts the authorization token directly into a web view or the browser title view and the mobile app retrieves it from there.

Yep, that's what we were after, except I think it was mostly useful for generating auth tokens for testing quickly without necessarily implementing an Oauth flow. As far as I can tell it wasn't used in any meaningful way in production. Definitely a nice to have but not critical.