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

WIP: Use init hook to execute login bypass #440

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

Conversation

ypcs
Copy link
Contributor

@ypcs ypcs commented Aug 13, 2020

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.

@ypcs ypcs self-assigned this Aug 13, 2020
@ypcs ypcs force-pushed the feature/rcp-bypass branch from c61041b to d82bc84 Compare August 14, 2020 05:32
@ypcs
Copy link
Contributor Author

ypcs commented Aug 14, 2020

Seems that hookin to RCP function described in initial version isn't working, but if we hook to init before RCP does, that would do the job. However, this would also mean that on every pageload we run couple lines of the bypass check code, so if we'd like to continue with this path, we would need to do some benchmarking about effects of this.

ypcs added 2 commits August 14, 2020 09:04
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.
@ypcs ypcs force-pushed the feature/rcp-bypass branch from bd4e5df to 9b3b3cc Compare August 14, 2020 06:04
@ypcs ypcs changed the title WIP Add hook for Restrict Content Pro login form Use init hook to execute login bypass Aug 14, 2020
@ypcs
Copy link
Contributor Author

ypcs commented Aug 14, 2020

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.

Copy link
Contributor

@ottok ottok left a 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..."

@ypcs ypcs changed the title Use init hook to execute login bypass WIP: Use init hook to execute login bypass Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants