Doorkeeper: Restrict authorization grant flows for public apps?

[f3ndot]

Now that PKCE has landed and public apps do not require a secret, we should talk about restricting which grant types should be allowed. Especially if use_refresh_token is enabled.

Problem

Assume Doorkeeper has use_refresh_token. If an app is a JS Single Page Application, and is configured as a non-confidential, then it doesn't require a secret. The developer can abuse Doorkeeper and incorrectly chose authorization_code instead of implicit (as they're supposed to).

Because of this, an SPA now has access to an refresh token without guaranteeing its identity and it's against spec to store refresh tokens in browser.

Solution

Industry best practice appears to state that there should be only one type of authorization flow to use based on the use case:

via Auth0.

I propose we do the following:

1. Only allow Authorization Code for confidential apps
2. Only allow non-confidential apps to use PKCE or Implicit
3. Only allow refresh tokens for PKCE, Authorization Code (if enabled)
4. Only allow refresh tokens for Password if its on a confidential app (if enabled)

Open Questions

What about Client Credentials? Should they be for confidential only? It looks like a yes, but I'm unsure.

References

1. Auth0's Restrictions on Refresh Tokens
2. OAuth.com's Refresh Tokens

[nbulaj]

Maybe we can find some answer on similar OAuth project for go-lang - Hydra ?

We need to spent some time on this issue exploration. I will back to it when I will have more time.

[f3ndot]

I'll do some more research too. I think it's a question of whether or not we want Doorkeeper to be opinionated and reduce risk of OAuth2 misuse/abuse.

At minimum I think we should not allow public apps to use authorization codes without PKCE.

[nbulaj]

Hi @f3ndot . Could we do anything for 5.0 release to increase Doorkeeper security?

[f3ndot]

I haven't looked too much but it looks like a lot of oauth libs don't try to be too opinionated.

Maybe a warning in server side logs and good documentation stating PKCE ought to be used when it's a public client

[nbulaj]

Agree. Would you like to produce some serverside logging?

I think we can add some links to the docs for PKCE section in README to help developers.

[f3ndot]

Perfect. I'll make a PR for the warnings

[nbulaj]

Hi! I think it's now possible with changes introduced in #1286 (but maybe with additional refactoring to persist this security check without developer intervention using configuration option). So anyone feel free to work on the issue :)

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.