Confusing app_api_validate_request event behaviour

[mattrogowski]

Affected version

2.2.13

I'm not sure if this is a bug or I'm misunderstanding how it's intended to work, but I feel it's the former.

Our use case was we need to authenticate the user being used with a super user key via a custom header, as we're using an external user ID stored in the user table to find the XF user. So instead of XF-Api-User we have XF-Api-Custom-User, and we find the user by a different column.

XF\Api\App::validateUserFromApiHeader() fires the app_api_validate_request event, and within this you set $result to be the user you've validated as. validateUserFromApiHeader then returns that if it is not null, and execution continues in start() to set the visitor based on the user returned. If you now make an API call to /api/me, it finds the correct user you returned and outputs the API response. On first glance it looks like it's all working.

The problem is that validateUserFromApiHeader also calls \XF::setApiKey($apiKey);, and updates the last use of the API key. In fact it actually validates the entire API key. However, this won't get run if you return your own user from the app_api_validate_request event. So while a call to /api/me works as the user has been set, no other calls work as the API key hasn't actually been set, rendering it largely pointless.

To me this is incorrect, as validateUserFromApiHeader seems to be doing too much. It's named "validate user", but it's actually "validate user, validate API key, and set API key", which seems counterintuitive to what the event within it does (returns a user, not sets your own API key). If you allow for a user to be returned from app_api_validate_requestwhich then short-circuits the entire rest of the function, then it should still actually set the API key somewhere else, because the event is only returning a user, not setting a completely custom key. In other words, I may want to find the user myself, but I still want all the scopes the API key has.

The current solutions are either:

• Return a user from app_api_validate_request by setting $result which short-circuits validateUserFromApiHeader(), meaning you need to manually call \XF::setApiKey($apiKey); and update the last used date (as well as probably validate the API key in general)
• Validate the user in app_api_validate_request and throw an API error if it doesn't exist, but don't set $result - now it will continue to validate and set the API key, and initially set the user to be a guest, but you need to re-set the correct user in the app_api_start_end event. The reason I can't do everything in this event is that if I need to throw a custom error for the user not existing, getApiErrorResponse is a protected method, which is why I just set an error in app_api_validate_request and let the core code throw the error where it currently does
• Extend XF\Http\Request::getApiUser() to change how this value is retrieved. This feels more hacky than using events, and also means I can't throw a custom API error if the user isn't found

It seems as if the intention of the existing functionally is that you either set your own user OR use an API key, I just can't see any utility in that because it's the API, all the endpoints require scopes that we don't have if we're only setting a user because we have no API key, unless you set your own key separately even though all the logic for that is right there. So instead of any of these methods which all feel clumsy to me, I think being able to override the user should be separate from the rest of the API key validation logic, or the event needs to be moved lower down and it'll run the entire function but just let you inject your own user. It's confusing that the purpose of app_api_validate_request is to find a user, but it skips the entire rest of the logic to set the API key that isn't user-specific if you return one and leaves you not able to make any API calls.