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.0: Final round of param renames #702

Open
wants to merge 1 commit into
base: v2.x
Choose a base branch
from

Conversation

johnnyshields
Copy link
Collaborator

@johnnyshields johnnyshields commented Jul 10, 2024

This PR adds the following deprecation/aliases. This will be the final round for the v2.0.0 release. I really feel these are a important; these names would have saved me considerable time when learning RubySaml. In addition, I see many projects using names like sp_cert in their code, and mapping it to the awkwardly-named existing RubySaml parameters--this is also the same with my project.

To avoid affecting users, this PR states that these aliases will exist until RubySaml 3.0.0.

Old Parameter New Parameter
certificate sp_cert
private_key sp_private_key
assertion_consumer_service_url sp_assertion_consumer_service_url
assertion_consumer_service_binding sp_assertion_consumer_service_binding
single_logout_service_url sp_slo_service_url
single_logout_service_binding sp_slo_service_binding

For reference, these parameters were previously deprecated/aliased. These will also last until RubySaml 3.0.0.

Old Parameter New Parameter
issuer sp_entity_id
idp_sso_target_url idp_sso_service_url
idp_slo_target_url idp_slo_service_url
assertion_consumer_logout_service_url sp_slo_service_url (previously single_logout_service_url)
assertion_consumer_logout_service_binding sp_slo_service_binding (previously single_logout_service_binding)

All other deprecations will continue to be planned to be removed in RubySaml 2.1.0, including compress_request/response and certificate_new, as they have a bigger effect on the code structure.

@johnnyshields
Copy link
Collaborator Author

@pitbulk ready for your review.

@pitbulk
Copy link
Collaborator

pitbulk commented Jul 10, 2024

I understand that when you are new in SAML, there will be confusion about the parameters to be set on configurations.
And in the first days of the toolkit, there were names that carried a lot of confusion, like the settings.issuer

If you analyze the settings, all the settings related to the IdP contain the prefix idp_

The rest are related to the SP or how the SP is going to act.

Adding the sp_ prefix for some parameters could add some semantic context, but at the same time, we are forcing a lot of projects already working with the current names to adapt its projects.

Rather than apply the change in the attributes of settings.rb, that will require a big refactor, we could consider introducing a new method, allowing initializations from a json that includes a dict or a yaml, similar than the one available in python3-saml. See settings.json and advanced_settings.json. Or at php-saml: settings_example.php and advanced_settings_example.php

Do you find those settings intuitive enough?

- certificate --> sp_cert
- private_key --> sp_private_key
- assertion_consumer_service_url --> sp_assertion_consumer_service_url
- assertion_consumer_service_binding --> sp_assertion_consumer_service_binding
- single_logout_service_url --> sp_slo_service_url
- single_logout_service_binding --> sp_slo_service_binding
@johnnyshields johnnyshields force-pushed the 2.0-more-param-renames branch from 98fc5c6 to 000ed7a Compare July 10, 2024 15:13
@johnnyshields
Copy link
Collaborator Author

johnnyshields commented Jul 10, 2024

@pitbulk The core problem in Ruby is that it's not clear what is SP and what is IdP. If you look at the basic settings in both Python and PHP, the distinction between SP and IdP is crystal-clear.

It is not good enough for users to assume "If a parameter doesn't have idp_ then it's an SP parameter." It was really, really confusing.

I think if we are going to rename things in 2.0, we should go for the gold. This PR does not break any existing usage. The deprecation logs will make it obvious, and we can keep them till 3.0.0.

The Python/PHP style Hash/Dict settings could be considered for 3.0 to standardize across languages, but even if we go that route, the renames in this PR are still very useful.

image

@pitbulk
Copy link
Collaborator

pitbulk commented Jul 10, 2024

My proposal is to accept a json or yaml with a similar structure that the one shared, and allow RubySaml Settings to read those parameters

That way, we don't need to change current parameters from Settings, we allow someone new to directly use the json or yaml and we also unify setting name input between the different toolkits.

Also, if we allow to read a json/yaml object, it gonna be easier later for complex projects to have those in the filesystem, or registered somewhere in a database.

@johnnyshields
Copy link
Collaborator Author

johnnyshields commented Jul 10, 2024

I don't have the time/energy to add JSON/YAML interface for the imminent 2.0 release--if we do it, we should take time and design the new interface carefully.

Even if we do JSON/YAML in 3.0, this PR will help today and will also make both external and internal code much more readable.

I'm not understanding your hesitation on this PR, given that:

  1. This PR will not break or change anything for existing users--the 1.x params continue to work.
  2. We are committing to keeping the 1.x params until (at least) 3.0.0
  3. We are already renaming a few params already--why not just clarify all the names, so users only need to fix the deprecations once?

I really don't think this will be a barrier for anyone to use 2.0.0...

If not adding this in 2.0.0, would you consider adding this in 2.1.0?

@johnnyshields johnnyshields changed the title v2.0: Final round of param renames [READY] v2.0: Final round of param renames Jul 13, 2024
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