Implement singleton pattern for OpenID_Connect_Generic class

[RobjS]

This will allow developers who want to be able to call methods belonging to this class (or methods belonging to any of this class's properties) to do so, without having to create a new instance, and therefore repeat all the bootstrapping.

Instead, they will just be able to call OpenID_Connect_Generic::get() to retrieve the singleton.

[daggerhart]

Hi @RobjS,

I've thought about this PR a number of times and ultimately don't want to turn the main plugin class into a singleton. The point of the main plugin class is to hook the rest of the plugin into WP, and nothing more. If there are some methods on this class that a developer may want access to, then it makes more sense to move those methods to another class that is either a stateless service/utility thing, or if the method is closely related to the client or client wrapper, maybe move it there.

Do you have a use-case that needs access to someone methods on the main plugin class? That would help think through what other changes might be needed.

[RobjS]

Hi @daggerhart,

Thanks for responding and merging the other two PRs. Hopefully it'll be useful if I provide some context around the issue we're having and what I was hoping to achieve with this PR.

We're seeing a small but sustained number of missing state errors. These have persisted even after applying the fix for the race condition (#171) and increasing the state time limit from a few minutes to several hours.

I think one possible cause of these is people clicking the OpenID Login button after the provided state token has expired (even given our increased state time limit). One potential fix for this could be to start the countdown on the state token validity when the login button is clicked, rather than when the page containing the login button is served.

Rather than open a PR making that (potentially quite substantial) change within the plugin, I was aiming to use the new openid-connect-generic-login-button-url filter from #191 to point users initially to an internal page when the button was clicked, and at that point call the client wrapper's get_authentication_url() method to generate the relevant login URL & state token, and redirect the user to that URL.

However, that method would only be available by new-ing up the client wrapper and its dependencies (and their dependencies in turn), which feels unnecessarily clunky. Instead, I was hoping this change would allow me to do something like:

$openIdConnectGeneric = \OpenID_Connect_Generic::get();
$url = $openIdConnectGeneric->client_wrapper->get_authentication_url();
wp_redirect($url);

My hope was that the change in this PR was self-contained enough that it would enable this level of customisation, without having any impact on other plugin users. But if you have a preferred approach, I'm happy to amend the PR accordingly 👍

[RobjS]

Hi @daggerhart, do you have any more thoughts on the above?

[daggerhart]

@RobjS Yes! Thanks for asking.

I don't mind making a singleton (or global variable) out of the plugin class for this version 3.x, but I wouldn't want people to use it directly. I'd rather that be paired with some simple functions for getting the specific thing people might want.

For example:

function oidcg_get_authentication_url() {
  return \OpenID_Connect_Generic::get()->client_wrapper->get_authentication_url();
}

This way, we aren't stuck with the singleton pattern forever just for the sake of backwards compatibility.

I imagine that the 4.x version of this module would be significantly different in architecture, and I wouldn't want people to start relying on the actual singleton within their site. Same applies to the client_wrapper, and other classes. Atm, the client wrapper is doing Waaaay too much work and needs to be split up into different services. If people start relying directly on that class for its methods, then we're a little bit stuck with that going forward.

What do you think?

[timnolte]

@daggerhart I would agree that it would be much better to provide global functions that we can have documented as the supported what of providing this functionality. This will allow us to adjust them, as needed, when/if the plugin structure changes.

[daggerhart]

@RobjS,

I thought about this more over the long weekend, and think we can move forward with this PR with a few changes.

1. Let's change the ::get() method name to ::instance() -- this is very minor, just preference.
2. Can you create a new includes/functions.php file, that makes use of the new singleton instance to provide the specific things you want to use? For example, the get_authentication_url() method from the client wrapper, or any other parts of the plugin you need in the short term. Doesn't have to be extensive, just what you need for now.

[RobjS]

Thanks @daggerhart, that all sounds good to me.

I've made those changes to the PR. It's just the one function in functions.php for now, but hopefully that works as a starting point.

[timnolte]

@RobjS please ensure you are using the local development environment provided by the plugin to ensure that your PR meets coding standards and code quality requirements. Right now your PR has failed some of the code quality checks. When setting up the local plugin development environment things are setup to ensure that your code is checked on your local machine before you are even able to commit code changes. Thanks!

[RobjS]

Thanks @timnolte. Sorry about that, it looks like a lot of tests & CI have been added since I originally started on the PR 👍

Everything should be passing now.

[timnolte]

@RobjS since we don't currently have unit testing in place, I'm assuming that you've tested these changes fully on your own development environment and have confirmed the plugin is still working as expected?

[gassan]

Why not simply to rename boostrap() to instance(), make constructor private and modify instance() like followed:

public static function instance() {
    static $instance;

    if ($instance === null) {
        .....
        $instance = new self( $settings, $logger );
 
        ... // add actions and filters
    }

    return $instance;
}

It is better to avoid publishing $instance variable. class should be accessible only with static instance() method

[gassan]

it would be nice to add the next function:

public function get_setting_values() {
    return $this->settings->get_values();
}

we could read settings (for our plugins) but not change them.

For example I need Register User Endpoint and in my case that would be:

preg_replace('/auth$/', 'register', OpenID_Connect_Generic::instance()->get_setting_values()['endpoint_login]);

[timnolte]

@gassan the plugin uses standard WordPress core methods for settings, I don't see a need to expose our own method when you can just get this using core WordPress functionality.

[gassan]

@timnolte Of course we can read settings from db. Nevertheless, I would like to ask @daggerhart to check whether the $settings can be public.
Thanks.

[timnolte]

@gassan you do realize that I'm also a maintainer of the plugin?

My point is that what methods are made public need to be carefully considered and should only be done where there is not already a method for accomplishing this. That is also why the plugin already supports so many hooks and generally provides what is needed through those means, but in some cases there is a need that can't be accomplished without a code change.

My stance is that there is no sense adding additional code to maintain, and develop tests for, that already have a working solution. From my standpoint the primary focus of changes right now need to be those that fix real issues that are preventing people from using the plugin, or requests that, if implemented, will enhance the security and the usability with existing IDPs.

[RobjS]

@RobjS since we don't currently have unit testing in place, I'm assuming that you've tested these changes fully on your own development environment and have confirmed the plugin is still working as expected?

@timnolte Yes, tested in my dev environment and the plugin is behaving as expected.

[timnolte]

@daggerhart I'm going to approve these changes.

[timnolte]

@RobjS FYI, would you be able to get this branch updated with the latest changes in the dev branch? I'm pushing out a fix release and will be looking to bring in as many approved PRs into the next feature release soon. Thanks!

[RobjS]

Thanks @timnolte - done.

[timnolte]

@RobjS my apologies, I'm actually getting PRs merged in ready for a new release, I know it's been a long time coming. For some reason GitHub isn't allowing my to rebase your branch to bring it up-to-date. Would you be able to get your branch up-to-date with the latest in our dev branch and then I'll get this merged in and ready for the release. Thanks!

[timnolte]

@RobjS never mind on my last comment. I made some updates to the repository configuration and that appears to have resolved the issues I was seeing.