openid-connect-generic icon indicating copy to clipboard operation
openid-connect-generic copied to clipboard

Add action 'openid-connect-generic-register-login-form'

Open grothoff opened this issue 5 years ago • 13 comments

This action makes it possible to grab the login form object and trigger it from other pages, such as the WooCommerce checkout page.

All Submissions:

Changes proposed in this Pull Request:

I needed this action for to integrate the plugin with WooCommerce. You can see a more comprehensive example where we integrated WooCommerce, Re:claimID and GNU Taler at https://git.taler.net/woocommerce-taler.git/ to create a one-click account-less shopping experience by providing the shipping/billing address data via OIDC (and OIDC implemented via Re:claim on top of the GNU Name System). This 2-line action was the smallest change I could imagine to use your plugin for the OIDC implementation.

How to test the changes in this Pull Request:

  1. It's adding a trivial action. I can't see how one would even test this. But it works for me ;-)

Other information:

  • [x] Have you added an explanation of what your changes do and why you'd like us to include them?
  • [x] Have you written new tests for your changes, as applicable? (Yes: don't think applicable).
  • [x] Have you successfully run tests with your changes locally? Yes.

Changelog entry

Enter a summary of all changes on this Pull Request. This will appear in the changelog if accepted.

Add action 'openid-connect-generic-register-login-form' to make it possible to grab the login form object and trigger the login.

grothoff avatar Oct 16 '20 20:10 grothoff

@grothoff also, please run the local development setup as your code changes didn't pass code quality and static analysis checks. These checks should have been performed before your local commit was even allowed but it appears you didn't setup your local development environment properly.

timnolte avatar Oct 16 '20 23:10 timnolte

I've modified the patch to undo the removal of whitespaces my editor did automatically, and ensured that the CI passes (not sure why it did not run on my first commit, when I fixed the whitespace it did run automatically this time).

grothoff avatar Oct 17 '20 09:10 grothoff

Hi there! Merging is somehow still 'blocked' by Github with "1 change requested", but I don't know what else I am asked to change (if anything), or if this is simply waiting for a review from @daggerhart. Please do tell me if there is something left for me to do here.

grothoff avatar Oct 19 '20 20:10 grothoff

@grothoff I'm looking at the example code you provided in the README updates and I'm not understanding why you are not just using the openid_connect_generic_login_button shortcode to add the login button to your WooCommerce form? Since this is an action hook that you are adding it is completely bypassing any ability for the shortcode to be added.

timnolte avatar Oct 21 '20 02:10 timnolte

You are right that I'm trying to get the login form. However, if I just do:

add_action( 'woocommerce_before_checkout_billing_form',
            function ( $checkout ) {
				$user = wp_get_current_user ();
   				if (0 == $user->ID) {
	 						echo do_shortcode( "[openid_connect_generic_login_button]" );
	                    }
			}
		  );

then the 'handle_redirect_cookie' logic is not run, and after the login the user is back on the front page instead of continuing with the checkout. Right now, my latest (cleaner) snippet looks like this to get the redirect cookie:

add_action ('openid-connect-generic-register-login-form',
    function ( $login_form ) {
        // show login form at the shopping cart (if not logged in)
	    add_action( 'woocommerce_before_checkout_billing_form',
                    function ( $checkout ) use ( $login_form ) {
						$user = wp_get_current_user ();
   						if (0 == $user->ID) {
	 						print ( $login_form->handle_login_page ('') );
	                    }
						
					});
   
		// Add action to set cookie to redirect back to current
		// (checkout) page after OIDC provided the data.
		add_action(
			'woocommerce_before_checkout_billing_form',
			function ( $checkout ) use ( $login_form ) {
                                 $login_form->handle_redirect_cookie ( );
                        }
			);
	}
);

I don't see a way to also set the redirect cookie with the shortcode. Is there?

grothoff avatar Oct 21 '20 07:10 grothoff

So there is some undocumented features of the shortcode. You can actually set the redirect_to attribute as well as the button_text attribute when using the shortcode. Adding the redirect_to attribute will set the cookie.

timnolte avatar Oct 21 '20 10:10 timnolte

I have tried explicitly with the target URL

echo do_shortcode( '[openid_connect_generic_login_button redirect_to="127.0.0.1:9999/?page_id=14"]' );

and (preferred by me)

echo do_shortcode( '[openid_connect_generic_login_button redirect_to="yes"]' );

and

echo do_shortcode( '[openid_connect_generic_login_button redirect_to]' );

None of them set the cookie, I always get dropped back onto the front page after login.

grothoff avatar Oct 21 '20 11:10 grothoff

The redirect_to should be a URL. That first example is not a valid URL. Please try with a valid URL. If it still doesn't work then this may be related to some more recent redirect issues that have been reported which may be tied to WordPress 5.5. Can you confirm what version of WordPress you are testing on? I'll do some testing again with the shortcode as I had specifically added this enhancement and I had confirmed it was working, but that was prior to WordPress 5.5.

timnolte avatar Oct 21 '20 12:10 timnolte

Also, it seems you should be turning on permalinks as I'm not sure that setting the redirect URL using query parameters works. I'll run through some tests in the shortcode to confirm this.

timnolte avatar Oct 21 '20 12:10 timnolte

I also tried with an http://-prefix for the URI, and using absolute URLs. Nothing works. I am indeed testing with Wordpress 5.5.1.

grothoff avatar Oct 21 '20 12:10 grothoff

Oh, and I'm not surprised that it doesn't work with the shortcode, as the function to set the cookie is AFAIK only called in a hook that is run on the main Wordpress page. (See the 2nd action I'm adding in the snippet.)

grothoff avatar Oct 21 '20 12:10 grothoff

The site I am using is only to test the GNU Taler payment plugin with the re:claim integration. The code should eventually run on various WooCommerce sites, and ideally not require those sites to have any particular other settings (like enabled permalinks). But yes, putting that link like this sucks --- and I'm aware it is too fragile, but for now I'm just trying to get your solution to work at all.

grothoff avatar Oct 21 '20 12:10 grothoff

@grothoff actually no, the login button shortcode does call the handle_redirect_cookie() function. And the login button did work on a client's site on WordPress 5.4 when the enhancement was made. Since WordPress 5.5 there seems to be new reports of overall redirect issues with the cookies being set. I'm looking at changing this and - - using the state as defined by the OpenID Connect standards as a best practice for managing that.

timnolte avatar Oct 22 '20 03:10 timnolte