Simplify refresh/logout handling (to fix #49, #50, and #51) #52
Conversation
|
I really like this approach. Thanks a lot for the work you've done recently on issues. I'm going to merge this soon and up the version to 3.3. One thing I'm not sure of is the removal of the On one hand, we probably don't need it. On the other it's "nice to have" at the very least to be able to query for which users are oauth users, and could be considered a BC issue if removed (possibly break someone's custom code/process). I'm tempted to leave it in because I don't see how it hurts to have it. Any thoughts? |
|
So, while I don't personally care (don't have BC issues because I'm still in testing stages), my reasoning for getting rid of it was that it has no coherent meaning. The documentation claimed it meant the user was created by the plugin, but that's not true in the case of linking existing users. Depending on the state of your settings and the state of any sessions for a user, it could be true or false with no correlation whatsoever to whether the user was created by the plugin. I couldn't come up with any way to document the existing behavior that made any sense, in fact. The closest you might be able to get is, "if you don't allow linking existing users and you never turned that flag on while your site was active, then the flag indicates whether the user was created by the plugin". It's possible that someone is actually using that information, if they have never allowed linking existing users. However, in that case the flag is rendundant: they could look at say, So, in order to "leave it in", it's necessary to decide on its intended semantics. Does it really mean "created by the plugin", or does it mean, "has used the plugin to log in at some point"? Either is possible going forward, but only the latter meaning can actually be implemented in a way that fixes existing data. Essentially, you could add a user_meta hook to force the value to true based on the presence of the subject identity. That would then always have the "right" meaning, if the intended meaning is "this user has a linked account". That seemed like a lot of work to go to for a feature I wasn't sure anybody was using, and if they were, they probably had bugs. :) Given that the plugin isn't on wordpress.com and thus doesn't auto-update, documenting the issue and bumping the major version (i.e to 4.0) should suffice to allow someone time to switch to a more appropriate field. |
|
Great points. Thank you! |
|
Excellent; this means I can start on a fix for #54, without this PR being a moving target. |
This PR simplifies refresh token management, moving it from an encrypted cookie to a session token field on the server (addressing issue #49), and keeping track of the refresh token's expiration time so that it doesn't produce an error trying to refresh with an expired token (#51). #50 is also fixed, by deferring the token refresh (and therefore logouts) until all WP plugins are finished loading.
As a side effect, the dependency on Defuse is dropped (no more encrypted cookies means no more encryption needed), and two user metadata fields are dropped as well (the flag that was at issue for #49, and the cookie encryption key).