RP-Initiated Logout phase #5

yannvery · 2018-08-24T09:42:51Z

Hello,

Here is an implementation of the logout phase initiated by the RP.

The relative spec:
http://openid.net/specs/openid-connect-session-1_0.html#RPLogout

I've also updated openid_connect dependency to 1.1.6 which supports end_session_endpoint in discovery response.

For information I already use a fork of omniauth_openid_connect with openid_connect 1.1.6 on a rails app in production.

ghost · 2018-08-24T09:42:57Z

test/lib/omniauth/strategies/openid_connect_test.rb

@@ -18,6 +18,63 @@ def test_request_phase
        strategy.request_phase
      end

+      def test_logout_phase_with_discovery


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

ghost · 2018-08-24T09:42:57Z

test/lib/omniauth/strategies/openid_connect_test.rb

@@ -18,6 +18,63 @@ def test_request_phase
        strategy.request_phase
      end

+      def test_logout_phase_with_discovery


Method has too many lines. [16/10]

ghost · 2018-08-24T09:42:57Z

test/lib/omniauth/strategies/openid_connect_test.rb

@@ -18,6 +18,63 @@ def test_request_phase
        strategy.request_phase
      end

+      def test_logout_phase_with_discovery
+        expected_redirect = /^https:\/\/example\.com\/logout$/


Use %r around regular expression.

ghost · 2018-08-24T09:42:57Z

test/lib/omniauth/strategies/openid_connect_test.rb

+        strategy.other_phase
+      end
+
+      def test_logout_phase_with_discovery_and_post_logout_redirect_uri


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

ghost · 2018-08-24T09:42:57Z

test/lib/omniauth/strategies/openid_connect_test.rb

+        strategy.other_phase
+      end
+
+      def test_logout_phase_with_discovery_and_post_logout_redirect_uri


Method has too many lines. [17/10]

ghost · 2018-08-24T09:42:58Z

lib/omniauth/strategies/openid_connect.rb

@@ -111,10 +113,27 @@ def callback_phase
        fail!(:failed_to_connect, e)
      end

+      def other_phase


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

#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.

ghost · 2018-08-24T09:42:58Z

lib/omniauth/strategies/openid_connect.rb

@@ -235,6 +255,19 @@ 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
+        URI.encode_www_form(post_logout_redirect_uri: options.post_logout_redirect_uri)


Line is too long. [87/80]

ghost · 2018-08-24T09:42:58Z

lib/omniauth/strategies/openid_connect.rb

+      end
+
+      def end_session_endpoint_is_valid?
+        client_options.end_session_endpoint && client_options.end_session_endpoint =~ URI.regexp


Line is too long. [96/80]

ghost · 2018-08-24T09:42:58Z

lib/omniauth/strategies/openid_connect.rb

+      end
+
+      def end_session_endpoint_is_valid?
+        client_options.end_session_endpoint && client_options.end_session_endpoint =~ URI.regexp


URI.regexp is obsolete and should not be used. Instead, use URI::DEFAULT_PARSER.make_regexp.

ghost · 2018-09-07T09:40:17Z

lib/omniauth/strategies/openid_connect.rb

@@ -111,10 +111,25 @@ def callback_phase
        fail!(:failed_to_connect, e)
      end

+      def other_phase
+        if logout_path_pattern.match(current_path)


Use match? instead of match when MatchData is not used.

m0n9oose · 2019-01-04T17:40:00Z

lib/omniauth/strategies/openid_connect.rb

-        client_options.userinfo_endpoint = config.userinfo_endpoint
-        client_options.jwks_uri = config.jwks_uri
+        return unless options.discovery
+        options.issuer = issuer if options.issuer.blank?


Am i right in assumption issuer won't be set if discovery disabled?

Yes.
Discovery issuer will be only used only discovery is enabled and options issuer was blank?

Another point, I'm wondering if you prefer that I avoid using rails blank? method here.

Yes.
Discovery issuer will be only used only discovery is enabled and options issuer was blank?

Is this intentional change? Old code has different behaviour, I'm not sure about this.

Another point, I'm wondering if you prefer that I avoid using rails blank? method here.

Yeah, good point, it'd be great to avoid rails' methods.

Ok I get it. You're right I've modified the behavior around the issuer.
Well it's not the purpose of the PR, so I've decided to keep the original implementation.

m0n9oose · 2019-01-04T17:53:11Z

lib/omniauth/strategies/openid_connect.rb

@@ -111,10 +111,25 @@ def callback_phase
        fail!(:failed_to_connect, e)
      end

+      def other_phase
+        if logout_path_pattern.match(current_path)


Please consider the following changes:

def other_phase return call_app! unless logout_path_pattern.match(current_path) discover! redirect(end_session_uri) if end_session_uri end

call_app! must be called when end_session_uri is falsy

I could change as the following (could be harder to read with multiple return and call_app! calls)

def other_phase return call_app! unless logout_path_pattern.match(current_path) discover! return redirect(end_session_uri) if end_session_uri call_app! end

m0n9oose · 2019-01-04T17:53:25Z

lib/omniauth/strategies/openid_connect.rb

-        client_options.jwks_uri = config.jwks_uri
+        return unless options.discovery
+        options.issuer = issuer if options.issuer.blank?
+        setup_client_options(config)


what's the point to pass config as argument?

what's the point of extraction setup_client_options method? how does it pay for itself?

please avoid using Rails methods in gems which can be used outside of Rails project (try)

Consider the following changes:

def discover! return unless options.discovery 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) end

Totally agree, I can't explain why I did that.
Thanks for your feedback.

m0n9oose · 2019-01-04T17:55:10Z

Please rebase your work branch on top of current master branch

ghost · 2019-01-17T13:54:18Z

test/lib/omniauth/strategies/openid_connect_test.rb

@@ -18,6 +18,63 @@ def test_request_phase
        strategy.request_phase
      end

+      def test_logout_phase_with_discovery


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

ghost · 2019-01-17T13:54:18Z

test/lib/omniauth/strategies/openid_connect_test.rb

@@ -18,6 +18,63 @@ def test_request_phase
        strategy.request_phase
      end

+      def test_logout_phase_with_discovery


Method has too many lines. [16/10]

ghost · 2019-01-17T13:54:18Z

test/lib/omniauth/strategies/openid_connect_test.rb

+        strategy.other_phase
+      end
+
+      def test_logout_phase_with_discovery_and_post_logout_redirect_uri


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

ghost · 2019-01-17T13:54:18Z

test/lib/omniauth/strategies/openid_connect_test.rb

+        strategy.other_phase
+      end
+
+      def test_logout_phase_with_discovery_and_post_logout_redirect_uri


Method has too many lines. [17/10]

ghost · 2019-01-17T13:54:18Z

lib/omniauth/strategies/openid_connect.rb

      def authorization_code
        request.params['code']
      end

+      def end_session_uri
+        return unless end_session_endpoint_is_valid?


Add empty line after guard clause.

ghost · 2019-01-17T13:54:18Z

lib/omniauth/strategies/openid_connect.rb

@@ -143,10 +158,13 @@ def issuer
      end

      def discover!
+        return unless options.discovery


Add empty line after guard clause.

ghost · 2019-01-17T13:54:18Z

lib/omniauth/strategies/openid_connect.rb

        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)


Line is too long. [118/80]

ghost · 2019-01-17T13:54:18Z

lib/omniauth/strategies/openid_connect.rb

@@ -235,6 +253,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


Add empty line after guard clause.

m0n9oose · 2019-01-17T20:16:56Z

lib/omniauth/strategies/openid_connect.rb

-        client_options.userinfo_endpoint = config.userinfo_endpoint
-        client_options.jwks_uri = config.jwks_uri
+        return unless options.discovery
+        options.issuer = issuer if options.issuer.blank?


Yes.
Discovery issuer will be only used only discovery is enabled and options issuer was blank?

