SAML-Toolkits / wordpress-saml

OneLogin SAML plugin for Wordpress
MIT License
65 stars 74 forks source link

Add more Php constants #34

Open BronsonQuick opened 7 years ago

BronsonQuick commented 7 years ago

Hey folks,

Thanks so much for creating this plugin. I'm currently using it for a client project at the moment and I was wondering if you'd be open to me adding a Pull Request that add some optional PHP constants for the following settings:

If those constants are defined then those settings would be automatically added and I'd also change those fields to read only and have a message saying that those constants have been defined. This would enable developers to keep those constants in version control and not have to set them up manually across multiple environments.

Let me know what you think of the idea. Thanks!

pitbulk commented 7 years ago

Those values are part of the plugin settings and stored on the database so I don't understand why you want to create constants...

If you want to use always the same value..define them on the settings.php file so you will be able to have them on a version control... but this is a custom solution, I don't think that is the common behavior that most of the users of the plugin need.

d3v1an7 commented 7 years ago

Hi @pitbulk, I'd love to see something like this. Although I guess it's not common behaviour, it definitely seems like a progressive way forward (especially for those of us using Bedrock). Having business critical credentials untracked in a db, with no audit trail feels like risky biscuits.

pitbulk commented 7 years ago

But you only store on database saml options and idp metadata (public info). You dont store any credentials...and as I said you can ignore the settings on database and use directly values from the settings.php file.

aidanlane commented 7 years ago

Hi @pitbulk, do you mean that edit onelogin's own settings.php? If so, we would really prefer not to have to do that.

We manage multiple environments (e.g. dev, staging and production), each with their own settings for SSO. If we re-sync the DB, then we have to update the onelogin SSO settings each time. This is very tedious.

Being able to set these programatically allows us to pull in the settings from environment variables and protected stores.

We also would be more than happy to provide a PR.

pitbulk commented 7 years ago

The settings.php file is at the end an array... you can modify it anytime before inject it in the Auth or Settings object.

aidanlane commented 7 years ago

Hi @pitbulk, again, do mean that we would need to edit your plugin's settings.php?

pitbulk commented 7 years ago

You can just edit settings.php and add another require "custom_settings.php" at the bottom of the file and put there the code that you need to override the settings.

Or override the initialize_saml method to use directly a custom_settings.php,

aidanlane commented 7 years ago

@pitbulk, are you really suggesting that we fork and edit your plugin or override one of your internal functions as the only way forward? What happens when you update your plugin, let alone refactor it?

pitbulk commented 7 years ago

Add 1 line to the settings.php:

require "<desired_path>/custom_settings.php";

vs

Create 4 php constants (can be more later) that implies security issues for the rest of the customers of the plugin (because any other plugin can override those constants and compromise the SAML integration).

aidanlane commented 7 years ago

Add 1 line to the settings.php

And end up with a forked plugin, introducing many issues.

Create 4 php constants (can be more later) that implies security issues for the rest of the customers of the plugin (because any other plugin can override those constants and compromise the SAML integration).

But any plugin can do that it now anyway via the WordPress options calls, just as this plugin does. Using constants over options is desired to reduce overheads of setting options all the time, which require DB calls. In fact, security is better with constants in the case where the DB is compromised. They also allow for devs to add encryption at rest if so desired.

pitbulk commented 7 years ago

Ok I'm open to merge a PR that adds PHP constants for IdP data, and create a setting on the "advanced settings section" to enable it use (by default disabled so constants will be ignored.

Is everybody ok with that solution? anyone can contribute that PR?

aidanlane commented 7 years ago

Could it be like 10up's ElasticPress and Delicious Brains' Amazon Web Services plugins, which just take the constants if supplied?

e.g. No constants:

screenshot-1

e.g. With constants: image

pitbulk commented 7 years ago

Having a "valid" SAMLResponse means that you can log in the app with the user info provided... control the IdP Metadata registered on the SAML settings is like control the validation process.

If any malicious plugin defines those IdP settings constants, we are giving admin access to our Wordpress instance. That why I want to set a setting to enable that functionality that you requires, because maybe makes sense for your test scenario... but I want to keep rest of the production instances as they used to be... of course that same malicious plugin can override values of the database.. but that then implies a database change that can be detected in some way.

aidanlane commented 7 years ago

Fair enough :) All of out sites would have the toggle switched on and then the data would be set programatically, right? Thanks again!

tnh commented 6 years ago

I'm assuming this is still open? I'm seeing if there is a way to inject iDp things via environment variables for non production environments using this plugin..

rumcais commented 2 years ago

I would also like to be able to load variables from ENV.