I am currently using the Services module for a project where an external application is responsible for creating, updating and deleting user accounts in a Drupal based intranet.

Most things are working just fine (thank you all), however there are a few points which I have doubts about:

The user.save service
Drupal's special users (uids 0 and 1) require some configurable protection against being changed. I don't want the external application to be able to change the values of these accounts.
There are other users (such as site editors or administrators) that I'd prefer the external application not change (signs of creeping paranoia?). These can be designated - and distinguished - by special roles.
The user.delete service
For the same reasons as above, I don't want the external application deleting uids 0 or 1, or any specially designated users.

From this short discussion hayrocker, one of the commiters, suggested integrating the User services module with the User Protect module. The attached patch is a result of that integration.

Limitations
The patch only takes into account the deletion and all account edits protections from User Protect. Single field protection, such as username or e-mail has not been integrated.

Testing
Testing has been carried out with and without the User Protect module enabled. When enabled, the following settings were used:

Protections defaults (admin/user/userprotect/protection_defaults)
All checkboxes unchecked
Administrator bypass (admin/user/userprotect/administrator_bypass)
As per the following snapshot (syger is the super user)
Protected roles (admin/user/userprotect/protected_roles)
As per the following snapshot (editore is a protected role, remote is the external application role)
Protected users (admin/user/userprotect)
As per the following snapshot (syger is the super user)

Explanation
As this patch is somewhat complex (and makes changes unrelated to the User Protect integration) I will try to explain each block in further detail for the committers.