Is this intentional change? Old code has different behaviour, I'm not sure about this.

Another point, I'm wondering if you prefer that I avoid using rails blank? method here.

Yeah, good point, it'd be great to avoid rails' methods.

m0n9oose · 2019-01-17T20:32:15Z

lib/omniauth/strategies/openid_connect.rb

@@ -111,10 +113,27 @@ def callback_phase
        fail!(:failed_to_connect, e)
      end

+      def other_phase


#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.

ghost · 2019-02-01T19:01:35Z

lib/omniauth/strategies/openid_connect.rb

@@ -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?


Use 2 (not 0) spaces for indentation.

ghost · 2019-02-01T19:01:35Z

lib/omniauth/strategies/openid_connect.rb

-          options.issuer = issuer if options.issuer.blank?
-          discover! if options.discovery
+        options.issuer = issuer if options.issuer.nil? || options.issuer.empty?
+          discover!


Inconsistent indentation detected.

ghost · 2019-02-01T19:01:35Z

lib/omniauth/strategies/openid_connect.rb

@@ -111,10 +113,26 @@ def callback_phase
        fail!(:failed_to_connect, e)
      end

+      def other_phase


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

ghost · 2019-02-01T19:01:35Z

lib/omniauth/strategies/openid_connect.rb

@@ -111,10 +113,26 @@ def callback_phase
        fail!(:failed_to_connect, e)
      end

+      def other_phase
+        if logout_path_pattern.match?(current_path)
+          options.issuer = issuer if options.issuer.nil? || options.issuer.empty?


Line is too long. [81/80]

ghost · 2019-02-01T19:01:35Z

lib/omniauth/strategies/openid_connect.rb

      def authorization_code
        request.params['code']
      end

+      def end_session_uri
+        return unless end_session_endpoint_is_valid?


Add empty line after guard clause.

ghost · 2019-02-01T19:01:35Z

lib/omniauth/strategies/openid_connect.rb

@@ -143,10 +161,12 @@ def issuer
      end

      def discover!
+        return unless options.discovery


Add empty line after guard clause.

ghost · 2019-02-01T19:01:35Z

lib/omniauth/strategies/openid_connect.rb

        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)


Line is too long. [118/80]

ghost · 2019-02-01T19:01:35Z

lib/omniauth/strategies/openid_connect.rb

@@ -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


Add empty line after guard clause.

ghost · 2019-02-01T19:03:49Z

lib/omniauth/strategies/openid_connect.rb

@@ -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?


Line is too long. [81/80]

yannvery · 2019-02-01T19:08:52Z

Sorry, for the delay of my response.
I prefer to not fix all the bot issues and avoid making too many modifications in 1 PR.

m0n9oose · 2019-02-01T19:22:37Z

Yeah, makes sense

…

-- Best regards, Ilia Shcherbinin

On 1 February 2019 at 20:08:53, Yann VERY ***@***.***) wrote: Sorry, for the delay of my response. I prefer to not fix all the bot issues and avoid making too many modifications in 1 PR. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#5 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABaPnOoaDB8lYlnNn7aLPVO_FCurtW2Xks5vJJDFgaJpZM4WLDk5> .

ghost · 2019-02-02T15:07:08Z

test/lib/omniauth/strategies/openid_connect_test.rb

@@ -18,6 +18,63 @@ def test_request_phase
        strategy.request_phase
      end

+      def test_logout_phase_with_discovery


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

ghost · 2019-02-02T15:07:09Z

test/lib/omniauth/strategies/openid_connect_test.rb

@@ -18,6 +18,63 @@ def test_request_phase
        strategy.request_phase
      end

+      def test_logout_phase_with_discovery


Method has too many lines. [16/10]

ghost · 2019-02-02T15:07:09Z

test/lib/omniauth/strategies/openid_connect_test.rb

+        strategy.other_phase
+      end
+
+      def test_logout_phase_with_discovery_and_post_logout_redirect_uri


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

