On 1/4/08, Brion Vibber brion@wikimedia.org wrote:

In general I'm against duplicate code in the API -- that's absolutely the wrong way forward, and doubly so when doing any sort of 'write' or security-related operation.

Instead, existing mixed code should be refactored into clean back- and front-end sections.

When I did the work on Userrights, I did my best to ensure that the rights-changing code was as modular as possible (it's all abstracted into backend subroutines, which don't make any reference to the form itself). These, therefore, may be very easily moved out into wherever they should be, or you could even instantiate the Userrightsform class and call the methods on that (after making them public).

On 1/4/08, Roan Kattouw roan.kattouw@home.nl wrote:

That's exactly what I did. What Brion couldn't really know is that the code in ApiChangeRights duplicated code only added to Userrights *later*. Anyway, I've re-added ApiChangeRights, which now handles disallowed groups more gracefully.

I think the sort of code duplication Brion is referring to is things like this. ApiChangeRights.php lines 51 to 59:

if(is_null($params['user']))

$this->dieUsage('The user parameter must be set', 'nouser');

$uName = User::getCanonicalName($params['user']);

$u = User::newFromName($uName);

if(!$u)

$this->dieUsage("Invalid username ``{$params['user']}''", 'invaliduser');

if($u->getId() == 0) // Anon or non-existent

$this->dieUsage("User ``{$params['user']}'' doesn't exist", 'nosuchuser');

This more or less entirely duplicates the content of UserrightsPage::fetchUser, and poorly. It doesn't support foreign users, at the very least. Instead it needs to be replaced with some call like $ur->fetchUser( $username );, followed by an error message if that's null. Similarly, lines 80 to 85 in ApiChangeRights.php are probably unnecessary:

if(empty($params['addto']) && empty($params['rmfrom']))

$this->dieUsage('At least one of the addto and rmfrom parameters must be set', 'noaddrm');

if(is_null($params['token']))

$this->dieUsage('The token parameter must be set', 'notoken');

if(!$wgUser->matchEditToken($params['token'], $uName))

$this->dieUsage('Invalid token', 'badtoken');

That should all be handled by the backend logic, which should just return whether it's successful or not. All of the saving logic, in particular, should be identical between the two. The API's entire processing routine should be something along the lines of

$ur = new UserrightsPage();

$dbw = wfGetDb(DB_MASTER);

$dbw->begin();

$ur->saveUserGroups($params['user'], $params['rmfrom'], $params['addto'], $params['reason']);

$dbw->commit();

or something not too different from that. The rest should all be display logic.

On 1/4/08, Roan Kattouw roan.kattouw@home.nl wrote:

There's the problem: I can't just go ahead and tell the user the request failed, I'd also like to tell them *why*. A simple return null isn't very useful. Also note that UserrightsPage's function for this task (saveUserGroups) doesn't return anything: it can't fail except through some fatal DB error or other disaster, in which case it'll never return anyway. Now I could go ahead and introduce more return value constants for bad tokens, or I could just check the token in ApiChangeRights, which is much simpler.

Duplicating logic is less robust, and leads to more bugs. The correct way to do it, which I believe is used elsewhere in the core code now to support the API, is to have error return codes of some kind, not to just check for all the errors in the display logic.

Brion Vibber schreef:

It's a bit cleaner in API terms, but I'm a bit leery of the way the functions were split up and errors returned with numerically-indexed arrays (plus I'm still very leery of having a change groups action in the API at all) so for now I've taken it back out.

Could you be more specific as to how the implementation could be changed to be of acceptable quality? Also, why don't you want changerights in the API? We have move, rollback and all the other good stuff already.

Roan Kattouw (Catrope)

Max Semenik schreef:

All these things could be useful for bots, but why could a bot possibly need to change user rights?

True. With the autopromote system VasilievVV added recently, there's much use for it any more.

Roan Kattouw (Catrope)

Roan Kattouw wrote:

Brion Vibber schreef:

...

It's a bit cleaner in API terms, but I'm a bit leery of the way the functions were split up and errors returned with numerically-indexed arrays (plus I'm still very leery of having a change groups action in the API at all) so for now I've taken it back out.

Could you be more specific as to how the implementation could be changed to be of acceptable quality? Also, why don't you want changerights in the API?

The more privileged operations are in an undermaintained secondary interface, the more likely we are to have security problems. As such I should warn that I currently would not accept a ChangeRights api module at all, no matter how it's implemented.

In theory though, API and UI modules should *both* make clean calls to backend classes. An ideal API or UI module should never touch the database, for instance, nor check permissions.

-- brion vibber (brion @ wikimedia.org)