WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#26706 closed enhancement (fixed)

Allow custom authentication handlers for all requests

Reported by: rmccue Owned by: nacin
Milestone: 3.9 Priority: normal
Severity: minor Version:
Component: Users Keywords: has-patch commit
Focuses: Cc:

Description

While it's possible to write custom authentication handlers with WordPress, these methods all eventually rely on using cookies to store the current user.

It should be possible to switch out the other half of this equation. For example, OAuth handlers should be able to set the current user based on query parameters, as well as checking their own nonces.

Currently, the main obstacle to this is get_currentuserinfo(), which is hardcoded to use wp_validate_auth_cookie(). To work around this, you have to check on plugins_loaded or a similar early hook; before any other code calls is_user_logged_in(), wp_get_current_user() or anything else that uses the underlying function.

I'd like to propose adding a filter to replace the existing wp_validate_auth_cookie() call, and have it return a WP_User. wp_validate_auth_cookie() can then be hooked into this by default.

(I don't have a patch for this one yet; just noting it so I don't forget.)

Attachments (3)

26706.diff (2.2 KB) - added by rmccue 3 years ago.
Convert get_currentuserinfo to use filters instead of hardcoding cookie functions
26706.2.diff (2.6 KB) - added by nacin 3 years ago.
26706.3.diff (2.5 KB) - added by nacin 3 years ago.
Alternative filter name, validate_auth_cookie.

Download all attachments as: .zip

Change History (24)

#1 follow-up: @SergeyBiryukov
4 years ago

Related: #19821

#2 @rmccue
4 years ago

Also worth mentioning that while you can't currently disable cookie generation, you *can* set your own data as needed on the set_auth_cookie/set_logged_in_cookie hooks, so it would be theoretically possible to not use the cookies altogether and still have a working system. (That is, there's no real hardcoded dependencies on cookies that I can find, apart from get_currentuserinfo())

#3 in reply to: ↑ 1 @rmccue
4 years ago

Replying to SergeyBiryukov:

Related: #19821

Similar, but custom auth handlers are probably still working in a browser environment. This is more for non-browser environments than anything else.

Not a big problem right now, but adding more APIs will make it one. The XMLRPC API calls wp_authenticate on each API call, but the JSON API has a custom filter for this purpose.

Unifying this across the whole of WP would be a bit nicer. :)

@rmccue
3 years ago

Convert get_currentuserinfo to use filters instead of hardcoding cookie functions

This ticket was mentioned in IRC in #wordpress-dev by rmccue. View the logs.


3 years ago

#6 @rmccue
3 years ago

  • Keywords has-patch added

has-patch will-travel

Working in my testing, plus solves my need nicely. Also splits out the checking of both cookies in a nicer way, IMO, by not having confusing logic. Win-win-win.

Not sure on the filter name (since get_currentuserinfo is a horrifically named function in the first place), but it's the obvious one. Comments also most welcome on the hook/function docs. :)

#7 @SergeyBiryukov
3 years ago

  • Component changed from General to Users

#8 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 3.9

#9 @nacin
3 years ago

  • Keywords commit added

This is very smart.

The only thing I don't like is the filter name, get_currentuserinfo. (Obviously, I also don't like the function name.) Let's come up with something new and get this in.

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.


3 years ago

#11 follow-up: @willnorris
3 years ago

agreed that this looks good. How about calling the filter wp_current_user ? Short, accurate, and not used elsewhere.

#12 in reply to: ↑ 11 @willnorris
3 years ago

Replying to willnorris:

agreed that this looks good. How about calling the filter wp_current_user ? Short, accurate, and not used elsewhere.

though I guess filters tend not to be prefixed with wp_ in the same way that functions do. Just current_user could work as well.

#13 @nacin
3 years ago

What about determine_current_user? It is imperative, like 'authenticate'.

We discussed "current_user_id" in IRC, but that sounds more like a filter in get_current_user_id(), not the determination of the current user. We then discussed set_current_user_id, which sounds more like it belongs in wp_set_current_user(), and indeed already has a set_current_user action.

Ultimately, the only reason why get_currentuserinfo() isn't a part of wp_get_current_user() is the issues of things being pluggable. So maybe "current_user" does actually work. It pairs nicely with set_current_user, though it doesn't run every time wp_get_current_user() is run, just on initialization. (Also, as just pointed out in IRC, it implies returning an object, not an ID.)

#14 @willnorris
3 years ago

ah, good point. I overlooked that it's filtering on the ID, not the user object. That does make 'current_user' slightly less descriptive.

Though I'm not sure that this filter only being called on initialization rather than every wp-get_current_user() call matters that much... is there really a case where the current user changes during the course of a request?

#15 @DrewAPicture
3 years ago

  • Keywords needs-docs added

The hook doc on <whatever_hook_name_you_decide_on> should describe _what_ is filterable, not what the filter is capable of, e.g. "Filter the current user." Also need a period at the end of the @param description.

#16 @jeremyfelt
3 years ago

I would go with determine_current_user.

An alternative would be something around "valid" or "cookie" only because of what we're using it for in core. Our default purpose is to filter whether the current user session is still valid by checking cookies. Maybe determine_current_user_session would work, though that pigeonholes it a bit.

@nacin
3 years ago

@nacin
3 years ago

Alternative filter name, validate_auth_cookie.

#17 @DrewAPicture
3 years ago

  • Keywords needs-docs removed

Looking at 26706.2.diff, seems like if 0 is passed to wp_set_current_user() that it will eventually return false itself.

So why not use a default of 0 on the validate_auth_cookie filter and, simply pass it to wp_set_current_user() once instead of twice with the check?

#18 @DrewAPicture
3 years ago

Oh, I see explicit false return in the old code.

#19 @nacin
3 years ago

ryan also likes determine_current_user.

#20 @nacin
3 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 27484:

Allow for custom authentication handlers for all requests.

Turn the logic used by wp_get_current_user() into a determine_current_user filter.

props rmccue.
fixes #26706.

#21 @DrewAPicture
3 years ago

In 28007:

Inline documentation fixes related to the determine_current_user filter

See #26706, #27700.

Note: See TracTickets for help on using tickets.