Page MenuHomePhabricator

API token handling needs to be less special-cased and hard-coded
Closed, ResolvedPublic

Description

If 'token' is a magic parameter that ApiMain understands, ApiBase should add it to getAllowedParams() and getParamDescription()


Version: unspecified
Severity: enhancement

Details

Reference
bz45199

Event Timeline

bzimport raised the priority of this task from to Low.Nov 22 2014, 1:41 AM
bzimport set Reference to bz45199.

We really need to overhaul how tokens work in general. It should be simpler for a module to add a token requirement, and all tokens should be gettable from action=tokens.

Brainstorming:

  • needsToken() returning true should be all that's needed for a module to add token protection. Errors, help, and paraminfo should be added automatically based on that.

Right now, it seems a module has to return true for needsToken() and non-false for getTokenSalt() and include a 'token' parameter in getAllowedParams() and either add itself somewhere in the middle of ApiTokens or hook ApiTokensGetTokenTypes. And maybe also add itself to ApiQueryInfo or hook APIQueryInfoTokens, and maybe ApiQueryRevisions or hook APIQueryRevisionsTokens, and maybe ApiQueryUsers or hook APIQueryUsersTokens, and maybe ApiQueryRecentChanges or hook APIQueryRecentChangesTokens. And some modules may just be doing it on their own.

  • Each module should have a method getToken() to get the token. Default would just return $wgUser->getEditToken( $this->getTokenSalt() ), at least for now.

Right now, it's using static functions all over the place to get the tokens. Which is nice in that it means classes don't have to be instantiated, but makes it hard to handle the next bullet.

  • Some modules need query parameters for salt. Somehow the module should be able to return the names of the parameters (as returned by getAllowedParams() and getParamDescription()) that the module needs to determine the token. Or maybe just add a flag to the data returned by getAllowedParams() to indicate "needed for tokens".

Right now, not really handled at all. It looks like the rollback token you can only get through prop=revisions, and the userrights token you can only get through list=users, and if there's anything in an extension it probably either implements its own "gettoken"-style parameter (see bug 35993) or has its own query module which is the only way to get its token (like rollback or userrights).

  • action=tokens should take module names, like action=help now does. "type" would be deprecated but kept for backwards compatibility. Its help would probably have to instantiate every API module to be able to call needsToken() on it and to check whether it needs extra parameters, unfortunately.

This also solves the problem where you don't necessarily know which "type" is needed for any particular module, although that could also be solved with better parameter documentation.

  • ApiTokens should provide a method that can be called by ApiQueryInfo, ApiQueryRevisions, etc to handle their special token-getting parameters. There would have to be some way to pass data through to getToken() so it could use the right page/revision/user/etc for each result instead of trying to grab it from the query string.

The annoying part is making sure we don't break any of the ways a module does tokens now.

Under ideal circumstances I'd recommend this:

  • drop all uses of token salt -- use the same token for all things in the session
  • return the token in the login response along with the session key
  • have a single method for fetching the token (if using saved login cookies, for instance)

This should help simplify things. :)

(In reply to comment #2)

Under ideal circumstances I'd recommend this:

  • drop all uses of token salt -- use the same token for all things in the

session

  • return the token in the login response along with the session key
  • have a single method for fetching the token (if using saved login cookies,

for instance)

This should help simplify things. :)

I'd agree with all except removing the salt. I'd prefer that the editing token not also work for creating accounts and deleting articles, but unfortunately I don't have too much of a reason because the reasons for giving per-request tokens in the browser interface don't apply to the API.