User Processor - add mapping target for user roles

Hi Douug!

Thank you very much for this patch! I've used it and works perfect, but I would like to ask you a question:
In my csv I import the name, the mail, the password and the role. It works ok, but if change the role (sometimes I need to chage the role of a user; for example, I create a user with the role 'member_fun_club', and in another import I change his role for 'no_member_fun_club'), and now, he has two roles.
Exist a way to remove o replace the old rol with the new?

If this has no solution, is there a way to remove a csv file of users using Feeds? I can't find a way to do this... :-(

Thanks in advance and thanks again for the patch!

Reroll of #2 for the latest version of dev. #2 didn't apply for me.

#4 worked great for me

#4 worked for me too. Thank you.

This patch is for the current dev version.

worked great for me. Thank you.

We can't hardcode the input format to be a comma separated list. It should handle multiple values like everything else.

will this patched be committed in the module?

I think it is great to include.

We can't hardcode the input format to be a comma separated list. It should handle multiple values like everything else.

What do you mean exactly? Why it is not okay to force users to seperate multiple roles with a comma? How is this solved in other cases?

Feeds has a strict separation between the parser and processor. The processor should not have any idea about the format of the feed. There are lots of instances where multi-valued targets are supported. It's up to the parser to parse things correctly.

That said, Feeds Tamper can handle the case where you need to separate a comma separated list.

* Removed the explode on "," in favor of using Feeds tamper.
* Added configuration setting for the processor to determine if new user roles should be created.

Closed #1257874: User Role Map tamper as a duplicate.

@grndlvl What do you think about checking if roles_list is an array or a string? That way if you are importing a CSV with just one role it won't break. Or just cast the string as an array which is the quick way.

Here's an interdiff snippet that you could apply against #13. This works well for me.
curl https://www.drupal.org/files/issues/1376774-interdiff.txt | patch -p1

Updated patch with modifications from #16 for use with drush make in my projects :)

Thanks @PascalAnimateur it's been a while thanks the combo:)

@MegaChriz or @grndlvl or @twistor thoughts?

Sounds like a good idea to me.

The patch in #17 works pretty good. I tested with the "Create user roles" option turned on and with that option turned off.

I noticed one issue during the test and I see a possible security issue. Furthermore, this needs automated tests. I agree with joelpittet that casting "roles_list" to an array when it is a string is very useful. + 1 for keeping that.

Issue: empty role

A role without a name is created when importing an user without a role specified in the source (when mapping to "User roles", with the "Create user roles" turned on).

Potential security issue

I see a potential security issue with mapping to role: users who can control the source to import, could supply the role of an administrator on the website and gain more privileges this way. Though this case is probably very rare, I think there should be an option to limit the roles to accept from the source or at least a warning message when mapping to role. What do you think?

Target configuration vs processor option

It would be nice if the setting "Create user roles" was implemented as target configuration, just as is the case when mapping to a taxonomy term field where you have the option to create new terms. I also experienced that this is harder to do when the mapper - in this case "roles_list" - is defined in the processor class (I tried to implement target configuration for passwords before: #1611554: Support for encrypted passwords).

Automated tests

I think the following cases need to be covered by the tests:

• "Create user roles" option turned off:
  • Single existing role: role should be assigned
  • Single non-existing role: role should NOT be created (nor assigned)
  • Multiple existing roles: roles should be assigned
• "Create user roles" option turned on:
  • Single existing role: role should be assigned
  • Single non-existing role: role should be created
  • Multiple existing roles: roles should be assigned
  • Multiple non-existing roles: roles should be created
  • Empty value: no roles should be created nor assigned

Re Potential security issue:
This is a relly good point. We probably should implement a configurable black/whitelist of existing roles as someone might actually have this thing open to the public. Though, I feel that in and of itself may be a security issue, but I could see it being a possibly acceptable use for integrations.

Re Target configuration vs processor option:
It certainly makes more sense on the target than the processor, I can't remember why, but I believe it was placed here strictly out of convience. (read harder to do)

Re #20 Limiting role options sounds like a great idea!

I've been using #17 and so far so good:)

Any chance the items in #20 could be a feature/task for a followup issue? Or do they need to be addressed here?

The items in #20 should be addressed here, in my opinion.

#17 Works for me too

@MegaChriz Could this be be commited without tests? I think this is a basic feature. Maybe a new issue can be created for tests regarding this issue.

@rcodina
I would prefer a patch with tests in this issue since I'm afraid a follow-up issue gets lost in this queue. I'm willing to write the tests eventually, but right now I'm working on some other Feeds stuff first, like porting Feeds extensible parsers to Drupal 8, see #2465535: [meta] Feeds extensible parsers 8.x roadmap.
Beside the tests, the patch also has other issues, see #20.

I won't postpone this on an 8.x version, but we need to forward port this once it's done.

Closed #2452837: Cannot map User roles as a dupe.

Re-rolled patch #17 against latest 2.x-dev .. maybe I'll look into addressing the issues mentioned in #20 sometime in the future.

Here's a patch that works with 2.0-alpha9.

Let's set this to needs review so people will review it. #29

Re-rolled the patch against latest 2.x-dev

I've been using this for a while:)

Okay @joelpittet, but it still needs work, See #20 ;).
There are no automated tests included with the patch. And has the issue that empty roles can get created been resolved? And can the feature be implemented as target configuration?

Oh yeah... forgot to look at the issue tags, was just poking at patches I re-applied.

What is a "target configuration". Sorry the term doesn't mean anything to me at the moment, though sounds like something I should know.

@joelpittet, no problem! :) Just thought I should correct the status.
Good that you ask what target configuration is. Target configuration is extra options/settings you can set when mapping to a target. For example, you can set a target as unique. Or for text fields where the text processing setting is set to "Filtered text (user selects text format)", you can configure the text format to use for the field.

It would be better if the "Create user roles" setting would be implemented as an option on the target, instead of being a setting of the user processor. The setting on the processor settings page makes no sense when not mapping to user role.

@joelpittet
Am I doing good with delegating this task (to avoid contribution burnout) ;)?

@MegaChriz doing well:) Thanks for the screenshot, that is super helpful.

I was working on the tests today and I figured I might as well update the implementation to let these newly added tests pass.

The attached patch addresses all my concerns from #20.

Summary of the changes:

• The "autocreate" option is now implemented as target configuration.
• An option to limit the allowed roles is added. By default all roles are allowed. All roles that are not available during configuring are also allowed. This means that when new roles are added later, these will be automatically allowed.
• Tests have been added to test the following behaviour:
  • Mapping to role without adding new roles.
  • Mapping to role with adding new roles.
  • Mapping to role with limited allowed roles.

One point of discussion is about the allowed roles. When roles are added after configuring the Feeds importer, these roles automatically get allowed. I did it that way because else newly created roles during the import wouldn't be allowed ;).

Hi,
just tested patch from #44 and it is working for me.
My source file contains comma seperated role_ID's but I changed them to user role names to be able to use this patch.

Maybe it would be handy if it was possible to choose if you have user role ID's of user role names as input (just like a taxonomy term import setting)

Thanks a lot for the great work!

Hi,
after some testing it looks good but I think that when re-importing, the user roles are not overwritten but added.
I have an example that a user his role changed in the source XML and that he kept his old role. The new role is added.

Is there a way to remove old roles or just overwrite all values with the imported ones?

@maxplus
An option to use role ID as input + an option to revoke roles look like useful additions.

Role ID as input

The option to use role ID as input could indeed be implemented similar as the taxonomy reference mapper.

Revoking roles

Revoking roles could be implemented as an option as well. What do you think, should it only revoke roles that are part of the "allowed roles"? This would mean the following:

1. Say you have 2 users: John and Jane. John has the role "admin" and Jane has the role "editor".
2. On the role mapping the option "Revoke roles" is enabled and the allowed roles to accept from the feed are "editor", "manager" and "tester".
3. The processor is configured to update existing users.
4. The following source is imported:
   

   name	roles
   John	manager, tester
   Jane	tester

This would result in John having the roles "admin", "manager" and "tester". Jane would get the "tester" role, but will no longer have the "editor" role. The thing is here that even though "admin" is not specified by the feed (and unspecified roles should be revoked), John will keep that role because of the "allowed roles" setting. This behaviour would make sense to me.

Both features would need an automated test.

Allowed roles

@maxplus
Do you have an opinion about the fact that roles that are added after the importer is configured automatically get allowed? (See #44 for why this happens.)

@MegaChriz

Thanks for your proposal!

In my use case, the allowed roles are not so important because Drupal is the slave system in my setup.
For me, I would like the Feeds importer to be the master so in your example John would end in having only the roles manager and tester and he would loose his admin role.

When you look at a multi value taxonomy term reference field in a Feeds importer, does that use the revoke policy also?
Are old taxonomy term reference values kept or overwritten with the new imported values?

About roles being automatically added to the allowed roles, that looks OK.

@maxplus
For the taxonomy reference mapper and for all other field mappers, the values are always overwritten. The reason for that is that else it wouldn't be possible to empty the field value. Also there could be a chance of duplicate values if values were appended each time. I guess for the taxonomy reference mapper it would be doable to unduplicate the list, but that wouldn't necessary make sense for every field mapper.
For roles I was more cautious to implement it in the same way, as it has impact on the permissions that an user has. Also, the roles mapper doesn't have the problem of duplicate values. I do realize that this behaviour is not consistent compared to the other mappers. What do you think? Does this justify the exception in behaviour?

@MegaChriz
In the first place, I'm more a fan of consistent behavior. I think the fact that changing user roles has permission implications is not a problem, it is the responsibility of the webmaster to be careful with this.

But having a second thought, I have no problem with the fact that you don't touch roles that are assigned to users that are not listed in the allowed value list.
=> by keeping that extra feature, you provide both possibility's:
1- people like me that want a complete overwrite or all roles: then I just need to add all roles to the allowed values
2- people that don't want some roles not be touched: they exclude those roles

Hm. I'm still not so sure about always revoking roles not provided by the feed. In the patch I've made this optional now, where the default is that roles are revoked. So at least when using the defaults, the behaviour is consistent with the other mappers.

The patch also adds an option for searching by role name or ID. The default is to search by name. Constants for the available options are added to the FeedsUserProcessor class.

Setting the roles has moved to the method FeedsUserProcessor::rolesListSetTarget().

While I'm writing this, I realize I probably also need to include a test that ensures roles are not revoked at all when the option to revoke roles is disabled.

Fixed a small mistake in FeedsUserProcessor::rolesListSetTarget() and added a test that ensures that no roles are revoked when the option "Revoke roles" is disabled.

@MegaChriz

Thanks for your great work!

I have tried to apply your patch from #53 but getting a "unexpectedly ends in middle of line" message:

patching file plugins/FeedsUserProcessor.inc
patching file tests/feeds.test
patching file tests/feeds/users_roles.csv
patching file tests/feeds_processor_user.test
patching file tests/feeds_tests.module
patch unexpectedly ends in middle of line
Hunk #1 succeeded at 352 with fuzz 1.

I'm using the current feeds dev 7.x-2.x-dev from 2016-Mar-29 using the patch command "patch -p1 < patch.patch"

@maxplus
Hm. I tried it too, but I did not get that error. The testbot also seemed to have no problems with applying the patch. Do you perhaps applied other patches as well? Else try to redownload the dev release, redownload the patch and apply the patch again.

@MegaChriz

Sorry, my fault. I did make a mistake applying your patch. It now applies clean at the current dev version.

It is working for me, using the revoke setting: GREAT!

I have noticed that the "Extra roles" on the settings page (admin/structure/feeds/feeds_importer_name/settings/FeedsUserProcessor) are not added anymore, but that is not a problem for me. Maybe we should remove that setting?

@maxplus
Thanks for reporting back!

I'm not sure yet what to do with the "Extra roles" setting on the User processor settings. There are other issues about that it is not working properly:
#2545964: User import does not add roles from "Additional roles" when updating existing users
#1894542: User import does not replace roles with "Additional roles" when replacing existing users
In your case, does it also not work when creating new users or only when updating existing users?

I've thought a bit about the "Additional roles" setting. I think it should be kept for backwards compatibility (and thus not breaking existing importer configurations).

The attached patch makes this setting work in combination with the role target. Any roles configured for "Additional roles" will always be added for each user that is created or updated. This also fixes #2545964: User import does not add roles from "Additional roles" when updating existing users. The role addition happens after the role target has done its work. So any roles that may be revoked during the role target process, may be re-added as a result of the "Additional roles" setting.

Committed #58 with a few corrections in the code comments. Thanks all!

@MegaChriz thanks!