Merge pull request #41 from RIPAGlobal/feature/v3.0.1

Version 3.0.1 with a bit of tidying, extra docs and another test

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)

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.
 

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

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"

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