ghost · 2019-02-02T15:07:09Z

test/lib/omniauth/strategies/openid_connect_test.rb

+        strategy.other_phase
+      end
+
+      def test_logout_phase_with_discovery_and_post_logout_redirect_uri


Method has too many lines. [17/10]

ghost · 2019-02-02T15:07:09Z

lib/omniauth/strategies/openid_connect.rb

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


Line is too long. [81/80]

ghost · 2019-02-02T15:07:09Z

lib/omniauth/strategies/openid_connect.rb

@@ -114,10 +116,26 @@ def callback_phase
        fail!(:failed_to_connect, e)
      end

+      def other_phase
+        if logout_path_pattern.match?(current_path)
+          options.issuer = issuer if options.issuer.nil? || options.issuer.empty?


Line is too long. [81/80]

ghost · 2019-02-02T15:07:09Z

lib/omniauth/strategies/openid_connect.rb

      def authorization_code
        params['code']
      end

+      def end_session_uri
+        return unless end_session_endpoint_is_valid?


Add empty line after guard clause.

ghost · 2019-02-02T15:07:09Z

lib/omniauth/strategies/openid_connect.rb

@@ -148,10 +166,12 @@ def issuer
      end

      def discover!
+        return unless options.discovery


Add empty line after guard clause.

ghost · 2019-02-02T15:07:09Z

lib/omniauth/strategies/openid_connect.rb

        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)


Line is too long. [118/80]

ghost · 2019-02-02T15:07:10Z

lib/omniauth/strategies/openid_connect.rb

@@ -240,6 +260,22 @@ def redirect_uri
        "#{ client_options.redirect_uri }?redirect_uri=#{ CGI.escape(params['redirect_uri']) }"
      end

+      def encoded_post_logout_redirect_uri
+        return unless options.post_logout_redirect_uri


Add empty line after guard clause.

Bump openid_connect to 1.1.6

ghost reviewed Aug 24, 2018

View reviewed changes

yannvery force-pushed the logout-endpoint branch 2 times, most recently from 1c2d23a to 4df356a Compare August 24, 2018 11:36

ghost reviewed Sep 7, 2018

View reviewed changes

yannvery force-pushed the logout-endpoint branch from 0873500 to 92ed82c Compare September 7, 2018 09:52

m0n9oose requested changes Jan 4, 2019

View reviewed changes

yannvery added 2 commits January 16, 2019 17:07

Add logout phase

d57beb4

Avoid to make a discover for each other_phase call

3ae8b9a

yannvery force-pushed the logout-endpoint branch from 92ed82c to 96b97fe Compare January 17, 2019 13:54

ghost reviewed Jan 17, 2019

View reviewed changes

m0n9oose requested changes Jan 17, 2019

View reviewed changes

yannvery force-pushed the logout-endpoint branch 2 times, most recently from 7708f34 to 834a062 Compare February 1, 2019 19:01

ghost reviewed Feb 1, 2019

View reviewed changes

avoid using match when MatchData is not used

694fc90

yannvery force-pushed the logout-endpoint branch from 834a062 to 694fc90 Compare February 1, 2019 19:03

ghost reviewed Feb 1, 2019

View reviewed changes

Merge branch 'master' into logout-endpoint

45987d8

ghost reviewed Feb 2, 2019

View reviewed changes

m0n9oose approved these changes Feb 2, 2019

View reviewed changes

m0n9oose merged commit 4c024ea into omniauth:master Feb 2, 2019

Eric-Guo pushed a commit to Eric-Guo/omniauth_openid_connect that referenced this pull request Dec 28, 2021

Merge pull request omniauth#5 from shawnfrench/shawnfrench-patch-1

2ab0b40

Bump openid_connect to 1.1.6

RP-Initiated Logout phase #5

RP-Initiated Logout phase #5

Conversation

yannvery commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m0n9oose commented Jan 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yannvery commented Feb 1, 2019

m0n9oose commented Feb 1, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment