Doorkeeper: Consider defaulting Application.confidential to false

[rhymes]

Recently we had an app break because of the following reasons:

• version 4.4.0 was released with the following changelog message: Backport security fix from 5.x for token revocation when using public clients
• we applied the security fix knowing revocation wasn't an issue for our use case
• the security fix went to production without deep testing
• Android users started complaining, iOS users were fine
• after reverting and debugging we noticed a difference between the two: Android clients didn't send the client secret, iOS did
• the security fix set confidential to true which seems to disable clients without client secret

Setting it to false fixed the issue.

My question is: shouldn't a breaking change like this be clearer?

The changelog doesn't mention it, the upgrade guide says to add the migration but it doesn't clearly state: "hey, the default will break clients without client secret key"

Expected behavior

Either the default should change or the documentation should be clearer about this change.

Thank you

[revskill10]

What do you mean by clearer ? Any example for it so that we could make the changelog better ?

[nbulaj]

Hi @rhymes . By default all the clients have secret key, so it doen't fully clear to me. Did you patch Doorkeeper models?

[rhymes]

@revskill10 I mean by explicitly saying in the changelog: "this change means that by default all clients logging without the secret will break" or something like that

@nbulaj the clients in this case are Android and iOS applications, they don't use Ruby, they just use OAuth2 against the Rails server. The Android client didn't send back the secret.

[nbulaj]

Hi @rhymes . I took a look at original PR and backport . You are right - by default now all the apps are considered confidential. I'll work on more clearer changelog and upgrade guides, thanks for mentioning!

[rhymes]

@nbulaj thank you! :-)

[nbulaj]

Upgrade guides updated (feel free to add or fix something here, it's public)

Changelog also updated

[arekt]

shouldn't be?
"You need to manually change confidential column to false if you are using public clients"

The confidential column is true by default, isn't it?

[nbulaj]

@arekt you are right, my fault. Fixed

[rhymes]

thank you! I think we can close the ticket :-)

[xtagon]

It says something different in the 4.4.0 section of the changelog than the 5.x section. Is the default different or was it just missed in the docs?

[nbulaj]

Whooops, it's a typo in changelog. I'll fix it, thanks @xtagon

UPD: fixed

[xtagon]

Thanks, that was fast!