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

RP-Initiated Logout phase #5

Merged
merged 4 commits into from
Feb 2, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 41 additions & 5 deletions lib/omniauth/strategies/openid_connect.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ class OpenIDConnect
authorization_endpoint: '/authorize',
token_endpoint: '/token',
userinfo_endpoint: '/userinfo',
jwks_uri: '/jwk'
jwks_uri: '/jwk',
end_session_endpoint: nil
}
option :issuer
option :discovery, false
Expand All @@ -42,6 +43,7 @@ class OpenIDConnect
option :send_nonce, true
option :send_scope_to_token_endpoint, true
option :client_auth_method
option :post_logout_redirect_uri

uid { user_info.sub }

Expand Down Expand Up @@ -82,8 +84,8 @@ def config
end

def request_phase
options.issuer = issuer if options.issuer.blank?
discover! if options.discovery
options.issuer = issuer if options.issuer.nil? || options.issuer.empty?
discover!
redirect authorize_uri
end

Expand All @@ -96,8 +98,8 @@ def callback_phase
elsif !request.params['code']
return fail!(:missing_code, OmniAuth::OpenIDConnect::MissingCodeError.new(request.params['error']))
else
options.issuer = issuer if options.issuer.blank?
discover! if options.discovery
options.issuer = issuer if options.issuer.nil? || options.issuer.empty?
Copy link

Choose a reason for hiding this comment

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

Line is too long. [81/80]

Copy link

Choose a reason for hiding this comment

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

Line is too long. [81/80]

discover!
Copy link

Choose a reason for hiding this comment

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

Inconsistent indentation detected.

client.redirect_uri = redirect_uri
client.authorization_code = authorization_code
access_token
Expand All @@ -111,10 +113,26 @@ def callback_phase
fail!(:failed_to_connect, e)
end

def other_phase
Copy link

Choose a reason for hiding this comment

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

Assignment Branch Condition size for other_phase is too high. [15.56/15]

Copy link
Collaborator

Choose a reason for hiding this comment

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

#5 (comment)

Not sure why but this thread marked as "outdated". Anyways, I'm totally fine with your latest suggestion. Apply it and we're good to go.

Copy link

Choose a reason for hiding this comment

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

Assignment Branch Condition size for other_phase is too high. [16.52/15]

Copy link

Choose a reason for hiding this comment

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

Assignment Branch Condition size for other_phase is too high. [16.52/15]

if logout_path_pattern.match?(current_path)
options.issuer = issuer if options.issuer.nil? || options.issuer.empty?
Copy link

Choose a reason for hiding this comment

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

Line is too long. [81/80]

Copy link

Choose a reason for hiding this comment

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

Line is too long. [81/80]

discover!
return redirect(end_session_uri) if end_session_uri
end
call_app!
end

def authorization_code
request.params['code']
end

def end_session_uri
return unless end_session_endpoint_is_valid?
Copy link

Choose a reason for hiding this comment

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

Add empty line after guard clause.

Copy link

Choose a reason for hiding this comment

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

Add empty line after guard clause.

Copy link

Choose a reason for hiding this comment

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

Add empty line after guard clause.

end_session_uri = URI(client_options.end_session_endpoint)
end_session_uri.query = encoded_post_logout_redirect_uri
end_session_uri.to_s
end

def authorize_uri
client.redirect_uri = redirect_uri
opts = {
Expand Down Expand Up @@ -143,10 +161,12 @@ def issuer
end

def discover!
return unless options.discovery
Copy link

Choose a reason for hiding this comment

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

Add empty line after guard clause.

Copy link

Choose a reason for hiding this comment

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

Add empty line after guard clause.

Copy link

Choose a reason for hiding this comment

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

Add empty line after guard clause.

client_options.authorization_endpoint = config.authorization_endpoint
client_options.token_endpoint = config.token_endpoint
client_options.userinfo_endpoint = config.userinfo_endpoint
client_options.jwks_uri = config.jwks_uri
client_options.end_session_endpoint = config.end_session_endpoint if config.respond_to?(:end_session_endpoint)
Copy link

Choose a reason for hiding this comment

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

Line is too long. [118/80]

Copy link

Choose a reason for hiding this comment

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

Line is too long. [118/80]

Copy link

Choose a reason for hiding this comment

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

Line is too long. [118/80]

end

def user_info
Expand Down Expand Up @@ -235,6 +255,22 @@ def redirect_uri
"#{ client_options.redirect_uri }?redirect_uri=#{ CGI.escape(request.params['redirect_uri']) }"
end

def encoded_post_logout_redirect_uri
return unless options.post_logout_redirect_uri
Copy link

Choose a reason for hiding this comment

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

Add empty line after guard clause.

Copy link

Choose a reason for hiding this comment

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

Add empty line after guard clause.

Copy link

Choose a reason for hiding this comment

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

Add empty line after guard clause.

URI.encode_www_form(
post_logout_redirect_uri: options.post_logout_redirect_uri
)
end

def end_session_endpoint_is_valid?
client_options.end_session_endpoint &&
client_options.end_session_endpoint =~ URI::DEFAULT_PARSER.make_regexp
end

def logout_path_pattern
@logout_path_pattern ||= %r{\A#{Regexp.quote(request_path)}(/logout)}
end

class CallbackError < StandardError
attr_accessor :error, :error_reason, :error_uri

Expand Down
58 changes: 58 additions & 0 deletions test/lib/omniauth/strategies/openid_connect_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,63 @@ def test_request_phase
strategy.request_phase
end

def test_logout_phase_with_discovery
Copy link

Choose a reason for hiding this comment

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

Assignment Branch Condition size for test_logout_phase_with_discovery is too high. [33.38/15]

Copy link

Choose a reason for hiding this comment

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

Method has too many lines. [16/10]

Copy link

Choose a reason for hiding this comment

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

Assignment Branch Condition size for test_logout_phase_with_discovery is too high. [33.38/15]

Copy link

Choose a reason for hiding this comment

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

Method has too many lines. [16/10]

Copy link

Choose a reason for hiding this comment

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

Assignment Branch Condition size for test_logout_phase_with_discovery is too high. [33.38/15]

Copy link

Choose a reason for hiding this comment

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

Method has too many lines. [16/10]

expected_redirect = %r{^https:\/\/example\.com\/logout$}
strategy.options.client_options.host = 'example.com'
strategy.options.discovery = true

issuer = stub('OpenIDConnect::Discovery::Issuer')
issuer.stubs(:issuer).returns('https://example.com/')
::OpenIDConnect::Discovery::Provider.stubs(:discover!).returns(issuer)

config = stub('OpenIDConnect::Discovery::Provder::Config')
config.stubs(:authorization_endpoint).returns('https://example.com/authorization')
config.stubs(:token_endpoint).returns('https://example.com/token')
config.stubs(:userinfo_endpoint).returns('https://example.com/userinfo')
config.stubs(:jwks_uri).returns('https://example.com/jwks')
config.stubs(:end_session_endpoint).returns('https://example.com/logout')
::OpenIDConnect::Discovery::Provider::Config.stubs(:discover!).with('https://example.com/').returns(config)

request.stubs(:path_info).returns('/auth/openidconnect/logout')

strategy.expects(:redirect).with(regexp_matches(expected_redirect))
strategy.other_phase
end

def test_logout_phase_with_discovery_and_post_logout_redirect_uri
Copy link

Choose a reason for hiding this comment

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

Assignment Branch Condition size for test_logout_phase_with_discovery_and_post_logout_redirect_uri is too high. [34.53/15]

Copy link

Choose a reason for hiding this comment

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

Method has too many lines. [17/10]

Copy link

Choose a reason for hiding this comment

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

Assignment Branch Condition size for test_logout_phase_with_discovery_and_post_logout_redirect_uri is too high. [34.53/15]

Copy link

Choose a reason for hiding this comment

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

Method has too many lines. [17/10]

Copy link

Choose a reason for hiding this comment

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

Assignment Branch Condition size for test_logout_phase_with_discovery_and_post_logout_redirect_uri is too high. [34.53/15]

Copy link

Choose a reason for hiding this comment

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

Method has too many lines. [17/10]

expected_redirect = 'https://example.com/logout?post_logout_redirect_uri=https%3A%2F%2Fmysite.com'
strategy.options.client_options.host = 'example.com'
strategy.options.discovery = true
strategy.options.post_logout_redirect_uri = 'https://mysite.com'

issuer = stub('OpenIDConnect::Discovery::Issuer')
issuer.stubs(:issuer).returns('https://example.com/')
::OpenIDConnect::Discovery::Provider.stubs(:discover!).returns(issuer)

config = stub('OpenIDConnect::Discovery::Provder::Config')
config.stubs(:authorization_endpoint).returns('https://example.com/authorization')
config.stubs(:token_endpoint).returns('https://example.com/token')
config.stubs(:userinfo_endpoint).returns('https://example.com/userinfo')
config.stubs(:jwks_uri).returns('https://example.com/jwks')
config.stubs(:end_session_endpoint).returns('https://example.com/logout')
::OpenIDConnect::Discovery::Provider::Config.stubs(:discover!).with('https://example.com/').returns(config)

request.stubs(:path_info).returns('/auth/openidconnect/logout')

strategy.expects(:redirect).with(expected_redirect)
strategy.other_phase
end

def test_logout_phase
strategy.options.issuer = 'example.com'
strategy.options.client_options.host = 'example.com'

request.stubs(:path_info).returns('/auth/openidconnect/logout')

strategy.expects(:call_app!)
strategy.other_phase
end

def test_request_phase_with_discovery
expected_redirect = /^https:\/\/example\.com\/authorization\?client_id=1234&nonce=\w{32}&response_type=code&scope=openid&state=\w{32}$/
strategy.options.client_options.host = 'example.com'
Expand All @@ -42,6 +99,7 @@ def test_request_phase_with_discovery
assert_equal strategy.options.client_options.token_endpoint, 'https://example.com/token'
assert_equal strategy.options.client_options.userinfo_endpoint, 'https://example.com/userinfo'
assert_equal strategy.options.client_options.jwks_uri, 'https://example.com/jwks'
assert_nil strategy.options.client_options.end_session_endpoint
end

def test_uid
Expand Down