search

cedaro / satispress

Expose installed WordPress plugins and themes as Composer packages.
500 stars 49 forks source link

Bugfix/disable authentication #93

Closed rickard-berg closed 5 years ago

rickard-berg commented 5 years ago

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.

bradyvercher commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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.