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

Add PKCE for auth code flow #979

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@chrisblatchley

chrisblatchley commented Jul 14, 2017

Enable PKCE.

Not expecting this to be merged just yet. specs aren't passing. I just wanted to run the implementation by you all before I spend the time to get them working.

@@ -0,0 +1,6 @@
class EnablePkce < ActiveRecord::Migration

This comment has been minimized.

@houndci-bot

houndci-bot Jul 14, 2017

Missing magic comment # frozen_string_literal: true.

@houndci-bot

houndci-bot Jul 14, 2017

Missing magic comment # frozen_string_literal: true.

def application_owner
migration_template(
'enable_pkce_migration.rb',
'db/migrate/enable_pkce.rb'

This comment has been minimized.

@houndci-bot

houndci-bot Jul 14, 2017

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@houndci-bot

houndci-bot Jul 14, 2017

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

def application_owner
migration_template(
'enable_pkce_migration.rb',

This comment has been minimized.

@houndci-bot

houndci-bot Jul 14, 2017

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@houndci-bot

houndci-bot Jul 14, 2017

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

class Doorkeeper::PkceGenerator < Rails::Generators::Base
include Rails::Generators::Migration
source_root File.expand_path('../templates', __FILE__)
desc 'Provide support for client application ownership.'

This comment has been minimized.

@houndci-bot

houndci-bot Jul 14, 2017

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@houndci-bot

houndci-bot Jul 14, 2017

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

class Doorkeeper::PkceGenerator < Rails::Generators::Base
include Rails::Generators::Migration
source_root File.expand_path('../templates', __FILE__)

This comment has been minimized.

@houndci-bot

houndci-bot Jul 14, 2017

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@houndci-bot

houndci-bot Jul 14, 2017

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

attr_accessor :server, :client, :response_type, :redirect_uri, :state

This comment has been minimized.

@houndci-bot

houndci-bot Jul 14, 2017

Extra blank line detected.

@houndci-bot

houndci-bot Jul 14, 2017

Extra blank line detected.

state: pre_auth.state
state: pre_auth.state,
code_challenge: pre_auth.code_challenge,
transformation_method: pre_auth.code_transformation_method

This comment has been minimized.

@houndci-bot

houndci-bot Jul 14, 2017

Line is too long. [83/80]

@houndci-bot

houndci-bot Jul 14, 2017

Line is too long. [83/80]

attr_accessor :server, :grant, :client, :redirect_uri, :access_token
attr_accessor :server, :grant, :client, :redirect_uri, :access_token, :code_verifier

This comment has been minimized.

@houndci-bot

houndci-bot Jul 14, 2017

Line is too long. [90/80]

@houndci-bot

houndci-bot Jul 14, 2017

Line is too long. [90/80]

return false unless code_verifier
if transformation_method == 'S256'
code_challenge == Base64.urlsafe_encode64(Digest::SHA256.digest(code_verifier))

This comment has been minimized.

@houndci-bot

houndci-bot Jul 14, 2017

Line is too long. [89/80]

@houndci-bot

houndci-bot Jul 14, 2017

Line is too long. [89/80]

return true unless code_challenge.present?
return false unless code_verifier
if transformation_method == 'S256'

This comment has been minimized.

@houndci-bot

houndci-bot Jul 14, 2017

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@houndci-bot

houndci-bot Jul 14, 2017

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@chrisblatchley

This comment has been minimized.

Show comment
Hide comment
@chrisblatchley

chrisblatchley Aug 2, 2017

Proposed WIP implementation for issue #975

chrisblatchley commented Aug 2, 2017

Proposed WIP implementation for issue #975

class EnablePkce < ActiveRecord::Migration
def change
add_column :oauth_access_grants, :code_challenge, :string, null: true
add_column :oauth_access_grants, :transformation_method, :string, null: true

This comment has been minimized.

@moneytree-doug

moneytree-doug Aug 21, 2017

I would suggest renaming this from transformation_method to code_challenge_method to keep it consistent.

@moneytree-doug

moneytree-doug Aug 21, 2017

I would suggest renaming this from transformation_method to code_challenge_method to keep it consistent.

@@ -24,6 +24,17 @@ module AccessGrantMixin
validates :token, uniqueness: true
before_validation :generate_token, on: :create
def verify_code(code_verifier)

This comment has been minimized.

@moneytree-doug

moneytree-doug Aug 21, 2017

I think it would be cleaner to move this logic to validate_code_verifier. It also seems to fit better with the style given: https://github.com/chrisblatchley/doorkeeper/blob/60e8502725b30807645905b6c941f114bc229f78/lib/doorkeeper/oauth/authorization_code_request.rb#L47

@moneytree-doug

moneytree-doug Aug 21, 2017

I think it would be cleaner to move this logic to validate_code_verifier. It also seems to fit better with the style given: https://github.com/chrisblatchley/doorkeeper/blob/60e8502725b30807645905b6c941f114bc229f78/lib/doorkeeper/oauth/authorization_code_request.rb#L47

@scope = attrs[:scope]
@state = attrs[:state]
@code_challenge = attrs[:code_challenge]
@code_challenge_method = attrs[:code_challenge_method] || 'plain'

This comment has been minimized.

@moneytree-doug

moneytree-doug Aug 21, 2017

I don't think it should ever fallback. I would suggest removing || 'plain' entirely. The request should be extremely explicit especially if it's plain, since this method is strongly advised against the the RFC.

@moneytree-doug

moneytree-doug Aug 21, 2017

I don't think it should ever fallback. I would suggest removing || 'plain' entirely. The request should be extremely explicit especially if it's plain, since this method is strongly advised against the the RFC.

@@ -79,6 +79,8 @@ en:
unauthorized_client: 'The client is not authorized to perform this request using this method.'
access_denied: 'The resource owner or authorization server denied the request.'
invalid_scope: 'The requested scope is invalid, unknown, or malformed.'
invalid_code_challenge_method: 'The transformation method must be plain or S256.'

This comment has been minimized.

@moneytree-doug

moneytree-doug Aug 21, 2017

Just a nit pick for consistency, but The code challenge method must be "plain" or "S256"

@moneytree-doug

moneytree-doug Aug 21, 2017

Just a nit pick for consistency, but The code challenge method must be "plain" or "S256"

@moneytree-doug

This comment has been minimized.

Show comment
Hide comment
@moneytree-doug

moneytree-doug Aug 21, 2017

This looks solid. I hope Doorkeeper merges this. @chrisblatchley Do you have any e2e tests coming?

moneytree-doug commented Aug 21, 2017

This looks solid. I hope Doorkeeper merges this. @chrisblatchley Do you have any e2e tests coming?

@rsharrott rsharrott referenced this pull request Nov 8, 2017

Closed

Implemented PKCE #997

@nbulaj nbulaj added the feature label Feb 4, 2018

@bpieck

This comment has been minimized.

Show comment
Hide comment
@bpieck

bpieck Feb 16, 2018

Contributor

Found some issue in this and fixed it. Added some tests in my fork of chrisbatchley's fork

Contributor

bpieck commented Feb 16, 2018

Found some issue in this and fixed it. Added some tests in my fork of chrisbatchley's fork

@nbulaj

This comment has been minimized.

Show comment
Hide comment
@nbulaj

nbulaj Feb 16, 2018

Member

Hi @bpieck . I prefer #997 solution (improved version of this one).

If you have some fixes or issues, you could post them to mentioned PR, because it is a candidate to be merged.

Member

nbulaj commented Feb 16, 2018

Hi @bpieck . I prefer #997 solution (improved version of this one).

If you have some fixes or issues, you could post them to mentioned PR, because it is a candidate to be merged.

@bpieck

This comment has been minimized.

Show comment
Hide comment
@bpieck

bpieck Feb 16, 2018

Contributor

Ok, I will start again with the other one. Maybe I get our Android developer's used library running with the other PR.

My Changes to this PR were in an PR to the Repo of chrisbatchley's fork. I just wanted to mention here, that there is an issue in this PR since last message here was: "This looks solid"

Contributor

bpieck commented Feb 16, 2018

Ok, I will start again with the other one. Maybe I get our Android developer's used library running with the other PR.

My Changes to this PR were in an PR to the Repo of chrisbatchley's fork. I just wanted to mention here, that there is an issue in this PR since last message here was: "This looks solid"

@nbulaj

This comment has been minimized.

Show comment
Hide comment
@nbulaj

nbulaj Feb 16, 2018

Member

Closing this PR in order to merge #997 that has some additional work (no need to have many same PRs that will confuse other developers).

Member

nbulaj commented Feb 16, 2018

Closing this PR in order to merge #997 that has some additional work (no need to have many same PRs that will confuse other developers).

@nbulaj nbulaj closed this Feb 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment