Merge pull request #735 from johnnyshields/v2.x-rework-uuid

pitbulk · web-flow · commit 9ab4d3f9d4c5 · 2025-01-15T12:34:27.000+01:00
v2.x - Rework SP request UUID generation to remove mutable constant
diff --git a/.rubocop.yml b/.rubocop.yml
@@ -16,6 +16,9 @@ AllCops:
     - 'tmp/**/*'
     - 'vendor/**/*'
 
+Style/Alias:
+  EnforcedStyle: prefer_alias_method
+
 Style/ModuleFunction:
   EnforcedStyle: extend_self
 
diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
@@ -181,7 +181,7 @@ Lint/UselessAssignment:
 # Offense count: 42
 # Configuration parameters: AllowedMethods, AllowedPatterns, CountRepeatedAttributes.
 Metrics/AbcSize:
-  Max: 100
+  Max: 200
 
 # Offense count: 1
 # Configuration parameters: CountComments, CountAsOne, AllowedMethods, AllowedPatterns.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,11 +13,12 @@
 * [#709](https://github.com/SAML-Toolkits/ruby-saml/pull/709) Allow passing in `Net::HTTP` `:open_timeout`, `:read_timeout`, and `:max_retries` settings to `IdpMetadataParser#parse_remote`.
 * [#715](https://github.com/SAML-Toolkits/ruby-saml/pull/715) Fix typo in error when SPNameQualifier value does not match the SP entityID.
 * [#718](https://github.com/SAML-Toolkits/ruby-saml/pull/718) Add support to retrieve from SAMLResponse the AuthnInstant and AuthnContextClassRef values
-* [#711](https://github.com/SAML-Toolkits/ruby-saml/pull/711) Standardize how RubySaml reads and formats certificate and private_key PEM values, including the `RubySaml::Util#format_cert` and `#format_private_key` methods.
+* [#711](https://github.com/SAML-Toolkits/ruby-saml/pull/711) Standardize how RubySaml reads and formats certificate and private_key PEM values, including the `RubySaml::Utils#format_cert` and `#format_private_key` methods.
 * [#732](https://github.com/SAML-Toolkits/ruby-saml/pull/732) Return original certificate and private key objects from `RubySaml::Utils` build functions.
 * [#732](https://github.com/SAML-Toolkits/ruby-saml/pull/732) Allow SP `certificate`, `certificate_new`, and `private_key` settings to be `OpenSSL::X509::Certificate` and `OpenSSL::PKey::PKey` objects respectively.
 * [#733](https://github.com/SAML-Toolkits/ruby-saml/pull/733) Allow `RubySaml::Utils.is_cert_expired` and `is_cert_active` to accept an optional time argument.
 * [#731](https://github.com/SAML-Toolkits/ruby-saml/pull/731) Add CI coverage for Ruby 3.4. Remove CI coverage for Ruby 1.x and 2.x.
+* [#735](https://github.com/SAML-Toolkits/ruby-saml/pull/735) Add `Settings#sp_uuid_prefix` and deprecate `Utils#set_prefix`.
 
 ### 1.18.0  (???)
 * [#718](https://github.com/SAML-Toolkits/ruby-saml/pull/718) Add support to retrieve from SAMLResponse the AuthnInstant and AuthnContextClassRef values
diff --git a/README.md b/README.md
@@ -963,6 +963,15 @@ end
 
 The `attribute_value` option additionally accepts an array of possible values.
 
+## SP-Originated Message IDs
+
+Ruby SAML automatically generates message IDs for SP-originated messages (AuthNRequest, etc.)
+By default, this is a UUID prefixed by the `_` character, for example `"_ea8b5fdf-0a71-4bef-9f87-5406ee746f5b"`. 
+To override this behavior, you may set `settings.sp_uuid_prefix` to a string of your choice.
+Note that the SAML specification requires that this type (`xsd:ID`) be an
+[NCName](https://www.w3.org/TR/xmlschema-2/#NCName), meaning that it must start with a letter
+or underscore, and can only contain letters, digits, underscores, hyphens, and periods.
+
 ## Custom Metadata Fields
 
 Some IdPs may require to add SPs to add additional fields (Organization, ContactPerson, etc.)
diff --git a/UPGRADING.md b/UPGRADING.md
@@ -77,6 +77,30 @@ settings.idp_slo_service_binding = :redirect
 
 For clarity, the default value of both parameters is `:redirect` if they are not set.
 
+### Change to message UUID prefix customization
+
+On SP-originated messages (`Authrequest`, `Logoutrequest`, `Logoutresponse`), RubySaml generates the
+`uuid` (aliased to `request_id` / `response_id`) using the `_` character as a default prefix,
+for example `_a1b3c5d7-9f1e-3d5c-7b1a-9f1e3d5c7b1a`. In RubySaml versions prior to `2.0.0`, it was
+possible to change this default prefix by either `RubySaml::Utils.set_prefix` or by mutating
+the `RubySaml::Utils::UUID_PREFIX` constant (which was what `.set_prefix` did.) In RubySaml `2.0.0`,
+this prefix is now set using `settings.sp_uuid_prefix`:
+
+```ruby
+# Change the default prefix from `_` to `my_id_`
+settings.sp_uuid_prefix = 'my_id_'
+
+# Create the AuthNRequest message
+request = RubySaml::Authrequest.new
+request.create(settings)
+request.uuid #=> "my_id_a1b3c5d7-9f1e-3d5c-7b1a-9f1e3d5c7b1a"
+```
+
+A side-effect of this change is that the `uuid` of the `Authrequest`, `Logoutrequest`, and `Logoutresponse`
+classes now is `nil` until the `#create` method is called (previously, it was set in the constructor.)
+After calling `#create` for the first time the `uuid` will not change, even if a `Settings` object with
+a different `sp_uuid_prefix` is passed-in on subsequent calls.
+
 ### Deprecation of compression settings
 
 The `settings.compress_request` and `settings.compress_response` parameters have been deprecated
@@ -103,11 +127,10 @@ The following parameters in `RubySaml::Settings` are deprecated and will be remo
 
 ### Minor changes to Util#format_cert and #format_private_key
 
-
 Version `2.0.0` standardizes how RubySaml reads and formats certificate and private key
 PEM strings. In general, version `2.0.0` is more permissive than `1.x`, and the changes
 are not anticipated to affect most users. Please note the change affects parameters
-such `#idp_cert` and `#certificate`, as well as the `RubySaml::Util#format_cert`
+such `#idp_cert` and `#certificate`, as well as the `RubySaml::Utils#format_cert`
 and `#format_private_key` methods. Specifically:
 
 
diff --git a/lib/ruby_saml/authrequest.rb b/lib/ruby_saml/authrequest.rb
@@ -16,25 +16,15 @@ class Authrequest < SamlMessage
 
     # AuthNRequest ID
     attr_accessor :uuid
-
-    # Initializes the AuthNRequest. An Authrequest Object that is an extension of the SamlMessage class.
-    # Asigns an ID, a random uuid.
-    #
-    def initialize
-      @uuid = RubySaml::Utils.uuid
-      super()
-    end
-
-    def request_id
-      @uuid
-    end
+    alias_method :request_id, :uuid
 
     # Creates the AuthNRequest string.
     # @param settings [RubySaml::Settings|nil] Toolkit settings
     # @param params [Hash] Some extra parameters to be added in the GET for example the RelayState
     # @return [String] AuthNRequest string that includes the SAMLRequest
     #
     def create(settings, params = {})
+      assign_uuid(settings)
       params = create_params(settings, params)
       params_prefix = /\?/.match?(settings.idp_sso_service_url) ? '&' : '?'
       saml_request = CGI.escape(params.delete("SAMLRequest"))
@@ -107,6 +97,7 @@ def create_authentication_xml_doc(settings)
 
     def create_xml_document(settings)
       time = Time.now.utc.strftime("%Y-%m-%dT%H:%M:%SZ")
+      assign_uuid(settings)
 
       request_doc = RubySaml::XML::Document.new
       request_doc.uuid = uuid
@@ -190,5 +181,9 @@ def sign_document(document, settings)
 
       document
     end
+
+    def assign_uuid(settings)
+      @uuid ||= RubySaml::Utils.generate_uuid(settings.sp_uuid_prefix) # rubocop:disable Naming/MemoizedInstanceVariableName
+    end
   end
 end
diff --git a/lib/ruby_saml/logoutrequest.rb b/lib/ruby_saml/logoutrequest.rb
@@ -14,25 +14,15 @@ class Logoutrequest < SamlMessage
 
     # Logout Request ID
     attr_accessor :uuid
-
-    # Initializes the Logout Request. A Logoutrequest Object that is an extension of the SamlMessage class.
-    # Asigns an ID, a random uuid.
-    #
-    def initialize
-      @uuid = RubySaml::Utils.uuid
-      super()
-    end
-
-    def request_id
-      @uuid
-    end
+    alias_method :request_id, :uuid
 
     # Creates the Logout Request string.
     # @param settings [RubySaml::Settings|nil] Toolkit settings
     # @param params [Hash] Some extra parameters to be added in the GET for example the RelayState
     # @return [String] Logout Request string that includes the SAMLRequest
     #
     def create(settings, params={})
+      assign_uuid(settings)
       params = create_params(settings, params)
       params_prefix = /\?/.match?(settings.idp_slo_service_url) ? '&' : '?'
       saml_request = CGI.escape(params.delete("SAMLRequest"))
@@ -105,6 +95,7 @@ def create_logout_request_xml_doc(settings)
 
     def create_xml_document(settings)
       time = Time.now.utc.strftime("%Y-%m-%dT%H:%M:%SZ")
+      assign_uuid(settings)
 
       request_doc = RubySaml::XML::Document.new
       request_doc.uuid = uuid
@@ -149,5 +140,9 @@ def sign_document(document, settings)
 
       document
     end
+
+    def assign_uuid(settings)
+      @uuid ||= RubySaml::Utils.generate_uuid(settings.sp_uuid_prefix) # rubocop:disable Naming/MemoizedInstanceVariableName
+    end
   end
 end
diff --git a/lib/ruby_saml/settings.rb b/lib/ruby_saml/settings.rb
@@ -48,6 +48,7 @@ def initialize(overrides = {}, keep_security_attributes = false)
     attr_reader   :assertion_consumer_service_binding
     attr_accessor :single_logout_service_url
     attr_reader   :single_logout_service_binding
+    attr_accessor :sp_uuid_prefix
     attr_accessor :sp_name_qualifier
     attr_accessor :name_identifier_format
     attr_accessor :name_identifier_value
diff --git a/lib/ruby_saml/slo_logoutresponse.rb b/lib/ruby_saml/slo_logoutresponse.rb
@@ -1,7 +1,6 @@
 # frozen_string_literal: true
 
 require "ruby_saml/logging"
-
 require "ruby_saml/saml_message"
 require "ruby_saml/utils"
 require "ruby_saml/setting_error"
@@ -15,18 +14,7 @@ class SloLogoutresponse < SamlMessage
 
     # Logout Response ID
     attr_accessor :uuid
-
-    # Initializes the Logout Response. A SloLogoutresponse Object that is an extension of the SamlMessage class.
-    # Asigns an ID, a random uuid.
-    #
-    def initialize
-      @uuid = RubySaml::Utils.uuid
-      super()
-    end
-
-    def response_id
-      @uuid
-    end
+    alias_method :response_id, :uuid
 
     # Creates the Logout Response string.
     # @param settings [RubySaml::Settings|nil] Toolkit settings
@@ -37,6 +25,7 @@ def response_id
     # @return [String] Logout Request string that includes the SAMLRequest
     #
     def create(settings, request_id = nil, logout_message = nil, params = {}, logout_status_code = nil)
+      assign_uuid(settings)
       params = create_params(settings, request_id, logout_message, params, logout_status_code)
       params_prefix = /\?/.match?(settings.idp_slo_service_url) ? '&' : '?'
       url = settings.idp_slo_response_service_url || settings.idp_slo_service_url
@@ -117,6 +106,7 @@ def create_logout_response_xml_doc(settings, request_id = nil, logout_message =
 
     def create_xml_document(settings, request_id = nil, logout_message = nil, status_code = nil)
       time = Time.now.utc.strftime('%Y-%m-%dT%H:%M:%SZ')
+      assign_uuid(settings)
 
       response_doc = RubySaml::XML::Document.new
       response_doc.uuid = uuid
@@ -160,5 +150,9 @@ def sign_document(document, settings)
 
       document
     end
+
+    def assign_uuid(settings)
+      @uuid ||= RubySaml::Utils.generate_uuid(settings.sp_uuid_prefix) # rubocop:disable Naming/MemoizedInstanceVariableName
+    end
   end
 end
diff --git a/lib/ruby_saml/utils.rb b/lib/ruby_saml/utils.rb
@@ -30,7 +30,11 @@ module Utils # rubocop:disable Metrics/ModuleLength
         (\d+)W                    # 8: Weeks
       )
     $/x
-    UUID_PREFIX = +'_'
+    UUID_DEFAULT_PREFIX = '_'
+
+    # @deprecated Use UUID_DEFAULT_PREFIX instead.
+    # This constant is intentionally unfrozen for backwards compatibility.
+    UUID_PREFIX = UUID_DEFAULT_PREFIX.dup
 
     # Checks if the x509 cert provided is expired.
     #
@@ -382,13 +386,22 @@ def retrieve_plaintext(cipher_text, symmetric_key, algorithm)
       end
     end
 
-    def set_prefix(value)
-      UUID_PREFIX.replace value
+    # @deprecated Use RubySaml::Settings#sp_uuid_prefix instead.
+    def set_prefix(_value)
+      raise NoMethodError.new('RubySaml::Util.set_prefix has been removed. Please use RubySaml::Settings#sp_uuid_prefix instead.')
     end
 
-    def uuid
-      "#{UUID_PREFIX}#{SecureRandom.uuid}"
+    # Generates a UUID with a prefix.
+    #
+    # @param prefix [String|false|nil] An explicit prefix override.
+    #   Using nil will use the default prefix, and false will use no prefix.
+    # @return [String] The generated UUID.
+    def generate_uuid(prefix = nil)
+      prefix = prefix.is_a?(FalseClass) ? nil : prefix || UUID_DEFAULT_PREFIX
+      "#{prefix}#{SecureRandom.uuid}"
     end
+    # @deprecated Use #generate_uuid
+    alias_method :uuid, :generate_uuid
 
     # Given two strings, attempt to match them as URIs using Rails' parse method.  If they can be parsed,
     # then the fully-qualified domain name and the host should performa a case-insensitive match, per the
diff --git a/test/authrequest_test.rb b/test/authrequest_test.rb
@@ -161,17 +161,50 @@ class AuthrequestTest < Minitest::Test
       assert auth_url.include?('&RelayState=http%3A%2F%2Fexample.com')
     end
 
-    it "creates request with ID prefixed with default '_'" do
-      request = RubySaml::Authrequest.new
+    describe "uuid" do
+      it "uuid is initialized to nil" do
+        request = RubySaml::Authrequest.new
 
-      assert_match(/^_/, request.uuid)
-    end
+        assert_nil(request.uuid)
+        assert_equal request.request_id, request.uuid
+      end
+
+      it "creates request with ID prefixed with default '_'" do
+        request = RubySaml::Authrequest.new
+        request.create(settings)
+
+        assert_match(/^_/, request.uuid)
+        assert_equal request.uuid, request.request_id
+      end
+
+      it "does not change even after repeated #create calls" do
+        request = RubySaml::Authrequest.new
+        request.create(settings)
+
+        uuid = request.uuid
+        request.create(settings)
+
+        assert_equal uuid, request.uuid
+        assert_equal request.uuid, request.request_id
+      end
+
+      it "creates request with ID prefixed by Settings#sp_uuid_prefix" do
+        settings.sp_uuid_prefix = 'test'
+        request = RubySaml::Authrequest.new
+        request.create(settings)
+
+        assert_match(/^test/, request.uuid)
+        assert_equal request.uuid, request.request_id
+      end
 
-    it "creates request with ID is prefixed, when :id_prefix is passed" do
-      RubySaml::Utils::set_prefix("test")
-      request = RubySaml::Authrequest.new
-      assert_match(/^test/, request.uuid)
-      RubySaml::Utils::set_prefix("_")
+      it "can mutate the uuid" do
+        request = RubySaml::Authrequest.new
+        request_id = request.request_id
+        assert_equal request_id, request.uuid
+        request.uuid = "new_uuid"
+        assert_equal "new_uuid", request.uuid
+        assert_equal request.uuid, request.request_id
+      end
     end
 
     describe "when the target url is not set" do
@@ -272,17 +305,6 @@ class AuthrequestTest < Minitest::Test
       assert auth_doc.to_s =~ /<saml:AuthnContextDeclRef>example\/decl\/ref<\/saml:AuthnContextDeclRef>/
     end
 
-    describe "#manipulate request_id" do
-      it "be able to modify the request id" do
-        authnrequest = RubySaml::Authrequest.new
-        request_id = authnrequest.request_id
-        assert_equal request_id, authnrequest.uuid
-        authnrequest.uuid = "new_uuid"
-        assert_equal authnrequest.request_id, authnrequest.uuid
-        assert_equal "new_uuid", authnrequest.request_id
-      end
-    end
-
     each_signature_algorithm do |sp_key_algo, sp_hash_algo|
       describe "#create_params signing with HTTP-POST binding" do
         before do
diff --git a/test/logoutrequest_test.rb b/test/logoutrequest_test.rb
diff --git a/test/slo_logoutresponse_test.rb b/test/slo_logoutresponse_test.rb
diff --git a/test/test_helper.rb b/test/test_helper.rb
