search

doorkeeper-gem / doorkeeper

Doorkeeper is an OAuth 2 provider for Ruby on Rails / Grape.
https://doorkeeper.gitbook.io/guides/
MIT License
5.34k stars 1.07k forks source link

Doorkeeper shouldn't generate a secret for public clients #1724

Closed ThisIsMissEm closed 2 months ago

ThisIsMissEm commented 3 months ago

Currently doorkeeper's Application model always generates a secret value, even when the client type should not have a client secret generated (i.e., public clients). This means we're storing a secret for applications even when a secret value is completely unneeded

I think the model & migrations likely need to be changed to make oauth_application.secret nullable, and only generate a secret if app.confidential?

The Doorkeeper::Application.by_uid_and_secret() method does do the right thing and not bother with secret validation if the client is public.

However, I'm not sure if POST /oauth/token will explicitly reject the request if client_secret is presented despite the client being public. It seems this happens more through a side effect of by_uid_and_secret()'s logic.

This would save significant database resources for applications such as Mastodon, where we've a significant number of applications, due to using a form of Dynamic Client Registration. (i.e., the application count is in the tens or hundreds of thousands, if not millions).

nbulaj commented 3 months ago

Thanks @ThisIsMissEm , agree, that would be great to change. Better to arrange API as well :+1: Would you like to propose a MR? Not sure when I'll be free to check it

nbulaj commented 3 months ago

Prepared a draft MR @ThisIsMissEm - https://github.com/doorkeeper-gem/doorkeeper/pull/1727 If you have a chance to check - will be thankful :handshake:

Basically API should already consider public/private clients when we introduced such functionality so I don't think we need to do here anything.

Existing applications have to remove not null constraint. Maybe I missed something more?