-
Notifications
You must be signed in to change notification settings - Fork 16
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
WIP: Use init hook to execute login bypass #440
base: master
Are you sure you want to change the base?
Conversation
c61041b
to
d82bc84
Compare
Seems that hookin to RCP function described in initial version isn't working, but if we hook to |
Customer notified us about `wp-test` testsuite not working on their site, which uses Restrict Content Pro for providing login form. It seems that this plugin completely overrides WordPress login processing, and doesn't trigger `login_init` action at all. Based on WordPress developer documentation <https://developer.wordpress.org/reference/hooks/login_init/> this hook *should* be fired when login form is initialized, but it seems that RCP has done it's own decisions to skip the hook. Instead of login-specific hook RCP uses WordPress core `init`, which gets triggered on every page load. This might affect site performance, as it basically means that every single pageload runs at least some `if` checks related to login functionality. This commit modifies how our test suite login bypass feature works, by hooking to that `init` hook. We first check if visitor is trying to do the bypass, and only if certain variable has been defined, we continue with the checks. This seems to fix the issue with RCP. However, as this modifies how *all* sites do the login bypass, this modification needs lots of testing before we can start using this. Also, as this makes WP run some bypass checks on each page load, it might affect WordPress performance. This means that we need to conduct extensive benchmarking before merging this.
bd4e5df
to
9b3b3cc
Compare
Note, PR still contains some extra comments that should be removed before merging. However, this now needs at least benchmarking, and optionally comments from sites using this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. As it adds one more hook to catch the login attempt, this seems to me as a pretty safe change. Did not test running code.
One minor mismatch in indentation starting from "add_action('login_init..."
Customer notified us about
wp-test
testsuite not working on their site,which uses Restrict Content Pro for providing login form. It seems that
this plugin completely overrides WordPress login processing, and doesn't
trigger
login_init
action at all.Based on WordPress developer documentation
https://developer.wordpress.org/reference/hooks/login_init/ this hook
should be fired when login form is initialized, but it seems that RCP
has done it's own decisions to skip the hook.
Instead of login-specific hook RCP uses WordPress core
init
, whichgets triggered on every page load. This might affect site performance,
as it basically means that every single pageload runs at least some
if
checks related to login functionality.
This commit modifies how our test suite login bypass feature works, by
hooking to that
init
hook. We first check if visitor is trying to dothe bypass, and only if certain variable has been defined, we continue
with the checks. This seems to fix the issue with RCP.
However, as this modifies how all sites do the login bypass, this
modification needs lots of testing before we can start using this. Also,
as this makes WP run some bypass checks on each page load, it might
affect WordPress performance. This means that we need to conduct
extensive benchmarking before merging this.