@@ -32,6 +32,13 @@ function user_service_delete($uid) {
Checks for User Protect deletion protection
@@ -66,7 +73,8 @@ function user_service_get($uid) {
Minor aesthetical modification, in line with other ored tests
@@ -170,7 +178,7 @@ function user_service_save($account) {
The drupal_execute function doesn't return anything, and $ret is not used elsewhere.
@@ -180,17 +188,36 @@ function user_service_save($account) {
This is the big one. I'll go through it piece by piece.
+    // Preload the state with some of the original user object values
+    $allowed = array('name', 'mail', 'mode', 'theme', 'signature', 'status', 'timezone', 'language', 'picture', 'contact', 'roles');
+    foreach ($update as $key => $value) {
+      if (in_array($key, $allowed)) {
+        $form_state['values'][$key] = $value;
+      }
+    }
Non integration code. By adding the previously saved user fields to the $form_state the external application does not have to provide all the values, but can make a partial change, by specifying the uid and status for example.
+    // Any logged in user is by default authenticated,
+    // and leaving this role set in the user's roles array
+    // causes big problems because of a FAPI hack that controls
+    // this checkbox on the user create and edit form (and thus
+    // causes problems with drupal_execute()). Therefore we just
+    // force it to 0 here.
+    if (isset($form_state['values']['roles'][2])) {
+      $form_state['values']['roles'][2] = 0;
+    }
Disables the authenticated user role in a similar way to the code on lines 161-163.
+    drupal_execute('user_profile_form', $form_state, $update, $category);
The drupal_execute function doesn't return anything, and $ret is not used elsewhere.
Non integration code. Instead of using the data received ($account) as the third parameter, I have changed this to use the previously saved user data ($update) which will then correctly fill the form's default values, as per the menu item user/%uid/edit
@@ -210,6 +237,14 @@ function user_service_save($account) {
Checks for User Protect all account edits protection

Best regards,

John

CommentFileSizeAuthor
#9 menu_get_item.patch1.72 KBjhl.verona
#2 userprotect.patch3.83 KBjhl.verona

Comments

jhl.verona’s picture

jhl.verona commented
Status: Active » Needs review

Wrong status, sorry. Can't seem to be able to edit the original post.

jhl.verona’s picture

jhl.verona commented
StatusFileSize
new userprotect.patch3.83 KB

And the patch got lost, along with the images... Hmm.

alfthecat’s picture

alfthecat commented

A shot in the dark but maybe worth trying.... Have you considered using the path access module to restrict pages for the role being used by the external application to login to the site?

See http://drupal.org/project/path_access

Like I said, shot in the dark but if it works it is a simple and swift resolution...

jhl.verona’s picture

jhl.verona commented

Thanks for the link.

Firstly, this patch is working just fine for me. Secondly, using the XML-RPC server, there is only one URL (services/xmlrpc) for all methods of all services.

I just need to make sure that certain users can't be deleted or modified. User Protect is perfect for this because using the Web UI, all requests would pass through user/%uid/edit. It just needed a small integration patch to keep it working its magic via a user service method call.

John

marcingy’s picture

marcingy commented

Haven't reviewed the patch but I'm against this going in as it is not drupal core functionality and as such has no place in services core as a result. If we add this then my fear is where does it stop....

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented

If we are handling users improperly, like going around traditional user save procedures that User Protect hooks into, then I am happy to look at those changes. I am less inclined to look at changes that exist only to integrate with User Protect, for the reasons that marcingy outlined. There may be some places where a patch there will make more sense. Additionally, I don't want to do an API change mid-major-version. I'm unsure if anything here would modify existing functionality.

Anyways I'll try to dig into this soon, thanks.

marcingy’s picture

marcingy commented
Status: Needs review » Needs work

I have had a chance to review and the very existence of 

if (function_exists('userprotect_perm')) {

makes this patch a no go for me. Yes we can support user_protect if change simple add data passed into drupal_excute that is currently missing for user save but actually referencing that module in code is out in my opinion.

jhl.verona’s picture

jhl.verona commented
Status: Needs work » Needs review
StatusFileSize
new menu_get_item.patch1.72 KB

Well I have to agree with marcingy in #5. Via the Web UI you can delete uid == 0 (anonymous user) and uid == 1 (superuser) - which is fun to correct afterwards. Programmatically, you can delete them all, of course.

I can't say that you're handling users improperly (heyrocker #6) exactly, and the previous patch creates a connection with User protect (as marcingy points out in #7).

However, I think that the maintainer for User Protect (hunmonk) could use your own arguments as to why he shouldn't allow a patch with a connection to User Service (I have not asked, the D6 project seems to be frozen while D7 work progresses).

However #6 does raise an interesting issue, so I will actually argue that you're using access handling improperly. You're using delete and save which, using the Web UI, correspond to user/%uid/delete and user/%uid/edit, however you're ignoring the menu_router access functions and providing (only) your own. User Protect actually modifies the menu_router access functions using hook_menu_alter.

So I have prepared another patch, which actually has better functionality, because individual fields are also protected. This simply uses menu_get_item for the corresponding Web UI path.

But there is one other modification, I have still changed the drupal_execute call:
drupal_execute('user_profile_form', $form_state, $update, $category);
to use the loaded user ($update), and not the received, possibly partial user data ($account). Following the $form contents around during the WEB UI process, on a sandbox with only user and profile modules enabled, the $form['_account']['#value'] always contains the complete, unmodified, user object. See also http://api.drupal.org/api/function/user_profile_form/6

I rest my case, m'lud. (And I won't bother you again, whatever the final decision.)

John

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented

Unfortunately, the way that Services is architected, we can't rely on the menu access necessarily. API Key services often run as anonymous, which is why they need their own access control separate from those that Drupal provides. This is all changing down the road but for now it is what it is.

Another solution I would consider looking into is that we dump the user_delete() API function to delete users, which is the one thing that User Protect has no way to hook into. I was thinking we could switch this to use drupal_execute(), which in theory User Protect should be able to work with. I'm not entirely sure that's going to fly though, because of its form_alter()s. It is something that would take some experimentation. On the user_save() side, this service already uses drupal_execute() with user_profile_form and looking at the User Protect code, it appears it is already protecting that form. John have you tried User Protect + User Save Service with no extra code? I would be curious if that works or not.

jhl.verona’s picture

jhl.verona commented

API Key services often run as anonymous

Oops, didn't know abut that. Hmm, what about testing (global) $user != 0 ... as well?
Example:

  global $user;
  if ($user != 0) {
    $item = menu_get_item('user/'. $uid .'/delete');
    if (!$item || !$item['access']) {
      return FALSE;
    }
  }

I much prefer the access control approach over my previous effort, because it's independent of any other modules used. Which is why I'll keep trying to push this.

However I am aware of your (reasonable) reluctance to add 'gratuitous' code. And I agree, writing code is like having children - 5 minutes of fun, then a lifetime of maintenance.

User Protect is actually quite a small piece of code.

It protects again deletion ('deletion' flag) using access control (line 392).

It also protects via access control if you've set the 'all account edits' flag (also line 392).

John have you tried User Protect + User Save Service with no extra code? I would be curious if that works or not.

Technically no. I tested with the patch.

However, the answer is (partially) yes, because UP modifies the form for all permissions except 'all account edits' (line 186).

I tested with a user (and role) where UP was protecting a single field - the username (in the UP UI 'username' checked, 'all account edits' unchecked'). In both cases the changes sent via user.save (I changed both the username and email) were made only to the email field.

The code also disables the 'Delete' button if the 'deletion' flag is checked - but that won't stop the programmatic form submission if you set 'op' to 'Delete' without testing.

Unfortunately UP doesn't modify the form if you've set the 'all account edits' flag - it just assumes you couldn't get there because of the access control. This is correct in the Web UI - even specifying user/123/edit gives you an 'access denied' error and no form.

Also, unfortunately, it loads the user account via arg(1). However, someone has sent a patch: http://drupal.org/node/752158#comment-3008882 and that patch has now been accepted.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented
Status: Needs review » Closed (won't fix)

Ultimately I've decided not to include this functionality in Services.