incompatibility with two-factor plugin after 1.1.2 update

[pjv]

After upgrading to version 1.1.2 on my store site, I'm no longer able to complete the login using Civic unless I deactivate the plugin two-factor (also on github).

After scanning the QR and authorizing on the mobile app, my site shows me the grey screen overlay and the Civic spinner, but it never completes the login - spinner just keeps spinning.

Looking at the debug log, two-factor is throwing some warnings and notices:

[26-Oct-2017 15:28:25 UTC] PHP Warning:  Missing argument 2 for Two_Factor_Core::wp_login(), called in /some/path/wp-includes/class-wp-hook.php on line 298 and defined in /some/path/plugins/two-factor/class.two-factor-core.php on line 227
[26-Oct-2017 15:28:25 UTC] PHP Notice:  Undefined variable: user in /some/path/plugins/two-factor/class.two-factor-core.php on line 228
[26-Oct-2017 15:28:25 UTC] PHP Notice:  Trying to get property of non-object in /some/path/plugins/two-factor/class.two-factor-core.php on line 228
[26-Oct-2017 15:28:25 UTC] PHP Notice:  Undefined variable: user in /some/path/plugins/two-factor/class.two-factor-core.php on line 234
[26-Oct-2017 15:28:25 UTC] PHP Warning:  call_user_func_array() expects parameter 1 to be a valid callback, function 'wp_login_viewport_meta' not found or invalid function name in /some/path/wp-includes/class-wp-hook.php on line 298

The authors of the two-factor plugin are aiming to have it rolled into core WP, so it would be important to make Civic login compatible with it.

EDIT: 1.1.1 was compatible with two-factor.

[cheelahim]

@pjv Thank you for bringing this up. I see where the issue with those warnings is but this is not the reason of your problem. I'm not sure if the whole workflow is correct. As I understand the two-factor auth can be enabled individually for specific user. But Civic authentication already implements similar workflow (Civic App essentially is your second factor). Having two-factor plugin activated and enabled for specific user makes the Civic authentication flow interrupted by the two-factor plugin to ask for second factor. So there is a conflict between them. What are you trying to achieve by doing so?

[pjv]

hi @cheelahim - I understand your explanation. What I would like to achieve is to have both options available: conventional TOTP two-factor auth and the Civic QR code auth (not really 2FA actually, but I understand what you mean). This was working for me in v. 1.1.1 .

The reason I want to keep available the conventional TOTP is because since we can't turn (or aren't turning) off conventional username / PW logins in wordpress, deactivating the 2FA for the conventional login then becomes a security issue because it leaves open the possibility that an attacker could authenticate with just my username and password without having to use 2FA.

I suppose it would be possible to bypass this security issue by having the Civic plugin turning off conventional username / PW authentication for user's who are using Civic altogether, but this seems like a bad solution to me. I think it would be better to have the two methods coexist so the user can optionally use whichever is more convenient without the loss of security.

[cheelahim]

@pjv I got your point. We will discuss this with two-factor plugin maintainers to find a solution. Maybe we could summon @kasparsd to ask for advice.

[pjv]

I just sent you guys a PR to fix this issue. Based on the suggestion from @kasparsd here, it just deactivates the two factor wp_login action while civic's wp_login action is running.

working for me.

[cheelahim]

Hey @pjv. I know this fix solves your issue with plugins compatibility but I'm very reluctant to accept the #6 PR. My concern is that it is too specific to be included in the main codebase i.e addresses single problem. This creates a dangerous precedent. If we will patch the core for every specific plugin, the plugin could bloat very quickly. The more future proof solution would be to introduce a custom action in wp_login handler, so anyone could hook into that and add some custom logic (i.e. disable conflicting plugin) in their own plugins.

[pjv]

Hi @cheelahim I understand that concern perfectly and I can re-submit with a do_action call in the wp_login function, and then write my own custom code to disable two factor.

Before I do that, however, I just want to reiterate something that I mentioned in the first post of this issue to see if it changes things for you. The authors of the Two Factor plugin are aiming to eventually merge it into WP core. If / when that happens, it seems like it wouldn't be a bad idea to have this particular specific workaround in place.

[cheelahim]

I totally see your point. If that would be the WP core, it would much easier to justify this. But we're having this discussion for some time now and it's still not there. If/then two-factor will become part of WP core I'm ready to get back to this proposal.

Actually we do 'civic_sip_auth' action upon civic SIP responce. Have you tried to hook into that and disable two-factor plugin?

[pjv]

Actually we do 'civic_sip_auth' action upon civic SIP responce. Have you tried to hook into that and disable two-factor plugin?

Yup, that works. Closing this and PR #6.

For posterity, in case anyone else needs this, here is code for functions.php or a custom plugin:

add_action('civic_sip_auth', 'plx_civic_two_factor');
function plx_civic_two_factor () {
  remove_action( 'wp_login', array( 'Two_Factor_Core', 'wp_login' ), 10, 2 ); 
}

[cheelahim]

Glad this worked out for you. We appreciate a constructive dialog. Thank you @pjv.