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

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

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

Conversation

grothoff
Copy link

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:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable? (Yes: don't think applicable).
  • 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.

Copy link
Collaborator

@timnolte timnolte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grothoff there are a bunch of unexpected changes all over the README file. Can you ensure that you only touch the lines you are adding?

I need to review your use case for this some as I'm not quite following why this is needed.

@timnolte
Copy link
Collaborator

@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.

@grothoff grothoff force-pushed the dev branch 3 times, most recently from 3a7b2d4 to 97e36e2 Compare October 17, 2020 07:13
make it possible to grab the login form object and trigger
it from other pages, such as the WooCommerce checkout page.
@grothoff
Copy link
Author

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
Copy link
Author

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.

@daggerhart daggerhart requested a review from timnolte October 20, 2020 11:59
@timnolte
Copy link
Collaborator

@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.

Comment on lines +379 to +399
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 () use ( $login_form ) {
$user = wp_get_current_user ();
if (0 == $user->ID) {
// ID 0 is used to indicate user is not logged in.
// Re-use filter logic to generate login page
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',
array( $login_form, 'handle_redirect_cookie' ) );

}
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This example leads me to believe that there is an attempt to get the login form which should already be available by using the openid_connect_generic_login_button shortcode.

@timnolte timnolte added enhancement Issues & PRs related to new features. status: blocked Issue or PR is blocked. status: needs feedback Needs feedback from the author/developers. labels Oct 21, 2020
@grothoff
Copy link
Author

grothoff commented Oct 21, 2020

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?

@timnolte
Copy link
Collaborator

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.

@grothoff
Copy link
Author

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.

@timnolte
Copy link
Collaborator

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
Copy link
Collaborator

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.

@grothoff
Copy link
Author

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
Copy link
Author

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
Copy link
Author

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.

@timnolte
Copy link
Collaborator

@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 timnolte added needs analysis Issues needing further investigation to the cause and/or change required. status: needs review PR that needs review. and removed status: needs feedback Needs feedback from the author/developers. labels Jan 20, 2021
@timnolte timnolte deleted the branch oidc-wp:develop December 23, 2023 00:56
@timnolte timnolte closed this Dec 23, 2023
@timnolte timnolte reopened this Dec 23, 2023
@timnolte timnolte changed the base branch from dev to develop December 23, 2023 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues & PRs related to new features. needs analysis Issues needing further investigation to the cause and/or change required. status: blocked Issue or PR is blocked. status: needs review PR that needs review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants