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

Allow for use of env variables to set redirect URIs #173

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

StoneWaves
Copy link

Load redirect_uri and logout_redirect_uri from an environment variable if its set, otherwise use the value stored in the database.
This allows the redirect uris to be set dynamically for review applications without needing to change a value in the database.

@psignoret
Copy link
Owner

Thanks for the pull request! Would you mind elaborating a bit on how this would be used? Could you provide a bit more details on your scenario?

@bradkovach
Copy link
Contributor

Would this be better off filtered/hooked instead of using env, so that the behavior could be managed by WordPress plugins?

@StoneWaves
Copy link
Author

We have various dynamically generated review environments that share the same database for testing purposes.
This would allow us to override the value that's stored in the database with the URL of each site allowing us to use the plugin for that site and not get redirected somewhere else.

@psignoret
Copy link
Owner

Thanks @StoneWaves, that makes sense. I agree with @bradkovach that this would be better implemented with a filter/hook so that the entire config can be overwritten by a different plugin in a more WordPress-y way.

Because this is some very sensitive configuration, we might want to have another setting (one which would not filterable) that enables the filter (e.g. "Allow other plugins to overwrite these settings")). (Though whether or not this is useful is debatable, since another plugin could still get to this data anyway.)

@bradkovach
Copy link
Contributor

A define could be used to enable "Allow other plugins to overwrite these settings" and this could be presented in the UI as a warning along the lines of "Other plugins may be able to overwrite these settings. Unset AADSSO_HOOKS_ACTIVE to disable this behavior."

Keeping this toggle in database has no real security benefit, since--as @psignoret mentioned--any plugin can use get_option and update_option or even add_option.

@psignoret psignoret force-pushed the master branch 2 times, most recently from 0320183 to 37c8428 Compare July 18, 2018 18:50
@peterjgordon
Copy link

How's that? It's been re-written to just apply filters now, allowing another plugin to pull from environment variables. Let me know if you'd like any changes made.

@peterjgordon
Copy link

@psignoret, @bradkovach, would appreciate your input here. Is this ready for merge?

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.

None yet

4 participants