Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[READY] v2.x - Rework SP request UUID generation to remove mutable constant #735

Merged
merged 17 commits into from
Jan 15, 2025

Conversation

johnnyshields
Copy link
Collaborator

@johnnyshields johnnyshields commented Jan 11, 2025

This PR reworks the way SP request UUID generation is handled.

  • Replace the mutable RubySaml::Utils::UUID_PREFIX constant and set_prefix method with Settings.sp_uuid_prefix
    • (Mutable constants are not thread-safe, violate principle of least surprise, etc.)
  • Make Authnrequest, etc. uuid attribute to be immutable.
  • Initialize Authnrequest, etc. uuid attribute when create is called, based on Settings.sp_uuid_prefix, not when instantiating the Ruby object.
image

- Replace the mutable RubySaml::Utils::UUID_PREFIX constant with `Settings.sp_uuid_prefix`
- Make Authnrequest, etc. `uuid` attribute to be immutable.
- Initialize Authnrequest, etc. `uuid` attribute when `create` is called, based on `Settings.sp_uuid_prefix`, not when instantiating the Ruby object.
@johnnyshields johnnyshields changed the title v2.x - Reworks SP request UUID generation to remove mutable constant. [WIP] v2.x - Reworks SP request UUID generation to remove mutable constant. Jan 11, 2025
@johnnyshields johnnyshields changed the title [WIP] v2.x - Reworks SP request UUID generation to remove mutable constant. [WIP] v2.x - Rework SP request UUID generation to remove mutable constant Jan 11, 2025
@johnnyshields
Copy link
Collaborator Author

johnnyshields commented Jan 11, 2025

@pitbulk would like to get your feedback on this. I am OK to keep the Authnrequest.uuid = mutator method if there's really a use case for it.

Edit: I've decided to leave the mutator in, so as to minimize change and provide users a workaround if they need it.

@pitbulk
Copy link
Collaborator

pitbulk commented Jan 13, 2025

@johnnyshields it seems tests are failing.

@johnnyshields johnnyshields changed the title [WIP] v2.x - Rework SP request UUID generation to remove mutable constant [READY] v2.x - Rework SP request UUID generation to remove mutable constant Jan 14, 2025
@johnnyshields
Copy link
Collaborator Author

johnnyshields commented Jan 14, 2025

@pitbulk This is ready for final review.

@johnnyshields
Copy link
Collaborator Author

johnnyshields commented Jan 14, 2025

1 failure due to that JRuby Zlib issue still. Seems my previous fix didn't resolve it... This is fine to merge.

@pitbulk
Copy link
Collaborator

pitbulk commented Jan 14, 2025

@johnnyshields, now that prefix can be defined, I believe that should be mentioned what the spec requires in terms of the possible values to be set for the ID attribute:

The type xsd:ID is used for an attribute that uniquely identifies an element in an XML document. An xsd:ID value must be an NCName. This means that it must start with a letter or underscore, and can only contain letters, digits, underscores, hyphens, and periods.

@johnnyshields
Copy link
Collaborator Author

@pitbulk please check now.

@pitbulk pitbulk merged commit 9ab4d3f into SAML-Toolkits:v2.x Jan 15, 2025
20 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants