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

[WIP] Configure IdP and SP with configuration object #183

Closed
wants to merge 12 commits into from

Conversation

Zogoo
Copy link
Collaborator

@Zogoo Zogoo commented Jul 13, 2022

The idea of this PR is that Gem should not care about how to store metadata. Because too much involving developer decisions makes this gem harder to maintain.
#71

Let's drop persistance of metadata and how to refresh it because those implementation could be done with another gem (top of this gem) for different web frameworks. Or even it could be done by developers If we just accept "SamlIdp::IncomingMetadata" (IMO this should be also renamed) for all configuration of IdP.

@Zogoo Zogoo self-assigned this Jul 13, 2022
@jphenow
Copy link
Collaborator

jphenow commented Jul 14, 2022

Makes sense I think. Can much of the same be accomplished without the config? Might be good to document.

Would definitely say this is a change of API to the point that it might be good to place this in a new major version - in which case if we wanted to make a lot of breaking changes, might be good to figure out if we want a branch for vnext and maintain master for bugs until vnext is ready or how to go about managing the branches there.

@Zogoo Zogoo changed the title No need to persist metadata [WIP] No need to persist metadata Jul 15, 2022
zogoo added 2 commits July 15, 2022 20:23
…or this reason dropping SP metadata logic. Because IdP project can decide how to provide metadata.
@Zogoo Zogoo closed this Jul 16, 2022
@Zogoo Zogoo reopened this Jul 16, 2022
@Zogoo Zogoo changed the title [WIP] No need to persist metadata [WIP] Support only object for SP provider and SP metadata, Drop metadata file loader, persister methdos Jul 16, 2022
@Zogoo Zogoo changed the title [WIP] Support only object for SP provider and SP metadata, Drop metadata file loader, persister methdos [WIP] Support only object for SP provider and SP metadata, Drop metadata file loader, persister methods Jul 16, 2022
@Zogoo Zogoo marked this pull request as draft July 16, 2022 07:23
@Zogoo Zogoo changed the base branch from master to v0.2.0-saml-lib January 3, 2024 18:16
@Zogoo Zogoo force-pushed the feature/simplify_gem branch from 7e85449 to 43a9b2d Compare January 3, 2024 20:00
@Zogoo Zogoo force-pushed the feature/simplify_gem branch from f4dc78e to 5021c34 Compare January 3, 2024 22:19
@Zogoo Zogoo added the breaking-change Changes should be treated with major version label Jan 4, 2024
@Zogoo Zogoo added this to the Simplify gem for SAML milestone Jan 4, 2024
@Zogoo Zogoo force-pushed the feature/simplify_gem branch 4 times, most recently from d8ca283 to 220ba26 Compare January 4, 2024 23:18
@Zogoo Zogoo force-pushed the feature/simplify_gem branch from 220ba26 to 5fa6eba Compare January 5, 2024 00:24
@Zogoo Zogoo changed the title [WIP] Support only object for SP provider and SP metadata, Drop metadata file loader, persister methods [WIP] Configure IdP and SP with configuration object Jan 5, 2024
@Zogoo Zogoo force-pushed the feature/simplify_gem branch 2 times, most recently from c0a54c0 to d4b376d Compare January 7, 2024 19:14
@Zogoo Zogoo force-pushed the feature/simplify_gem branch from d4b376d to 0ed50c6 Compare January 7, 2024 23:44
@Zogoo
Copy link
Collaborator Author

Zogoo commented Jan 8, 2024

@jphenow, @mjobin-mdsol I have almost finished the required code changes and started working on test cases. Could you guys please do a quick review to confirm the changes are okay?

The main change is that the IdpConfig and SpConfig objects allow to configure of the IdP for specific service providers. And it's not a global config because this allows 2 things

  1. Gem users can use it for multiple SPs. This means SP config can be different for different SPs. Like X509 certificate, NameIDFormat etc.
  2. Gem users can make decisions about how they want to manage configs like DB, Global Object, Redis etc.

@Zogoo Zogoo requested review from jphenow and mjobin-mdsol January 8, 2024 15:07
Comment on lines +12 to +13
This was originally setup by @lawrencepit to test SAML Clients. I took it closer to a real SAML IDP implementation.
Forked from <https://github.com/lawrencepit/ruby-saml-idp>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This was originally setup by @lawrencepit to test SAML Clients. I took it closer to a real SAML IDP implementation.
Forked from <https://github.com/lawrencepit/ruby-saml-idp>
This was originally a fork of @lawrencepit's project <https://github.com/lawrencepit/ruby-saml-idp> which was for testing SAML Clients.
This project implements an actual SAML IdP.


Be sure to load a file like this during your app initialization:
2. SP data such
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
2. SP data such
2. ServiceProvider configuration

config.signed_assertion = false # Default: true which means signed assertions on the SAML Response
config.compress = true # Default: false which means the SAML Response is not being compressed

Principal (e.g. User) is passed in when you `encode_response`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Principal (e.g. User) is passed in when you `encode_response`
# Principal (e.g. User) is passed in when you `encode_response`

Comment on lines +127 to +128
If you have a method called `asserted_attributes` in your Principal class,
there is no need to define it here in the config.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If you have a method called `asserted_attributes` in your Principal class,
there is no need to define it here in the config.
# If you have a method called `asserted_attributes` in your Principal class,
# there is no need to define it here in the config.

If you have a method called `asserted_attributes` in your Principal class,
there is no need to define it here in the config.

config.saml_attributes =>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this showing what saml_attributes returns? I'm confused by the => here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I forgot to mention that I haven't work documents yet. Once I finish code, I will update all documents.

# persistent: -> (p) { p.id },
# },
# }
configure_sp do |config|
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe spell out service_provider instead of sp? Abbreviations get confusing IMO.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is this defined? I'm having trouble following how this gets configured/used.

@@ -229,13 +202,13 @@ The second parameter is optional and default to your configuration `SamlIdp.conf
To act as a Service Provider which generates SAML Requests and can react to SAML Responses use the
excellent [ruby-saml](https://github.com/onelogin/ruby-saml) gem.

## Author
# Author
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just remove the author section.

Copy link
Collaborator

@mjobin-mdsol mjobin-mdsol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I glanced at it, but I am sorry I don't have useful comments at the moment.

@Zogoo
Copy link
Collaborator Author

Zogoo commented Oct 24, 2024

This was too big PR for the current gem, I will create small PRs that allow transfer slowly to v2

@Zogoo Zogoo closed this Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Changes should be treated with major version enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants