fix: make require_state skip verification of state

In #127,
`require_state` was introduced because according to
https://openid.net/specs/openid-connect-core-1_0.html#rfc.section.3.1.2.1,
`state` is recommended but not required:

```
state
    RECOMMENDED. Opaque value used to maintain state between the
    request and the callback. Typically, Cross-Site Request Forgery
    (CSRF, XSRF) mitigation is done by cryptographically binding the
    value of this parameter with a browser cookie.
```

During review, the `require_state` parameter was modified to verify
`state` as long as `stored_state` was present. However, `stored_state`
always holds at least a random value, so when `require_state` were
`false` and if an OpenID provider did not relay the `state` value,
authentication would halt with a "Invalid 'state' parameter" error.

This commit updates it so that if `require_state` is set to `false`,
the `state` parameter is never checked at all.

Author: stanhu

lib/omniauth/strategies/openid_connect.rb

@@ -120,7 +120,7 @@ def request_phase
       def callback_phase
         error = params['error_reason'] || params['error']
         error_description = params['error_description'] || params['error_reason']
-        invalid_state = (options.require_state && params['state'].to_s.empty?) || params['state'] != stored_state
+        invalid_state = options.require_state && (params['state'].to_s.empty? || params['state'] != stored_state)
 
         raise CallbackError, error: params['error'], reason: error_description, uri: params['error_uri'] if error
         raise CallbackError, error: :csrf_detected, reason: "Invalid 'state' parameter" if invalid_state

test/lib/omniauth/strategies/openid_connect_test.rb

@@ -501,12 +501,43 @@ def test_callback_phase_with_invalid_state_without_state_verification
         state = SecureRandom.hex(16)
 
         strategy.options.require_state = false
+        strategy.options.discovery = false
 
         request.stubs(:params).returns('code' => code, 'state' => 'foobar')
         request.stubs(:path).returns('')
 
+        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(:jwks).returns(JSON::JWK::Set.new(jwks['keys']))
+
+        ::OpenIDConnect::Discovery::Provider::Config.stubs(:discover!).with('https://example.com/').returns(config)
+
+        id_token = stub('OpenIDConnect::ResponseObject::IdToken')
+        id_token.stubs(:raw_attributes).returns('sub' => 'sub', 'name' => 'name', 'email' => 'email')
+        id_token.stubs(:verify!).with(issuer: 'https://example.com/', client_id: @identifier, nonce: nonce).returns(true)
+        ::OpenIDConnect::ResponseObject::IdToken.stubs(:decode).returns(id_token)
+
+        strategy.unstub(:user_info)
+        access_token = stub('OpenIDConnect::AccessToken')
+        access_token.stubs(:access_token)
+        access_token.stubs(:refresh_token)
+        access_token.stubs(:expires_in)
+        access_token.stubs(:scope)
+        access_token.stubs(:id_token).returns(jwt.to_s)
+        client.expects(:access_token!).at_least_once.returns(access_token)
+        access_token.expects(:userinfo!).returns(user_info)
+
         strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce })
-        strategy.expects(:fail!)
         strategy.callback_phase
       end