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

Allow to set null secret value for Applications if it's public #1727

Closed nbulaj closed 2 months ago

nbulaj commented 3 months ago

Aims to fix #1724

  rails generate doorkeeper:remove_applications_secret_not_null_constraint
nbulaj commented 3 months ago

The only thing which makes me worry is legacy apps which already had NOT NULL constraint. If they will add a public app DB will throw an error for NULL secret. I have to think how to make these changes backward compatible.

nbulaj commented 3 months ago

Maybe it can be done via:

def self.null_secret_allowed?
  return @null_secret_allowed if defined?(@null_secret_allowed)

  @null_secret_allowed = model_class.columns.detect { |column| column.name == "secret" }&.null
end

UPD: yeah we already have almost similar check for PKCE - pkce_supported?

ThisIsMissEm commented 3 months ago

Perhaps we could modify the migration / generator for enabling public clients to check that secret is nullable?

nbulaj commented 3 months ago

Perhaps we could modify the migration / generator for enabling public clients to check that secret is nullable?

You mean add a custom constraint into database migration? I already added a check on a model level, but custom constraints are DB-dependant so I;m not sure if we wanna to support all possible variants.

ThisIsMissEm commented 3 months ago

I mean, create a migration that can be applied (like adding PKCE) that drops the not-null constrain on client_secret, since it's currently marked as not-null, and we can say that that's how you enable public clients.