saml-idp / saml_idp

Ruby SAML Identity Provider, best used with Rails (though not required)
MIT License
263 stars 181 forks source link

Add support for signed and compressed responses #174

Closed pepito2k closed 1 year ago

pepito2k commented 2 years ago

We needed an app to act as an IdP and allow users to sign in to different service providers.

Some of those vendors require the assertions to be signed and the response to be compressed.

To accomplish this task an IdP initiated flow is used, a SAML response is sent to the service provider including the information they need to sign in the user into their system.

Zogoo commented 2 years ago

@pepito2k it seems like your branch is bit outdated, if you do rebase from current master branch we happy to review your idea.

pepito2k commented 2 years ago

@Zogoo it's not that my branch is outdated, I'm trying to merge changes made in a really old fork of this gem, I already rebased and merged (it was a painfully process) to be up to date in order to create this PR.

If you see something that's wrong or out of the place, I am happy to fix my PR with your suggestions.

pepito2k commented 2 years ago

@Zogoo ?

Zogoo commented 2 years ago

@Zogoo it's not that my branch is outdated, I'm trying to merge changes made in a really old fork of this gem, I already rebased and merged (it was a painfully process) to be up to date in order to create this PR.

If you see something that's wrong or out of the place, I am happy to fix my PR with your suggestions.

@pepito2k I think there are a lot of things looked like changed in your repo. And it makes difficult to review it. I would suggest to put your changes from the latest master branch. And I would suggest to avoid creating unnecessary changed like syntax changes in code that you are not creating or updating. Another example: Current maser branch is already using git action and PR looked like adding new files.

pepito2k commented 2 years ago

@Zogoo I don't see how my PR is adding any github actions: image

Anyways, if you don't feel like merging, I don't really care, we are already using my version of the gem in production. It just felt right to share back to the community, but if the community is not willing to actually receive contributions, there's nothing else I can do.

Thanks.

Zogoo commented 2 years ago

@pepito2k I'm sorry it seems like we have misunderstood about contribution. We are very happy to see your valuable feature. I have just seen that some html files were marked as new files and some of files were auto corrected (probably your editor). spec/rails_app/app/controllers/saml_idp_controller.rb - auto corrected. app/controllers/saml_idp/idp_controller.rb - marked as new file app/views/saml_idp/idp/create.html.erb - marked as new file So, I wasn't sure what is the real commits from you. I know code it self quite uncommon coding style and at-least rubocop should be applied there. But that kind of changes could be 2 different PR then our review process will be bit faster. I hope you can understand my concern.

pepito2k commented 2 years ago

I'm sorry, I totally lost interest on this. I've sent a pull request 6 months ago, nobody took the effort to review anything.

I can see someone reviewing this in other 6 months from now asking for changes that I will not do, I don't even have the code on my computer anymore, we made changes to this gem to accomplish our needs and we thought it was a good idea to share back.

As I said before, we are already using this in production, so nothing will change for us if you accept this PR or not.

Zogoo commented 2 years ago

I'm sorry, I totally lost interest on this. I've sent a pull request 6 months ago, nobody took the effort to review anything.

I can see someone reviewing this in other 6 months from now asking for changes that I will not do, I don't even have the code on my computer anymore, we made changes to this gem to accomplish our needs and we thought it was a good idea to share back.

As I said before, we are already using this in production, so nothing will change for us if you accept this PR or not.

@pepito2k thanks for your contribution. I'm happy that gem helped your production environment feature. Meanwhile, let's see other guys how react your PR and if we find out that it's helping other peoples, let us clean up your PR and merge it to master version. Thanks again and lots.

jphenow commented 1 year ago

closing with #190