From ae60231d87222fd661b79a81db8aa5c823b41e6b Mon Sep 17 00:00:00 2001 From: Ben Campbell Date: Mon, 15 Oct 2018 16:26:46 -0600 Subject: [PATCH] Support Microsoft apps on v2 endpoint --- .rubocop.yml | 6 + .../strategies/azure_activedirectory.rb | 214 +++++++++--------- omniauth-azure-activedirectory.gemspec | 3 +- spec/fixtures/id_token.txt | 2 +- spec/fixtures/id_token_bad_audience.txt | 2 +- spec/fixtures/id_token_bad_chash.txt | 2 +- spec/fixtures/id_token_bad_issuer.txt | 2 +- spec/fixtures/id_token_bad_kid.txt | 2 +- spec/fixtures/id_token_bad_nonce.txt | 2 +- spec/fixtures/id_token_no_alg.txt | 2 +- spec/fixtures/id_token_with_chash.txt | 1 + spec/fixtures/x5c.txt | 2 +- .../strategies/azure_activedirectory_spec.rb | 148 ++++++------ 13 files changed, 203 insertions(+), 185 deletions(-) create mode 100644 spec/fixtures/id_token_with_chash.txt diff --git a/.rubocop.yml b/.rubocop.yml index 9bbc2a4..dbc4993 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -4,5 +4,11 @@ AllCops: Exclude: - 'spec/fixtures/**/*' +Metrics/LineLength: + Enabled: false + +Metrics/BlockLength: + Enabled: false + Style/Encoding: Enabled: false diff --git a/lib/omniauth/strategies/azure_activedirectory.rb b/lib/omniauth/strategies/azure_activedirectory.rb index 20339c5..bfeba93 100644 --- a/lib/omniauth/strategies/azure_activedirectory.rb +++ b/lib/omniauth/strategies/azure_activedirectory.rb @@ -21,81 +21,96 @@ #------------------------------------------------------------------------------- require 'jwt' -require 'omniauth' +require 'omniauth/strategies/oauth2' require 'openssl' require 'securerandom' module OmniAuth module Strategies # A strategy for authentication against Azure Active Directory. - class AzureActiveDirectory + # rubocop:disable Metrics/ClassLength + class AzureActiveDirectory < OmniAuth::Strategies::OAuth2 include OmniAuth::AzureActiveDirectory - include OmniAuth::Strategy - class OAuthError < StandardError; end + BASE_SCOPES = %w[openid profile email].freeze + DEFAULT_RESPONSE_MODE = 'form_post'.freeze + DEFAULT_RESPONSE_TYPE = 'code id_token'.freeze + DEFAULT_TENANT = 'common'.freeze + + option :name, 'azure_activedirectory' - ## - # The client id (key) and tenant must be configured when the OmniAuth - # middleware is installed. Example: - # - # require 'omniauth' - # require 'omniauth-azure-activedirectory' - # - # use OmniAuth::Builder do - # provider :azure_activedirectory, ENV['AAD_KEY'], ENV['AAD_TENANT'] - # end - # - args [:client_id, :tenant] option :client_id, nil - option :tenant, nil + option :client_secret, nil + option :response_mode, DEFAULT_RESPONSE_MODE + option :response_type, DEFAULT_RESPONSE_TYPE + option :scope, BASE_SCOPES.join(' ') + option :tenant, DEFAULT_TENANT + option :verify_iss, false + + credentials { { code: @code } } + uid { claims['sub'] } - # Field renaming is an attempt to fit the OmniAuth recommended schema as - # best as possible. - # - # @see https://github.com/intridea/omniauth/wiki/Auth-Hash-Schema - uid { @claims['sub'] } info do - { name: @claims['name'], - email: @claims['email'] || @claims['upn'], - first_name: @claims['given_name'], - last_name: @claims['family_name'] } + { + # Since name is not always present, default to oid because `name` is + # required for an auth_hash to be valid. The name should not be used + # for anything except display. + name: claims['name'] || claims['oid'], + # Email is only included in personal accounts requests. For sign-ins + # with an AD account, the preferred_username key might be an email, + # but not always; it could be a phone number or just a generic + # username. + email: claims['email'], + preferred_username: claims['preferred_username'], + oid: claims['oid'], + tid: claims['tid'] + } end - credentials { { code: @code } } extra do { session_state: @session_state, raw_info: { id_token: @id_token, - id_token_claims: @claims, - id_token_header: @header } } + id_token_claims: claims, + id_token_header: header } } end - DEFAULT_RESPONSE_TYPE = 'code id_token' - DEFAULT_RESPONSE_MODE = 'form_post' + uid { claims['sub'] } - ## - # Overridden method from OmniAuth::Strategy. This is the first step in the - # authentication process. - def request_phase - redirect authorize_endpoint_url + def client + options.client_options.authorize_url = URI(openid_config['authorization_endpoint']) + options.client_options.token_url = URI(openid_config['token_endpoint']) + + super end - ## - # Overridden method from OmniAuth::Strategy. This is the second step in - # the authentication process. It is called after the user enters - # credentials at the authorization endpoint. - def callback_phase - error = request.params['error_reason'] || request.params['error'] - fail(OAuthError, error) if error - @session_state = request.params['session_state'] - @id_token = request.params['id_token'] - @code = request.params['code'] - @claims, @header = validate_and_parse_id_token(@id_token) - validate_chash(@code, @claims, @header) + def callback_url + full_host + script_name + callback_path + end + + def authorize_params + options.authorize_params[:nonce] = new_nonce + options.authorize_params[:response_mode] = options.response_mode + options.authorize_params[:response_type] = options.response_type + super end private + def claims + @claims ||= decoded_token[0] + end + + def header + @header ||= decoded_token[1] + end + + def decoded_token + @session_state = request.params['session_state'] + @id_token = access_token.params['id_token'] + @decoded_token ||= validate_and_parse_id_token(@id_token) + end + ## # Constructs a one-time-use authorize_endpoint. This method will use # a new nonce on each invocation. @@ -105,8 +120,6 @@ def authorize_endpoint_url uri = URI(openid_config['authorization_endpoint']) uri.query = URI.encode_www_form(client_id: client_id, redirect_uri: callback_url, - response_mode: response_mode, - response_type: response_type, nonce: new_nonce) uri.to_s end @@ -118,15 +131,8 @@ def authorize_endpoint_url # @return String def client_id return options.client_id if options.client_id - fail StandardError, 'No client_id specified in AzureAD configuration.' - end - ## - # The expected id token issuer taken from the discovery endpoint. - # - # @return String - def issuer - openid_config['issuer'] + raise StandardError, 'No client_id specified in AzureAD configuration.' end ## @@ -166,12 +172,23 @@ def fetch_signing_keys # # @return Hash def fetch_openid_config - JSON.parse(Net::HTTP.get(URI(openid_config_url))) + JSON.parse( + Net::HTTP.get( + URI(openid_config_url) + ) + ) rescue JSON::ParserError raise StandardError, 'Unable to fetch OpenId configuration for ' \ 'AzureAD tenant.' end + # The expected id token issuer taken from the discovery endpoint. + # + # @return String + def issuer + openid_config['issuer'] + end + ## # Generates a new nonce for one time use. Stores it in the session so # multiple users don't share nonces. All nonces should be generated by @@ -195,7 +212,7 @@ def openid_config # # @return String def openid_config_url - "https://login.windows.net/#{tenant}/.well-known/openid-configuration" + "https://login.microsoftonline.com/#{options.tenant}/v2.0/.well-known/openid-configuration/" end ## @@ -207,26 +224,6 @@ def read_nonce session.delete('omniauth-azure-activedirectory.nonce') end - ## - # The response_type that will be set in the authorization request query - # parameters. Can be overridden by the client, but it shouldn't need to - # be. - # - # @return String - def response_type - options[:response_type] || DEFAULT_RESPONSE_TYPE - end - - ## - # The response_mode that will be set in the authorization request query - # parameters. Can be overridden by the client, but it shouldn't need to - # be. - # - # @return String - def response_mode - options[:response_mode] || DEFAULT_RESPONSE_MODE - end - ## # The keys used to sign the id token JWTs. This is just a memoized version # of #fetch_signing_keys. @@ -243,17 +240,8 @@ def signing_keys # @return String def signing_keys_url return openid_config['jwks_uri'] if openid_config.include? 'jwks_uri' - fail StandardError, 'No jwks_uri in OpenId config response.' - end - ## - # The tenant of the calling application. Note that this must be - # explicitly configured when installing the AzureAD OmniAuth strategy. - # - # @return String - def tenant - return options.tenant if options.tenant - fail StandardError, 'No tenant specified in AzureAD configuration.' + raise StandardError, 'No jwks_uri in OpenId config response.' end ## @@ -263,14 +251,11 @@ def tenant # See OpenId Connect Core 3.1.3.7 and 3.2.2.11. # # @return Claims, Header + # + # rubocop:disable Metrics/MethodLength + # rubocop:disable Metrics/AbcSize def validate_and_parse_id_token(id_token) - # The second parameter is the public key to verify the signature. - # However, that key is overridden by the value of the executed block - # if one is present. - # - # If you're thinking that this looks ugly with the raw nil and boolean, - # see https://github.com/jwt/ruby-jwt/issues/59. - jwt_claims, jwt_header = + claims, token_header = JWT.decode(id_token, nil, true, verify_options) do |header| # There should always be one key from the discovery endpoint that # matches the id in the JWT header. @@ -278,16 +263,22 @@ def validate_and_parse_id_token(id_token) key['kid'] == header['kid'] end || {})['x5c'] if x5c.nil? || x5c.empty? - fail JWT::VerificationError, - 'No keys from key endpoint match the id token' + raise JWT::VerificationError, 'No keys from key endpoint match the id token' end + # The key also contains other fields, such as n and e, that are # redundant. x5c is sufficient to verify the id token. OpenSSL::X509::Certificate.new(JWT.base64url_decode(x5c.first)).public_key end - return jwt_claims, jwt_header if jwt_claims['nonce'] == read_nonce - fail JWT::DecodeError, 'Returned nonce did not match.' + + @code = request.params['code'] + validate_chash(@code, claims, token_header) + return claims, token_header if claims['nonce'] == read_nonce + + raise JWT::DecodeError, 'Returned nonce did not match.' end + # rubocop:enable Metrics/MethodLength + # rubocop:enable Metrics/AbcSize ## # Verifies that the c_hash the id token claims matches the authorization @@ -297,13 +288,15 @@ def validate_and_parse_id_token(id_token) # @param Hash claims # @param Hash header def validate_chash(code, claims, header) + return if claims['c_hash'].nil? + # This maps RS256 -> sha256, ES384 -> sha384, etc. algorithm = (header['alg'] || 'RS256').sub(/RS|ES|HS/, 'sha') full_hash = OpenSSL::Digest.new(algorithm).digest code c_hash = JWT.base64url_encode full_hash[0..full_hash.length / 2 - 1] return if c_hash == claims['c_hash'] - fail JWT::VerificationError, - 'c_hash in id token does not match auth code.' + + raise JWT::VerificationError, 'c_hash in id token does not match auth code.' end ## @@ -314,15 +307,18 @@ def validate_chash(code, claims, header) # # @return Hash def verify_options - { verify_expiration: true, - verify_not_before: true, - verify_iat: true, - verify_iss: true, - 'iss' => issuer, + { + aud: client.id, + iss: issuer, verify_aud: true, - 'aud' => client_id } + verify_expiration: true, + verify_iat: true, + verify_iss: options.verify_iss, + verify_not_before: true + } end end + # rubocop:enable Metrics/ClassLength end end diff --git a/omniauth-azure-activedirectory.gemspec b/omniauth-azure-activedirectory.gemspec index 5fd46ad..b54d1df 100644 --- a/omniauth-azure-activedirectory.gemspec +++ b/omniauth-azure-activedirectory.gemspec @@ -16,10 +16,11 @@ Gem::Specification.new do |s| s.add_runtime_dependency 'jwt', '~> 1.5' s.add_runtime_dependency 'omniauth', '~> 1.1' + s.add_runtime_dependency 'omniauth-oauth2', '~> 1.4' s.add_development_dependency 'rake', '~> 10.4' s.add_development_dependency 'rspec', '~> 3.3' s.add_development_dependency 'rubocop', '~> 0.32' s.add_development_dependency 'simplecov', '~> 0.10' - s.add_development_dependency 'webmock', '~> 1.21' + s.add_development_dependency 'webmock', '~> 2.3' end diff --git a/spec/fixtures/id_token.txt b/spec/fixtures/id_token.txt index a918b7a..a72aab2 100644 --- a/spec/fixtures/id_token.txt +++ b/spec/fixtures/id_token.txt @@ -1 +1 @@ -eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6ImFiYzEyMyJ9.eyJpc3MiOiJodHRwczovL3N0cy53aW5kb3dzLm5ldC9idW5jaC1vZi1yYW5kb20tY2hhcnMiLCJuYW1lIjoiSm9obiBTbWl0aCIsImF1ZCI6InRoZSBjbGllbnQgaWQiLCJub25jZSI6Im15IG5vbmNlIiwiZ2l2ZW5fbmFtZSI6IkpvaG4iLCJmYW1pbHlfbmFtZSI6InNtaXRoIiwiZW1haWwiOiJqc21pdGhAY29udG9zby5jb20iLCJjX2hhc2giOiJWcFRRaWk1VF84cmd3eEEtV3RiMkJ3In0.Xz9SL1dm9xeJ3YtBIwSpL7SHEMr5lsL32mkJIugoAt7rNhj2ZauN77_N3skU9FIRTVb_XBFHrLo1AXion7RWoOGAMk8xnuQRlhamGoWsjttWE9oO6J6kuQPSDBvLTA88UqLoGNezDwFNfgUFQq-66m33ZWdkiNOFoFZ_In6DtAwxHZZUys-KoYD3iCbviUoBzU57aV-SBsWyComq39pDGpw03qZoa_xgRfujdVHG1DKlO5VG79kUE3ySYWJyBYVKUdAzjH1iotjpPA1svtqytn4CUldAMi4nnf7iq5SCJMb4ucu0mN6AhJH9ktY--fGY6_OidyiDe4F57ZzLw-3jOQ \ No newline at end of file +eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6ImFiYzEyMyJ9.eyJpc3MiOiJodHRwczovL3N0cy53aW5kb3dzLm5ldC9idW5jaC1vZi1yYW5kb20tY2hhcnMiLCJuYW1lIjoiSm9obiBTbWl0aCIsImF1ZCI6InRoZSBjbGllbnQgaWQiLCJub25jZSI6Im15IG5vbmNlIiwiZW1haWwiOiJqc21pdGhAY29udG9zby5jb20iLCJwcmVmZXJyZWRfdXNlcm5hbWUiOiJqc21pdGhAY29udG9zby5jb20iLCJ2ZXIiOiIyLjAifQ.eFqQS4R9OVqorc4qSY9bOvsxmmL3OQJ96IHD9JD6Y6jiD_YTAifNP-FahOqRZjqh7bCdU1XUnvKM7RC0z_CRFkHkOJ9bzV0iTsBYSpeX9v4QUSAhtxKvcrAufCyT-NOViv4WvvnYbZDQRO7BxU80OI9poSgkw8rKST28EA-cB2c diff --git a/spec/fixtures/id_token_bad_audience.txt b/spec/fixtures/id_token_bad_audience.txt index 023cc20..38a3c56 100644 --- a/spec/fixtures/id_token_bad_audience.txt +++ b/spec/fixtures/id_token_bad_audience.txt @@ -1 +1 @@ -eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6ImFiYzEyMyJ9.eyJpc3MiOiJodHRwczovL3N0cy53aW5kb3dzLm5ldC9idW5jaC1vZi1yYW5kb20tY2hhcnMiLCJuYW1lIjoiSm9obiBTbWl0aCIsImF1ZCI6Im5vdCB0aGUgY2xpZW50IGlkIiwibm9uY2UiOiJteSBub25jZSIsImdpdmVuX25hbWUiOiJKb2huIiwiZmFtaWx5X25hbWUiOiJzbWl0aCIsImVtYWlsIjoianNtaXRoQGNvbnRvc28uY29tIn0.gGCFTcGDBb2P5tPRj2ojUUiwOJoSslQlxEVTElZY6FCzCZzcypsnrYOB9Adkztp2AWF4wW9fT58lqXwaahKquXtK4wyK4KoNBxXBhS4sDMeNpVkK4914tT_6gecvyUsI_tlJaS0epd5c0mN9-1QjgvNEirY1L-XYaT290LmLYVYIFTEJXoSlnwvv081k0txdJZKr14JXd_bSLUbhGSd-NcGZkJuVmg2F_C65zd1wUsQOkV2iVdJ-ycaDJklv8-DFfDFIHQio8S9yqyieHwArRmTW9HzcoHhWBh2MIItszoTSbmQEF062NtNBPW8gyk1OSot5X9klJUhgPAAFqJ0TJQ \ No newline at end of file +eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6ImFiYzEyMyJ9.eyJpc3MiOiJodHRwczovL3N0cy53aW5kb3dzLm5ldC9idW5jaC1vZi1yYW5kb20tY2hhcnMiLCJuYW1lIjoiSm9obiBTbWl0aCIsImF1ZCI6Im5vdCB0aGUgY2xpZW50IGlkIiwibm9uY2UiOiJteSBub25jZSIsImVtYWlsIjoianNtaXRoQGNvbnRvc28uY29tIiwicHJlZmVycmVkX3VzZXJuYW1lIjoianNtaXRoQGNvbnRvc28uY29tIiwidmVyIjoiMi4wIn0.e9s97ndTZw8umsfsaLntzzjIEyiIRGRTa9mvBiYEN-rddxlkhunN2VzHmlG_7W5D8yHkwMFKrvNnF2ciw5UcMfwzL3bBDaZOm_eyl-XGVOG93407sjEU7kjUerfmiSstGFIUqw-xtBa5uAV7rpbhSBn71sS234xvUsRb7x6Vs6s diff --git a/spec/fixtures/id_token_bad_chash.txt b/spec/fixtures/id_token_bad_chash.txt index cef7c9e..2d17198 100644 --- a/spec/fixtures/id_token_bad_chash.txt +++ b/spec/fixtures/id_token_bad_chash.txt @@ -1 +1 @@ -eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6ImFiYzEyMyJ9.eyJpc3MiOiJodHRwczovL3N0cy53aW5kb3dzLm5ldC9idW5jaC1vZi1yYW5kb20tY2hhcnMiLCJuYW1lIjoiSm9obiBTbWl0aCIsImF1ZCI6InRoZSBjbGllbnQgaWQiLCJub25jZSI6Im15IG5vbmNlIiwiZ2l2ZW5fbmFtZSI6IkpvaG4iLCJmYW1pbHlfbmFtZSI6InNtaXRoIiwiZW1haWwiOiJqc21pdGhAY29udG9zby5jb20iLCJjX2hhc2giOiI1WEhDdk9qcUgtbnJBXzNXNUJYaWJnIn0.EHPFrxMbr2EH1IZ15F3oP1yvrukadY4ggZSOxm625qPoMxfhv-QzFm0YMhkAG0cLM_LSHi_RgedDwTGuzOtIWqmheYzydnhtbIBeKmx0RSgdob6WeiTZwJ93VRiV4q82Qda41JaaIl_wdWd4lVyVstd9o9jPYMtKEVLgaNDrtHt6pPxEGCVraiaM6tVyKc5XIHu3wNaqle5UZZREU6oirwTCUhXDZkz1g5qY2-aWUdfFVsElSarJuMcxGDPD20hvx7T3D7SCHAF8WhKw3AwQyrodKWCo3cqDOz6vHymb_-ELkJc14GMbtJiyrf6XYySZZoseoI_WBDmLfKcK637xCw \ No newline at end of file +eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6ImFiYzEyMyJ9.eyJpc3MiOiJodHRwczovL3N0cy53aW5kb3dzLm5ldC9idW5jaC1vZi1yYW5kb20tY2hhcnMiLCJuYW1lIjoiSm9obiBTbWl0aCIsImF1ZCI6InRoZSBjbGllbnQgaWQiLCJub25jZSI6Im15IG5vbmNlIiwiZW1haWwiOiJqc21pdGhAY29udG9zby5jb20iLCJwcmVmZXJyZWRfdXNlcm5hbWUiOiJqc21pdGhAY29udG9zby5jb20iLCJ2ZXIiOiIyLjAiLCJjX2hhc2giOiJWcFRRaWk1VF84cmd3eEEtV3RiMkIyIn0.L5S6cTvXrzFCBX_wXPUfLJqnSX9GZ3LXyvW8U63sGo_RjPNNBsbAaUZ8FlUUDF_U4HMTh2Q1BHK8UxXDC7K1JtpvoytpXSkJ7TPJqpdFLQ6xtrkvKiR1StIwEK-NcbHdS4Rta3Lqwrc3_vUGa_4DNUARHeqvjzw-nG0AVaTIlLs diff --git a/spec/fixtures/id_token_bad_issuer.txt b/spec/fixtures/id_token_bad_issuer.txt index 1d11c38..936dd9d 100644 --- a/spec/fixtures/id_token_bad_issuer.txt +++ b/spec/fixtures/id_token_bad_issuer.txt @@ -1 +1 @@ -eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6ImFiYzEyMyJ9.eyJpc3MiOiJodHRwczovL3N0cy5pbXBvc3Rlci5uZXQvYnVuY2gtb2YtcmFuZG9tLWNoYXJzIiwibmFtZSI6IkpvaG4gU21pdGgiLCJhdWQiOiJ0aGUgY2xpZW50IGlkIiwibm9uY2UiOiJteSBub25jZSIsImdpdmVuX25hbWUiOiJKb2huIiwiZmFtaWx5X25hbWUiOiJzbWl0aCIsImVtYWlsIjoianNtaXRoQGNvbnRvc28uY29tIn0.fwnJuRsif_Td3MfXoyADHidYJyPFdWSBBoLbVAu4Lz3-pmSln9Vgxl5KowqEKq2LX5n0aqyWVGLcoT-_G_PNXuizuNmssv5vreKLvDMpsFXt2irdwGYDCRki7KQPBk3bn12YjBzE2EqiRy7dTEG_0vWoh1RqoNP72BBL8xYQUlIOFleZhT5KGNYbh7rvcmDq7aA-xdaXT3QJfHCHpitW_zVzZ5Gok_awcdx_v3r3eFbG2IT0PfmT40Ljia0aP2i60KgsOLLHarYO51KFNDEfr1pUDf4IweaEzstbVLwk-_5ulJ5QgByhNJrmWDfrGeCQRk0SO2XX-EUsVn5ySVJ5CQ \ No newline at end of file +eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6ImFiYzEyMyJ9.eyJpc3MiOiJodHRwczovL3N0cy5pbXBvc3Rlci5uZXQvYnVuY2gtb2YtcmFuZG9tLWNoYXJzIiwibmFtZSI6IkpvaG4gU21pdGgiLCJhdWQiOiJ0aGUgY2xpZW50IGlkIiwibm9uY2UiOiJteSBub25jZSIsImVtYWlsIjoianNtaXRoQGNvbnRvc28uY29tIiwicHJlZmVycmVkX3VzZXJuYW1lIjoianNtaXRoQGNvbnRvc28uY29tIiwidmVyIjoiMi4wIn0.jkUJ33TIcZ4kyXEB86cn-smKc8J_ROB6QK-u1QgzTTFZDPha-VJKLOZlZChsTXAq8LSlGIEGJAnqFxIS-Rm9nbBQK70v5YD7P0lFb1YvLwIbW1d764_bG_oAj7jPR7i9boyD7Udvivr19mIHFF6iSAQJV26o3UFa6pXsiZNvlUQ diff --git a/spec/fixtures/id_token_bad_kid.txt b/spec/fixtures/id_token_bad_kid.txt index 672527b..4cf56a4 100644 --- a/spec/fixtures/id_token_bad_kid.txt +++ b/spec/fixtures/id_token_bad_kid.txt @@ -1 +1 @@ -eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6ImFiYzEyMzQifQ.eyJpc3MiOiJodHRwczovL3N0cy53aW5kb3dzLm5ldC9idW5jaC1vZi1yYW5kb20tY2hhcnMiLCJuYW1lIjoiSm9obiBTbWl0aCIsImF1ZCI6InRoZSBjbGllbnQgaWQiLCJub25jZSI6Im15IG5vbmNlIiwiZ2l2ZW5fbmFtZSI6IkpvaG4iLCJmYW1pbHlfbmFtZSI6InNtaXRoIiwiZW1haWwiOiJqc21pdGhAY29udG9zby5jb20iLCJjX2hhc2giOiI1WEhDdk9qcUgtbnJBXzNXNUJYaWJnIn0.gWGBc9rH30SN17Ikm1CjqIYAyzFHX0yeRQu85sVYLE3r5k26bjS3R6rTJcCQlYqHPRdoPcnkUgT1QVbdThw34ICrODoavs7I5QYEn_jKP9zM4UJEKQaCLBAtitzrk1KDEf0GLcNKif-MYu7MiQfoOzwCGWfIs-vgqk4lv0JUs9OlSLp5LHru7G3jKy6Qswbrpxpjzm9I8BnKEdhUfhz4P6wIf9KLMmhgeGQdQ6wBuxPmOf9r6EKIij2AENhFp1qP90m8kXq9tIt5FZFjwIs_G6spLl0gQXyx0qC8rP5JTkqwrBUieWU-BRqVdax8YxA0iDKzZyfsMV92yVcZT6S_NQ \ No newline at end of file +eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6Inp4Yzg5MCJ9.eyJpc3MiOiJodHRwczovL3N0cy53aW5kb3dzLm5ldC9idW5jaC1vZi1yYW5kb20tY2hhcnMiLCJuYW1lIjoiSm9obiBTbWl0aCIsImF1ZCI6InRoZSBjbGllbnQgaWQiLCJub25jZSI6Im15IG5vbmNlIiwiZW1haWwiOiJqc21pdGhAY29udG9zby5jb20iLCJwcmVmZXJyZWRfdXNlcm5hbWUiOiJqc21pdGhAY29udG9zby5jb20iLCJ2ZXIiOiIyLjAifQ.I2LrcnOc4Ka3LcfJQBb4mCt4rcJF1eCIyHiQ_dMVvR5eeQTPpotxhR6gxURV-bC9gE-TZYpgal4Z28kyS_kv_Rv-mh6o6pRnRdOf9ASmruf76vaWjJkm_gmQ7tjytsWCLTsUTN0sFSk5MSwCstt6l8sEjpmvivnRi2BF85q56Vs diff --git a/spec/fixtures/id_token_bad_nonce.txt b/spec/fixtures/id_token_bad_nonce.txt index da7cc95..a90c6e2 100644 --- a/spec/fixtures/id_token_bad_nonce.txt +++ b/spec/fixtures/id_token_bad_nonce.txt @@ -1 +1 @@ -eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6ImFiYzEyMyJ9.eyJpc3MiOiJodHRwczovL3N0cy53aW5kb3dzLm5ldC9idW5jaC1vZi1yYW5kb20tY2hhcnMiLCJuYW1lIjoiSm9obiBTbWl0aCIsImF1ZCI6InRoZSBjbGllbnQgaWQiLCJub25jZSI6Im5vdCBteSBub25jZSIsImdpdmVuX25hbWUiOiJKb2huIiwiZmFtaWx5X25hbWUiOiJzbWl0aCIsImVtYWlsIjoianNtaXRoQGNvbnRvc28uY29tIn0.KBSOCiy0sUF33akept9nPNFgmMWxiPWDBVA0dZqQaF6gQkhQu82irAQzB2Ygkh9KDeIEf0MSZWLDq0X3W3Fke4wzjrbzL-2QM4l-KgPFbJixqtJYHSPOidHSCQ8vA0v8kES3H_zqU6QisygwwXLh2ozqKXMnsrBPIAtiZz_a0vPbHtrYbb-WIrtTruMemcTt5OkbfDIttzi5EarakQg93vraIb0jK0szuAqLkXFOzcIIGPgyAvVpHZveqm99GR04tbKyTRIyJP1vZwtIpu89PKFM3soWcWd_kjAWcS81ZTzEn5_P-1QL7YTInK53acNXZHh08Fba3Um4J_KlV2KRjw \ No newline at end of file +eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6ImFiYzEyMyJ9.eyJpc3MiOiJodHRwczovL3N0cy53aW5kb3dzLm5ldC9idW5jaC1vZi1yYW5kb20tY2hhcnMiLCJuYW1lIjoiSm9obiBTbWl0aCIsImF1ZCI6InRoZSBjbGllbnQgaWQiLCJub25jZSI6Im5vdCBteSBub25jZSIsImVtYWlsIjoianNtaXRoQGNvbnRvc28uY29tIiwicHJlZmVycmVkX3VzZXJuYW1lIjoianNtaXRoQGNvbnRvc28uY29tIiwidmVyIjoiMi4wIn0.YMNhaW8g-jP5dJ0_zpB0qNMi2NvysztpcB6j9C31EbrAckfhckJE4urART7MMFy_cdNEIFLTZT5yT4bj10EnBujkNmCVVmCG7KSE7hdqWkSbM_3IPNQayVOXqz6hkde_-Di2L40nZ-HvNNt6q1sRaxTj6oF6DEnZBQ2iYga3fIQ diff --git a/spec/fixtures/id_token_no_alg.txt b/spec/fixtures/id_token_no_alg.txt index a918b7a..b633d58 100644 --- a/spec/fixtures/id_token_no_alg.txt +++ b/spec/fixtures/id_token_no_alg.txt @@ -1 +1 @@ -eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6ImFiYzEyMyJ9.eyJpc3MiOiJodHRwczovL3N0cy53aW5kb3dzLm5ldC9idW5jaC1vZi1yYW5kb20tY2hhcnMiLCJuYW1lIjoiSm9obiBTbWl0aCIsImF1ZCI6InRoZSBjbGllbnQgaWQiLCJub25jZSI6Im15IG5vbmNlIiwiZ2l2ZW5fbmFtZSI6IkpvaG4iLCJmYW1pbHlfbmFtZSI6InNtaXRoIiwiZW1haWwiOiJqc21pdGhAY29udG9zby5jb20iLCJjX2hhc2giOiJWcFRRaWk1VF84cmd3eEEtV3RiMkJ3In0.Xz9SL1dm9xeJ3YtBIwSpL7SHEMr5lsL32mkJIugoAt7rNhj2ZauN77_N3skU9FIRTVb_XBFHrLo1AXion7RWoOGAMk8xnuQRlhamGoWsjttWE9oO6J6kuQPSDBvLTA88UqLoGNezDwFNfgUFQq-66m33ZWdkiNOFoFZ_In6DtAwxHZZUys-KoYD3iCbviUoBzU57aV-SBsWyComq39pDGpw03qZoa_xgRfujdVHG1DKlO5VG79kUE3ySYWJyBYVKUdAzjH1iotjpPA1svtqytn4CUldAMi4nnf7iq5SCJMb4ucu0mN6AhJH9ktY--fGY6_OidyiDe4F57ZzLw-3jOQ \ No newline at end of file +eyJ0eXAiOiJKV1QiLCJhbGciOiJSUzI1NiIsImtpZCI6ImFiYzEyMyJ9.eyJpc3MiOiJodHRwczovL3N0cy53aW5kb3dzLm5ldC9idW5jaC1vZi1yYW5kb20tY2hhcnMiLCJuYW1lIjoiSm9obiBTbWl0aCIsImF1ZCI6InRoZSBjbGllbnQgaWQiLCJub25jZSI6Im15IG5vbmNlIiwiZ2l2ZW5fbmFtZSI6IkpvaG4iLCJmYW1pbHlfbmFtZSI6InNtaXRoIiwiZW1haWwiOiJqc21pdGhAY29udG9zby5jb20iLCJjX2hhc2giOiJWcFRRaWk1VF84cmd3eEEtV3RiMkJ3In0.Xz9SL1dm9xeJ3YtBIwSpL7SHEMr5lsL32mkJIugoAt7rNhj2ZauN77_N3skU9FIRTVb_XBFHrLo1AXion7RWoOGAMk8xnuQRlhamGoWsjttWE9oO6J6kuQPSDBvLTA88UqLoGNezDwFNfgUFQq-66m33ZWdkiNOFoFZ_In6DtAwxHZZUys-KoYD3iCbviUoBzU57aV-SBsWyComq39pDGpw03qZoa_xgRfujdVHG1DKlO5VG79kUE3ySYWJyBYVKUdAzjH1iotjpPA1svtqytn4CUldAMi4nnf7iq5SCJMb4ucu0mN6AhJH9ktY--fGY6_OidyiDe4F57ZzLw-3jOQ diff --git a/spec/fixtures/id_token_with_chash.txt b/spec/fixtures/id_token_with_chash.txt new file mode 100644 index 0000000..1c2159a --- /dev/null +++ b/spec/fixtures/id_token_with_chash.txt @@ -0,0 +1 @@ +eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6ImFiYzEyMyJ9.eyJpc3MiOiJodHRwczovL3N0cy53aW5kb3dzLm5ldC9idW5jaC1vZi1yYW5kb20tY2hhcnMiLCJuYW1lIjoiSm9obiBTbWl0aCIsImF1ZCI6InRoZSBjbGllbnQgaWQiLCJub25jZSI6Im15IG5vbmNlIiwiZW1haWwiOiJqc21pdGhAY29udG9zby5jb20iLCJwcmVmZXJyZWRfdXNlcm5hbWUiOiJqc21pdGhAY29udG9zby5jb20iLCJ2ZXIiOiIyLjAiLCJjX2hhc2giOiJWcFRRaWk1VF84cmd3eEEtV3RiMkJ3In0.LHxAkKs0PENBIok-klfRA6glaJX3Q3IxdH_C9NWg52RBaKo5SABUQR6yIo1LGzfYmF1yVGvbaNgDZ-P_iVf0bhdG07KJwHBpB9-JKdZu42G1bQFfrbxWd0X8iWAGfosgjCxzK1lLzWL96oc1e0_JD3tMTxRz7oDToLhfw2Bnb8I diff --git a/spec/fixtures/x5c.txt b/spec/fixtures/x5c.txt index d1c038a..7b87a53 100644 --- a/spec/fixtures/x5c.txt +++ b/spec/fixtures/x5c.txt @@ -1 +1 @@ -MIIBwzCCAbegAwIBAgIBATADBgEAMDAxEzARBgoJkiaJk_IsZAEZFgNvcmcxGTAXBgoJkiaJk_IsZAEZFglydWJ5LWxhbmcwHhcNMTUwODA2MjMyOTE4WhcNMjUwODAzMjMyOTE4WjAwMRMwEQYKCZImiZPyLGQBGRYDb3JnMRkwFwYKCZImiZPyLGQBGRYJcnVieS1sYW5nMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAxyunSConbK4z1T47vukPa0OaNcY_R6l8Z0TILR1w5O_YkMbJGRXpZORFPVs83xLoxs7RcPWnibQhQ4G6m6fwEA4rutvOm-3xWyey-OOZdVqpNmxqd2VcCCI6AoUtl8m4w0UFuu-oiELj7BF8cGS5wvHYATEBEY72n_84uu-LF423Ffe8HeMEY00nO3ZVcV9MjFBVps8RAwL62ooXPZyly6fQt4728wlZPs3EMijS-Bj4auokx6ssBooaF9XiZJKKCAe16epbBB9S4XVlNXo6EhZ-PBpMFRJknDwWFFYPVOX3NlBp8chi8VaXmDXqsFJiwkkeIqg4I6WFAM4BsnDInwIDAQABMAMGAQADAQA \ No newline at end of file +LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUJjekNCM2FBREFnRUNBZ0VBTUEwR0NTcUdTSWIzRFFFQkJRVUFNQUF3SGhjTk1UZ3hNREUxTWpJeE1qTXkKV2hjTk1UZ3hNREUxTWpNeE1qUTFXakFBTUlHZk1BMEdDU3FHU0liM0RRRUJBUVVBQTRHTkFEQ0JpUUtCZ1FEZApsYXRSalJqb2dvM1dvamdHSEZIWUx1Z2RVV0FZOWlSM2Z5NGFyV05BMUtvUzhrVnczM2NKaWJYcjhidndVQVVwCmFyQ3dsdmRiSDZkdkVPZm91MC9nQ0ZRc0hVZlFyU0R2K011U1VNQWU4anpLRTRxVytqSyt4UVU5YTAzR1VuS0gKa2tsZStRMHBYL2c2alhaN3IxL3hBSzVEbzJrUStYNXhLOWNpcFJnRUt3SURBUUFCTUEwR0NTcUdTSWIzRFFFQgpCUVVBQTRHQkFLVFMveFRJdUhwUHJMNUQ5YkNJMXZvRlRld2hsMmtuNzU1bVgwMTAwNnNFbFdMR2RvSlNUeWgrCjdYNWtsUEVpeW9yRkpLeFc0TzFZRDNxUW5sTUhrRTBSZlNDYURQaWlhYUF4UnhnWklkTFVFblFmSS82L3FQV0IKcjR2UlY3aTV3ZTNoMXNSSFhsbHUwT05DbmR0eTlvbks2UnVGTmFHdGprLzkwRVphSm1BegotLS0tLUVORCBDRVJUSUZJQ0FURS0tLS0tCg diff --git a/spec/omniauth/strategies/azure_activedirectory_spec.rb b/spec/omniauth/strategies/azure_activedirectory_spec.rb index dbdc791..e2d495c 100644 --- a/spec/omniauth/strategies/azure_activedirectory_spec.rb +++ b/spec/omniauth/strategies/azure_activedirectory_spec.rb @@ -26,86 +26,101 @@ # This was fairly awkward to test. I've stubbed every endpoint and am simulating # the state of the request. Especially large strings are stored in fixtures. describe OmniAuth::Strategies::AzureActiveDirectory do - let(:app) { -> _ { [200, {}, ['Hello world.']] } } - let(:x5c) { File.read(File.expand_path('../../../fixtures/x5c.txt', __FILE__)) } + let(:app) { ->(_) { [200, {}, ['Hello world.']] } } + let(:x5c) { File.read(File.expand_path('../../fixtures/x5c.txt', __dir__)) } # These values were used to create the "successful" id_token JWT. let(:client_id) { 'the client id' } let(:code) { 'code' } let(:email) { 'jsmith@contoso.com' } - let(:family_name) { 'smith' } - let(:given_name) { 'John' } let(:issuer) { 'https://sts.windows.net/bunch-of-random-chars' } let(:kid) { 'abc123' } let(:name) { 'John Smith' } + let(:preferred_username) { 'jsmith@contoso.com' } let(:nonce) { 'my nonce' } let(:session_state) { 'session state' } let(:auth_endpoint_host) { 'authorize.com' } + let(:options) { { verify_iss: true } } + let(:base_config) { { client_id: client_id, client_secret: 'secret', tenant: tenant }.merge(options) } + let(:tenant) { 'tenant' } + let(:openid_config_response) do + { + issuer: issuer, + authorization_endpoint: "http://#{auth_endpoint_host}", + jwks_uri: "https://login.microsoftonline.com/#{tenant}/discovery/v2.0/keys", + token_endpoint: "https://login.microsoftonline.com/#{tenant}/oauth2/v2.0/token" + }.to_json + end + let(:keys_response) do + { + keys: [ + { + kid: kid, + x5c: [x5c] + } + ] + }.to_json + end + let(:env) { { 'rack.session' => { 'omniauth-azure-activedirectory.nonce' => nonce } } } + let(:client) { OAuth2::Client.new('abc', 'def') } - let(:hybrid_flow_params) do - { 'id_token' => id_token, + let(:id_token) { File.read(File.expand_path('../../fixtures/id_token.txt', __dir__)) } + let(:params) do + { + 'id_token' => id_token, 'session_state' => session_state, - 'code' => code } + 'code' => code + } + end + let(:request) { double('Request', params: params, path_info: 'path') } + let(:access_token) do + OAuth2::AccessToken.from_hash( + client, + 'id_token' => id_token + ) + end + let(:strategy) do + described_class.new(app, base_config).tap do |s| + allow(s).to receive(:request) { request } + allow(s).to receive(:access_token).and_return(access_token) + end end - let(:tenant) { 'tenant' } - let(:openid_config_response) { "{\"issuer\":\"#{issuer}\",\"authorization_endpoint\":\"http://#{auth_endpoint_host}\",\"jwks_uri\":\"https://login.windows.net/common/discovery/keys\"}" } - let(:keys_response) { "{\"keys\":[{\"kid\":\"#{kid}\",\"x5c\":[\"#{x5c}\"]}]}" } - - let(:env) { { 'rack.session' => { 'omniauth-azure-activedirectory.nonce' => nonce } } } - + subject { -> { strategy.auth_hash } } before(:each) do - stub_request(:get, "https://login.windows.net/#{tenant}/.well-known/openid-configuration") + strategy.call!(env) + stub_request(:get, "https://login.microsoftonline.com/#{tenant}/v2.0/.well-known/openid-configuration/") .to_return(status: 200, body: openid_config_response) - stub_request(:get, 'https://login.windows.net/common/discovery/keys') + stub_request(:get, "https://login.microsoftonline.com/#{tenant}/discovery/v2.0/keys") .to_return(status: 200, body: keys_response) end - describe '#callback_phase' do - let(:request) { double('Request', params: hybrid_flow_params, path_info: 'path') } - let(:strategy) do - described_class.new(app, client_id, tenant).tap do |s| - allow(s).to receive(:request) { request } - end - end - - subject { -> { strategy.callback_phase } } - before(:each) { strategy.call!(env) } - + describe '#auth_hash' do context 'with a successful response' do # payload: # { 'iss' => 'https://sts.windows.net/bunch-of-random-chars', # 'name' => 'John Smith', # 'aud' => 'the client id', # 'nonce' => 'my nonce', - # 'email' => 'jsmith@contoso.com', - # 'given_name' => 'John', - # 'family_name' => 'Smith' } + # 'email' => 'jsmith@contoso.com' } # headers: # { 'typ' => 'JWT', # 'alg' => 'RS256', # 'kid' => 'abc123' } # - let(:id_token) { File.read(File.expand_path('../../../fixtures/id_token.txt', __FILE__)) } # If it passes this test, then the id was successfully validated. it { is_expected.to_not raise_error } describe 'the auth hash' do - before(:each) { strategy.callback_phase } - - subject { env['omniauth.auth'] } + subject { strategy.auth_hash } it 'should contain the name' do expect(subject.info['name']).to eq name end - it 'should contain the first name' do - expect(subject.info['first_name']).to eq given_name - end - - it 'should contain the last name' do - expect(subject.info['last_name']).to eq family_name + it 'should contain the preferred_username' do + expect(subject.info['preferred_username']).to eq preferred_username end it 'should contain the email' do @@ -123,10 +138,11 @@ end context 'with an invalid issuer' do + subject { -> { strategy.auth_hash } } # payload: # { 'iss' => 'https://sts.imposter.net/bunch-of-random-chars', ... } # - let(:id_token) { File.read(File.expand_path('../../../fixtures/id_token_bad_issuer.txt', __FILE__)) } + let(:id_token) { File.read(File.expand_path('../../fixtures/id_token_bad_issuer.txt', __dir__)) } it { is_expected.to raise_error JWT::InvalidIssuerError } end @@ -134,7 +150,7 @@ # payload: # { 'aud' => 'not the client id', ... } # - let(:id_token) { File.read(File.expand_path('../../../fixtures/id_token_bad_audience.txt', __FILE__)) } + let(:id_token) { File.read(File.expand_path('../../fixtures/id_token_bad_audience.txt', __dir__)) } it { is_expected.to raise_error JWT::InvalidAudError } end @@ -142,46 +158,44 @@ # payload: # { 'nonce' => 'not my nonce', ... } # - let(:id_token) { File.read(File.expand_path('../../../fixtures/id_token_bad_nonce.txt', __FILE__)) } + let(:id_token) { File.read(File.expand_path('../../fixtures/id_token_bad_nonce.txt', __dir__)) } it { is_expected.to raise_error JWT::DecodeError } end context 'with the wrong x5c' do - let(:x5c) { File.read(File.expand_path('../../../fixtures/x5c_different.txt', __FILE__)) } - let(:id_token) { File.read(File.expand_path('../../../fixtures/id_token.txt', __FILE__)) } + let(:x5c) { File.read(File.expand_path('../../fixtures/x5c_different.txt', __dir__)) } + let(:id_token) { File.read(File.expand_path('../../fixtures/id_token.txt', __dir__)) } it { is_expected.to raise_error JWT::VerificationError } end - context 'with a non-matching c_hash' do - let(:id_token) { File.read(File.expand_path('../../../fixtures/id_token_bad_chash.txt', __FILE__)) } - it { is_expected.to raise_error JWT::VerificationError } + # From Microsoft docs: + # > The code hash is included in ID tokens only when the ID token is issued + # > with an OAuth 2.0 authorization code. + # + # Since the c_hash isn't always included, we will only validate if it's + # present in the id_token. + context 'when c_hash is included in id_token' do + context 'with a matching c_hash' do + let(:id_token) { File.read(File.expand_path('../../fixtures/id_token_with_chash.txt', __dir__)) } + it { is_expected.to_not raise_error } + end + + context 'with a non-matching c_hash' do + let(:id_token) { File.read(File.expand_path('../../fixtures/id_token_bad_chash.txt', __dir__)) } + it { is_expected.to raise_error JWT::VerificationError } + end end context 'with a non-matching kid' do - let(:id_token) { File.read(File.expand_path('../../../fixtures/id_token_bad_kid.txt', __FILE__)) } + let(:id_token) { File.read(File.expand_path('../../fixtures/id_token_bad_kid.txt', __dir__)) } it { is_expected.to raise_error JWT::VerificationError } end - - context 'with no alg header' do - let(:id_token) { File.read(File.expand_path('../../../fixtures/id_token_no_alg.txt', __FILE__)) } - - it 'should correctly parse using default RS256' do - expect(subject).to_not raise_error - end - - describe 'the auth hash' do - subject { env['omniauth.auth'] } - before(:each) { strategy.callback_phase } - - it 'should default to RS256' do - expect(subject.info['name']).to eq name - end - end - end end describe '#request_phase' do - let(:strategy) { described_class.new(app, client_id, tenant) } + let(:strategy) do + described_class.new(app, base_config) + end subject { strategy.request_phase } before(:each) { strategy.call!(env) } @@ -195,7 +209,7 @@ end describe '#read_nonce' do - let(:strategy) { described_class.new(app, client_id, tenant) } + let(:strategy) { described_class.new(app, base_config) } let(:env) { { 'rack.session' => {} } } before(:each) { strategy.call!(env) } subject { strategy.send(:read_nonce) }