diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a92838..0c01c39 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Change Log +## v3.0.1 (2024-11-21) + +* Fixes a minor error in [`UPGRADING.md`](UPGRADING.md) reported in #38, via #40 (thanks to @kennethgeerts) +* Fixes incorrect attempt to verify JWT token issuer when the AD FS tenant `adfs` is specified, via #39 (thanks to @washu) + ## v3.0.0 (2024-10-22) * To upgrade from the Azure ActiveDirectory V2 gem, please see [`UPGRADING.md`](UPGRADING.md) diff --git a/README.md b/README.md index fd0301d..33a3152 100644 --- a/README.md +++ b/README.md @@ -116,7 +116,7 @@ If you're using the client assertion flow, you need to register your certificate | `authorize_params` | Additional parameters passed as URL query data in the initial OAuth redirection to Microsoft. See below for more. Empty Hash default. | | `domain_hint` | If defined, sets (overwriting, if already present) `domain_hint` inside `authorize_params`. Default `nil` / none. | | `scope` | If defined, sets (overwriting, if already present) `scope` inside `authorize_params`. Default is `OmniAuth::Strategies::EntraId::DEFAULT_SCOPE` (at the time of writing, this is `'openid profile email'`). | -| `adfs` | If defined, modifies the URLs so they work with an on premise ADFS server. In order to use this you also need to set the `base_url` correctly and fill the `tenant_id` with `'adfs'`. | +| `adfs?` or `adfs` | If set to `true`, modifies the URLs so they work with an on-premise AD FS server (Active Directory Federation Services). In order to use this you also need to set the `base_url` correctly and fill the `tenant_id` with `'adfs'`. Note that the option name variation without the question mark only works for directly-specified options; provider classes must always define an override method called `adfs?`. | In addition, as a special case, if the request URL contains a query parameter `prompt`, then this will be written into `authorize_params` under that key, overwriting if present any other value there. Note that this comes from the current request URL at the time OAuth flow is commencing, _not_ via static options Hash data or via a custom provider class - but you _could_ just as easily set `scope` inside a custom `authorize_params` returned from a provider class, as shown in an example later; the request URL query mechanism is just another way of doing the same thing. diff --git a/lib/omniauth/entra_id/version.rb b/lib/omniauth/entra_id/version.rb index 4db8f55..c71a6f8 100644 --- a/lib/omniauth/entra_id/version.rb +++ b/lib/omniauth/entra_id/version.rb @@ -1,8 +1,8 @@ module OmniAuth module Entra module Id - VERSION = "3.0.0" - DATE = "2024-10-22" + VERSION = "3.0.1" + DATE = "2024-11-21" end end end diff --git a/lib/omniauth/strategies/entra_id.rb b/lib/omniauth/strategies/entra_id.rb index 92d0634..29e26c4 100644 --- a/lib/omniauth/strategies/entra_id.rb +++ b/lib/omniauth/strategies/entra_id.rb @@ -13,7 +13,8 @@ class EntraId < OmniAuth::Strategies::OAuth2 DEFAULT_SCOPE = 'openid profile email' COMMON_TENANT_ID = 'common' - ADFS_TENANT_ID = 'adfs' + AD_FS_TENANT_ID = 'adfs' + # The tenant_provider must return client_id, client_secret and, # optionally, tenant_id and base_url. # @@ -135,9 +136,17 @@ def raw_info # For multi-tenant apps (the 'common' tenant_id) it doesn't make any # sense to verify the token issuer, because the value of 'iss' in the - # token depends on the 'tid' in the token itself. We should also skip for ADFS local instance, as we dont put a valid tenant id in its place, but adfs instead + # token depends on the 'tid' in the token itself. We should also skip + # for AD FS local instances, as we don't put a valid tenant ID in its + # place, but "adfs" (see AD_FS_TENANT_ID) instead. # - issuer = if options.tenant_id.nil? || options.tenant_id == COMMON_TENANT_ID || options.tenant_id == ADFS_TENANT_ID + do_not_verify = ( + options.tenant_id.nil? || + options.tenant_id == COMMON_TENANT_ID || + options.tenant_id == AD_FS_TENANT_ID + ) + + issuer = if do_not_verify nil else "#{options.base_url || BASE_URL}/#{options.tenant_id}/v2.0" diff --git a/spec/omniauth/strategies/entra_id_spec.rb b/spec/omniauth/strategies/entra_id_spec.rb index b027c43..f1e7269 100644 --- a/spec/omniauth/strategies/entra_id_spec.rb +++ b/spec/omniauth/strategies/entra_id_spec.rb @@ -231,24 +231,51 @@ end # "describe '#client' do" end # "describe 'static common configuration' do" - describe 'static configuration with on premise ADFS' do + describe 'static configuration with on premise AD FS' do let(:options) { @options || {} } + subject do - OmniAuth::Strategies::EntraId.new(app, {client_id: 'id', client_secret: 'secret', tenant_id: 'adfs', base_url: 'https://login.contoso.com', adfs: true}.merge(options)) + OmniAuth::Strategies::EntraId.new( + app, + { + client_id: 'id', + client_secret: 'secret', + tenant_id: 'adfs', + base_url: 'https://login.contoso.com' + }.merge(options) # 'adfs' or 'adfs?' is set here, by defining @options + ) end - describe '#client' do - it 'has correct authorize url' do - allow(subject).to receive(:request) { request } - expect(subject.client.options[:authorize_url]).to eql('https://login.contoso.com/adfs/oauth2/authorize') + shared_examples 'an integration aware of AD FS' do + describe 'wherein #client' do + it 'has correct authorize url' do + allow(subject).to receive(:request) { request } + expect(subject.client.options[:authorize_url]).to eql('https://login.contoso.com/adfs/oauth2/authorize') + end + + it 'has correct token url' do + allow(subject).to receive(:request) { request } + expect(subject.client.options[:token_url]).to eql('https://login.contoso.com/adfs/oauth2/token') + end + end # "describe 'wherein #client' do" + end # "shared_examples 'an integration aware of AD FS wherein' do" + + context ':adfs option variant' do + before :each do + @options = { adfs: true } end - it 'has correct token url' do - allow(subject).to receive(:request) { request } - expect(subject.client.options[:token_url]).to eql('https://login.contoso.com/adfs/oauth2/token') + it_behaves_like 'an integration aware of AD FS' + end # "context ':adfs option variant' do" + + context ':adfs? option variant' do + before :each do + @options = { adfs?: true } end - end # "describe '#client' do" - end # "describe 'static configuration with on premise ADFS' do" + + it_behaves_like 'an integration aware of AD FS' + end # "context ':adfs? option variant' do" + end # "describe 'static configuration with on premise AD FS' do" describe 'dynamic configuration' do let(:provider_klass) { @@ -420,7 +447,7 @@ def client_secret end # "describe '#client' do" end # "describe 'dynamic common configuration' do" - describe 'dynamic configuration with on premise ADFS' do + describe 'dynamic configuration with on premise AD FS' do let(:provider_klass) { Class.new { def initialize(strategy) @@ -465,7 +492,7 @@ def adfs? expect(subject.client.options[:token_url]).to eql('https://login.contoso.com/adfs/oauth2/token') end end # "describe '#client' do" - end # "describe 'dynamic configuration with on premise ADFS' do" + end # "describe 'dynamic configuration with on premise AD FS' do" describe 'raw_info and validation' do let(:issued_at ) { Time.now.utc.to_i } @@ -623,7 +650,7 @@ def adfs? end end # "context 'when invalid' do" - context 'multi-tenant' do + context 'multi-tenant, AD FS' do let(:id_token_info) do hash = super() hash['iss'] = 'invalid issuer that should be ignored' @@ -649,7 +676,17 @@ def adfs? expect { subject.info }.to_not raise_error() end end # "context '"common" tenant specified' do" - end # "context 'multi-tenant' do" + + context '"adfs" tenant specified' do + subject do + OmniAuth::Strategies::EntraId.new(app, {client_id: 'id', client_secret: 'secret', tenant_id: OmniAuth::Strategies::EntraId::AD_FS_TENANT_ID}) + end + + it 'skips issuer validation since tenant ID is unknown' do + expect { subject.info }.to_not raise_error() + end + end # "context '"common" tenant specified' do" + end # "context 'multi-tenant, AD FS' do" end # "context 'issuers' do" context 'with an invalid not_before' do