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) security fixes #1409

Merged
merged 1 commit into from
Jul 21, 2016
Merged

fix(oauth2) security fixes #1409

merged 1 commit into from
Jul 21, 2016

Conversation

subnetmarco
Copy link
Member

@subnetmarco subnetmarco commented Jul 15, 2016

  • Authorization codes can only be used once.
  • Authorization codes can only be used by the application that created it.

This fix involves a migration of the database to support an additional credential_id column in the oauth2_authorization_codes table.

ALTER TABLE oauth2_authorization_codes ADD COLUMN credential_id uuid REFERENCES oauth2_credentials (id) ON DELETE CASCADE;
]],
down = [[
ALTER TABLE jwt_secrets DROP COLUMN credential_id;
Copy link

Choose a reason for hiding this comment

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

Typo: wrong table name

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch.

@thibaultcha thibaultcha changed the title (fix) OAuth2 security fixes fix(oauth2) security fixes Jul 20, 2016
{
name = "2016-07-15-oauth2_code_credential_id",
up = [[
TRUNCATE oauth2_authorization_codes;
Copy link
Member

Choose a reason for hiding this comment

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

Is that not an issue for current production instances?

Copy link
Member Author

Choose a reason for hiding this comment

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

Authorization codes are different than access tokens. They are supposed to be consumed immediately and they expire after 5 minutes. Not a big deal.

@thibaultcha
Copy link
Member

Just making sure. Ok then

@subnetmarco subnetmarco 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/needs review labels Jul 21, 2016
@subnetmarco subnetmarco merged commit b3ece5f into next Jul 21, 2016
@subnetmarco subnetmarco deleted the fix/oauth2-security branch July 21, 2016 20:19
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.

4 participants