Session refresh should happen in determine_current_user

[pjeby]

The current strategy for refreshing tokens and expiring the current session is to run at init or wp_loaded, but after observing the many corner cases for logout and token timeouts (I haven't posted them all as issues yet, believe it or not!), I think that the root of the problems with logout scenarios is that the user is first logged in, and then logged back out.

However, Wordpress has a determine_current_user filter that is used to tell if the user is logged in in the first place. If token refresh/validation runs there, it would be able to simply return false if the session couldn't be continued, just as if their session had timed out normally due to cookie expiration. The request would then proceed just as it would have normally.

The upside to this approach is consistency in interaction with Wordpress as a whole. Rather than some plugins seeing a logged-in user followed by a logout, plugins would simply see no user at all.

The downsides are twofold: first, the filter would need to be added very early in the bootstrapping process instead of init. And there'd need to be some error checking to detect whether the current user has already been loaded when the filter is added, and to then add an admin message warning that there's an incompatible plugin in use. (Or possibly just die(), since the result is an untethered session that can't be terminated.)

Second, we'd have to give up the current way of displaying token refresh errors when the refresh fails. Wordpress has no way that I know of to add user-visible error messages to the current page, so we'd have to rely solely on logging for identifying such errors. (And we could add an action hook so other plugins could act on the errors, perhaps by using a specific plugin or theme support for user-visible messages.)

I think this second issue may be a blessing in disguise, though: the end user has no idea WTF they can do about any error that happens with the IdP anyway; silently logging them out is actually pretty reasonable behavior that has minimal risk of user confusion. The downside would be that it would take a longer time for an admin running a buggy IdP to find out about the issue, as people might just think the timeout was a bit irregular. :grin:

Anyway, that's my current thought process on logging the user out. There would still be one corner case this doesn't fix, which is clicking "Log out" when your token has expired. Just as it does currently, that would lead to getting a confirmation prompt, and if you agreed to the prompt, getting an error from the IdP about the session not being active. (I think figuring out a fix for that will have to wait until the rest of this is done, though.)

[pjeby]

On further reflection, I've noticed that there's a pattern to the kind of issues I'm finding with the plugin: nearly all the things I'm reporting as bugs or requesting as features have to do with front end users for things like Woocommerce, LearnDash, etc. But ISTM that this plugin was actually written with back end users in mind, who are probably used to seeing Wordpress's interstitial error messages and being redirected to the admin dashboard on login or the home page on logout.

Since I'm not in that latter category (I'm not a WP user by choice), I'm not sure if the sorts of changes I'm proposing and implementing are appropriate for the plugin.

Or to put it another way, do you see the direction in my issues and PRs as "OIDC Generic, version 4", or stuff that should just be done in a fork?

[daggerhart]

Regarding the 2nd issue about direction:

I think you should become a co-maintainer.

I'm also not a full-time WP dev. Used to be, but now I do much more Drupal. I created this plugin because I needed it for a project, but then that project changed direction. I continue to maintain the plugin because it's fun to do and people use it, but I actually don't have a use-case atm.

I have multiple local test sites setup for various IDPs, and will continue working on it, but I can't make the best possible plugin on my own. You seem to have a strong handle on oauth specs and a desire to make this plugin better for all types of users. So I think it makes the most sense for you to start rocking a version 4 with your crazy ideas about having good UX ;).

Let me know if you're interested, I'll add you as a collaborator and start a "version 4 planning" issue where we can collaborate.

Regarding the 1st issue about session refreshing... I'm going to have to get back to you on that when I've had more time to process it :D

[pjeby]

So, maintainer sounds fine, except that I'm not sure how long I can commit to support -- especially since I'm worried that I might end up needing to replace the other 2400 lines of the plugin besides the ones I already deleted. :wink:

Like you, my interest is for one specific project. I'm finding the further I go along on that project, that the session management design of this plugin simply doesn't work for what I want, which is more or less what Amazon, Github, and other big boys do: leave you logged in without much fuss most of the time, but force you to have authenticated within some time window to do higher-privilege operations. That means, in fact, that I actually could have sessions that don't bother with the refresh tokens or possibly even access tokens, as long as the plugin supports backchannel logout and revocation. (Keycloak provides these as non-standard hooks, but Keycloak's what I'm going to be using.)

So, aside from the session management and logout issues, the biggest deficiencies for me in the plugin as it stands is the lack of API for controlling the flow. There's no way for an arbitrary piece of code in another plugin to say, "give me a login URL with these parameters", for example, let alone, "check if the user authenticated recently enough and if not force re-login", or "try and see if this anon user has logged on into the IdP via another app in this browser, and log them in here if so".

I'm really pretty torn because I don't want to invent wheels here, but I'm also thinking that maintaining backward compatibility might be a PITA. So I'm not 100% sure even what I want to do, let alone how it might influence the plugin going forward.

If I were starting over, I'd just use PHP 5.6 and composer and make it Keycloak specific, because those are pretty much my own project parameters. In practice, Keycloak-specificity isn't that big a deal: apart from outbound endpoints, there are only a couple of inbound endpoints (k_logout and k_push_not_before) that work in conjunction with an extra parameter in the auth code request. So if somebody else were to handle making it work when Keycloak isn't the IdP, that could probably work.

The big risk I'm dealing with right now is the possibility that Wordpress' own auth machinery (let alone Woocommerce or any other plugins I'm working with) might not be designed to handle the sort of session flow I'm aiming at. I haven't got my head wrapped around how WP's cookies work, exactly, but I'm getting there.

Anyway, I'm willing to explore some possibilities here, but being realistic, my time constraints might mean I end up needing to implement a quick and dirty, PHP 5.6/composer/Keycloak version that's not terribly backward compatible or supporting of all existing workflows. (For example, I don't care about supporting WP-only users and am fine with requiring all logins to be via the plugin, always redirecting to origin page, using wp-json endpoints in place of the current admin redirect URI, leaving out privacy reinforcement, etc. Those other flows could possibly be forward-ported later, if someone (maybe me) has time.)

[daggerhart]

I'm about to be out of touch for the holidays so just wanted to reply real quick with some thoughts.

Everything you mentioned wanted the system to do makes sense to me, but I don't think the refactoring task is quite as big of a problem as "rewrite the entire plugin". Most of the work that needs to be done (as I see it) is in the client wrapper class, which needs a refactor anyways as it has become a bit of a "do everything" object, and then accompanying admin UI.

This evening I'll create a new issue with the idea's you've mentioned and start working on a 4.x branch. I'm happy to focus on the modularity, admin UI, and backwards compatibility aspects. If you want to take on your keycloak use-case a priority for the new branch, that'd be great! If not, no worries at all.

[drzraf]

I personally quite annoyed by the 3600-seconds disconnection. How to move forward on this topic?

[drzraf]

I think I finally solved the Google refresh token issue: Google only issues a refresh_token if access_type=offline has been passed to the auth URL.

        add_filter( 'openid-connect-generic-auth-url', function( string $url ) {
            return $url . '&access_type=offline';
            // return $url . '&access_type=offline&prompt=consent'; // prompt to "reauthorize" for an offline token? 
        });

Without this, Google OpenID always lead to disconnection after 1h

[timnolte]

@drzraf this has me thinking about digging into the OIDC specs to determine if that request parameter is a standard or Google specific. If it's in the spec I think it could be something we could add to the plugin in terms of "Authenticate users offline". Basically giving admins the option of not allowing the IDP to dictate the logged in session time. In some cases it is company policy for an application to honor the IDP session limits.

[drzraf]

access_type=offline is non standard. The standard mandate the use of the offline scope for this to work but Google is not following it (see the dozens of reactions at https://github.com/googleapis/google-api-python-client/issues/213)

Storing a refresh_token is sensitive, so that could indeed be an admin-only option (or at least, a hook). Digging further inside token refreshing with Google, I was beaten by https://github.com/oidc-wp/openid-connect-generic/issues/404

[timnolte]

@drzraf hmm, OK, yeah if that URL parameter isn't in the standard then it's not something that I would to specially code to support. I was originally thinking after reading your fix comment that I would want to be doing a check for the offline scope, as I would want that to be a part of an offline feature.