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

[nbulaj]

Aims to fix #1724

rails generate doorkeeper:remove_applications_secret_not_null_constraint

[ThisIsMissEm]

This looks good to me, also, fwiw, I would've been happy to work on a PR, but was a bit stumped by the test failures. With the gemfile change I'll see if I can get the tests running locally with this branch.

[nbulaj]

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]

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]

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

[nbulaj]

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]

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.

[ThisIsMissEm]

So I tried this locally on main and still can't get the tests passing, per #1721

[nbulaj]

Works for me and GitHub CI 🤔
Sorry I don't use dev-containers / docker / other local setups, not sure what can happen there

[ThisIsMissEm]

I don't either!