Skip to content
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

fix(oauth2) remove unique constraint on client_secret #2447

Merged
merged 1 commit into from
May 4, 2017

Conversation

p0pr0ck5
Copy link
Contributor

Summary

Remove the unique constraint on client_secret in the oauth2_credentials table. There appears to be no need to hold this constraint; indeed, no other authentication plugins take this approach (apart from JWT, which builds logic around maintaining a unique key).

Full changelog

  • remove unique constraint on client_secret

Issues resolved

Fix #1972

{
name = "2017-04-24-oauth2_client_secret_not_unique",
up = [[
ALTER TABLE oauth2_credentials DROP CONSTRAINT oauth2_credentials_client_secret_key;
Copy link
Member

@bungle bungle May 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have IF EXISTS here (I'm not sure, but it doesn't hurt)?

@p0pr0ck5 p0pr0ck5 force-pushed the fix/oauth2-client-secret-unique branch 4 times, most recently from 3d7ae27 to 53a5e97 Compare May 3, 2017 02:06
@p0pr0ck5 p0pr0ck5 force-pushed the fix/oauth2-client-secret-unique branch from 53a5e97 to 2e2a366 Compare May 3, 2017 02:21
@p0pr0ck5
Copy link
Contributor Author

p0pr0ck5 commented May 3, 2017

@bungle the migration now includes the IF EXISTS clause.

@thibaultcha thibaultcha added pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) and removed pr/status/please review labels May 4, 2017
@p0pr0ck5 p0pr0ck5 merged commit a1bdede into next May 4, 2017
@p0pr0ck5 p0pr0ck5 deleted the fix/oauth2-client-secret-unique branch May 4, 2017 18:07
@p0pr0ck5
Copy link
Contributor Author

p0pr0ck5 commented May 4, 2017

crap, missed the hold merge label. @thibaultcha do you want me to revert this?

@thibaultcha
Copy link
Member

No that's fine, I think we should have a status/ready and a status/ready (hold merge) distinction

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants