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

Bugfix/disable authentication #93

Merged
merged 7 commits into from
Mar 13, 2019
Merged

Bugfix/disable authentication #93

merged 7 commits into from
Mar 13, 2019

Conversation

rickard-berg
Copy link
Contributor

Disabling authentication by adding an empty array to the filter "satispress_authentication_servers" currently doesn't work.
I've fixed that by adding the satispress_view_package- and satispress_download_package-capabilities to all users, if the filter "satispress_authentication_servers" returns an empty array.

Rickard Berg and others added 4 commits March 12, 2019 08:31
src/ServiceProvider.php Outdated Show resolved Hide resolved
@bradyvercher
Copy link
Member

Thanks for the PR @rickardbergatangrycreative! I think the capabilities were added after settling on returning an empty array for the authentication servers to disable authentication, so I hadn't considered this.

Adding a new entry in the container seems like it could cause issues if the authentication.disable value were changed separately from the servers list. It also feels a little weird to be disabling authentication in the capabilities provider.

How about dropping the authentication.disable entry and moving the user_has_cap filter out of the Capabilities provider and into the Authentication provider?

@rickard-berg
Copy link
Contributor Author

@bradyvercher I agree, and I actually tried to come up with a solution with the Authentication provider, but couldn't figure it out for some reason and then got stuck with the idea of capabilities.

I've implemented your suggestion now.
I'm new to WordPress development, but hopefully I moved everything correctly.

@bradyvercher
Copy link
Member

Thanks for making that change and welcome to WordPress development! Feel free to reach out if you have any questions about anything.

There are a few additional minor things I'd like to see before merging this, so I'll leave a couple comments on the code.

src/Provider/Authentication.php Show resolved Hide resolved
src/Provider/Authentication.php Outdated Show resolved Hide resolved
src/Provider/Authentication.php Outdated Show resolved Hide resolved
src/Provider/Authentication.php Outdated Show resolved Hide resolved
src/Provider/Capabilities.php Outdated Show resolved Hide resolved
@bradyvercher bradyvercher merged commit 8321e59 into cedaro:develop Mar 13, 2019
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.

3 participants