-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Consider defaulting Application.confidential to false #1142
Comments
What do you mean by |
Hi @rhymes . By default all the clients have secret key, so it doen't fully clear to me. Did you patch Doorkeeper models? |
@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. |
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! |
@nbulaj thank you! :-) |
Upgrade guides updated (feel free to add or fix something here, it's public) |
shouldn't be? The confidential column is true by default, isn't it? |
@arekt you are right, my fault. Fixed |
thank you! I think we can close the ticket :-) |
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? |
Whooops, it's a typo in changelog. I'll fix it, thanks @xtagon UPD: fixed |
Thanks, that was fast! |
Recently we had an app break because of the following reasons:
Backport security fix from 5.x for token revocation when using public clients
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
The text was updated successfully, but these errors were encountered: