dmchale / disable-json-api

Public repo for the "Disable REST API" WordPress plugin, currently with 90,000+ active installs in the wordpress.org repository
10 stars 9 forks source link

whitelist_routes should return original $access if $access is already a WP_Error object #16

Closed tangrufus closed 7 years ago

tangrufus commented 7 years ago

Reason: do not suppress previous error message

dmchale commented 7 years ago

I would preferably have the plugin return the "lockdown" error BEFORE other errors are raised... since we're hooking at priority 20 currently, do you think it makes more sense to move it to priority 5 so it happens BEFORE anything at the default level?

We could still make the update to return $access if it is a WP_Error, but at least there would be much less risk of other things happening unless someone was very insistent on their actions happening early also.

dmchale commented 7 years ago

I thought about it more today - I think it actually makes sense to stay hooked at 20. The best way to ensure DRA returns its own errors is to hook LATE, to ensure as few things as possible (hopefully nothing) hooks after it and overrides our own WP_Error() response. Hooking early means anything which happens at the default priority may take our own results and throw it away.

So - best plan (I think) is to hook late, return prior errors so they are not suppressed, and do our thing last.

This would mean that the "early return access" commit would be good (https://github.com/dmchale/disable-json-api/pull/19/commits/ccbead70b6be4c04efa95ae0c9ed58a3af27b517) but we wouldn't need the other commit to change the priority (https://github.com/dmchale/disable-json-api/pull/19/commits/4bd0c4b510ff4ddb641df7cb97fa5f7015483b68)

Agree/disagree @TangRufus ?

tangrufus commented 7 years ago

agree, stay 20 but early return $access

Side note: if you want to implement https://wordpress.org/support/topic/feature-request-filter-hook-for-auth-error/ , now seems a good time to get it done

dmchale commented 7 years ago

Implemented early exit for people allowed to view the API (for now this is only people who pass is_logged_in(), but you never know if this could be extended later), and also added the filter - good call on that : https://github.com/dmchale/disable-json-api/commit/3b692df6011d5cc94837f264b1048161f1531eb9

Updated how WP_Error is handled. I decided it was probably best instead of ONLY returning previously-set WP_Error objects to instead add the DRA error to the pile and return it with any existing errors using the WP_Error->add() function. : https://github.com/dmchale/disable-json-api/commit/182453636ccfba2057a42a896d8d9d457c8d17ad

And we're still hooking at 20 :)