For feature-parity with D7, we need a UI for batch-creation of CAS users, located at /admin/people/cas/create

CommentFileSizeAuthor
#51 d8_addusers_ui-2724849-51.patch9.88 KBbkosborne
#49 d8_addusers_ui-2724849-49.patch9.93 KBbkosborne
#47 d8_addusers_ui-2724849-47.patch9.82 KBbkosborne
#40 d8_addusers_ui-2724849-40.patch7.43 KBjoegraduate
#40 interdiff-40.txt478 bytesjoegraduate
#39 d8_addusers_ui-2724849-39.patch7.49 KBjoegraduate
#39 interdiff-39.txt1.53 KBjoegraduate
#38 d8_addusers_ui-2724849-38.patch7.45 KBjoegraduate
#38 interdiff-38.txt7.51 KBjoegraduate
#36 d8_addusers_ui-2724849-36.patch6.78 KBjoegraduate
#36 interdiff-36.txt6.06 KBjoegraduate
#21 d8_addusers_ui-2724849-19.patch3.46 KBShabbir
#17 interdiff.txt5.46 KBShabbir
#17 d8_addusers_ui-2724849-17.patch3.46 KBShabbir
#15 d8_addusers_ui-2724849-15.patch3.09 KBShawn DeArmond
#10 d8_addusers_ui-2724849-10.patch2.72 KBShawn DeArmond
#9 d8_addusers_ui-2724849-9.patch2.25 KBShawn DeArmond
#6 d8_addusers_ui-2724849-6.patch405 bytesShawn DeArmond
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mhagedon created an issue. See original summary.

joegraduate’s picture

Title: How do we load users? » How do we add CAS users (D8)?
Related issues: +#2623784: Regression: Track which users are allowed to authenticate with CAS
joegraduate’s picture

Title: How do we add CAS users (D8)? » Create UI for adding CAS users (D8)?
Issue summary: View changes
Parent issue: » #2623784: Regression: Track which users are allowed to authenticate with CAS
Related issues: -#2623784: Regression: Track which users are allowed to authenticate with CAS
Shawn DeArmond’s picture

Assigned: Unassigned » Shawn DeArmond
Shawn DeArmond’s picture

Title: Create UI for adding CAS users (D8)? » Create UI for bulk-adding CAS users

Making this issue specific to re-creating the /admin/people/cas/create page in D8.

Shawn DeArmond’s picture

Issue summary: View changes
Status: Active » Needs work
Issue tags: +neworleans2016
FileSize
405 bytes

This patch provides the form and the action link. The form doesn't DO anything, but we're waiting on #2623784: Regression: Track which users are allowed to authenticate with CAS before we figure out the submit methods.

Shawn DeArmond’s picture

Assigned: Shawn DeArmond » Unassigned
bkosborne’s picture

Thanks guys. Looks like the form never made it in your patch Shawn =/

We can revisit this once the new parent issue is implemented.

Shawn DeArmond’s picture

Shawn DeArmond’s picture

Added another field in the form to add roles to the new users.

bkosborne’s picture

Status: Postponed » Needs work

This is ready to be worked on now that externalauth was added.

bkosborne’s picture

I'm not marking this as a requirement to get the D8 beta released. I see this has a nice-to-have at the moment.

metzlerd’s picture

Status: Needs work » Needs review

Should be marked as needs review unless I'm missing something.

Shawn DeArmond’s picture

Assigned: Unassigned » Shawn DeArmond
Status: Needs review » Needs work

The form is there, but it doesn't DO anything. It still needs work.

And I do think it's pretty important. It's the primary interface that my site managers tend to use to create new users on the site.

It's my itch to scratch, though, so I'm working on it now.

Shawn DeArmond’s picture

Form submits and users are created.

Probably should use the batch api like the d7 version.

Also needs tests.

Shabbir’s picture

@Shawn, I applied your #15 patch to the latest git clone and found two issues:

  1. In the bulk add cas username form, Entering multiple usernames on separate lines does not create a new entry in the authmap table against each line, instead all usernames are comnbined and created as a single entry.
  2. If a cas username exist and you try to add it again through this form, then it gives The website encountered an unexpected error. Please try again later. error. Instead it should give a soft error with a message through $form_state->setErrorByName.

I have a suggestion, how about if we also provide a CSV upload functionality as well, this will help reduce the task to again enter in the system and instead simply upload the file which will have the list of usernames.

Since the issue is assigned to you, I can also provide a patch for the same If you dont mind.

Just a suggestion!

Shabbir’s picture

I am adding a patch and interdiff which takes care of the two points stated by me in my previous comment. Hope it helps!

Nitesh Pawar’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 17: d8_addusers_ui-2724849-17.patch, failed testing.

The last submitted patch, 17: d8_addusers_ui-2724849-17.patch, failed testing.

Shabbir’s picture

Sorry my bad, there was some issue in the patch which didn't allow to create the files. Here's the new patch!

bkosborne’s picture

Status: Needs review » Needs work

Does anyone have thoughts about somehow supporting the use case of Drupal usernames being different from CAS usernames? This patch assumes that the two are the same, which is not such a bad thing for this bulk add task, but if we keep it like that, we should indicate as such in the description on this page.

  1. +++ b/cas.links.action.yml
    @@ -0,0 +1,6 @@
    +  route_name: cas.UsersAdd
    

    let's keep this standardized and use cas.users_add

  2. +++ b/cas.links.action.yml
    @@ -0,0 +1,6 @@
    \ No newline at end of file
    

    needs newline

  3. +++ b/cas.routing.yml
    @@ -48,3 +48,10 @@ cas.logout:
    +cas.UsersAdd:
    

    same as above

  4. +++ b/cas.routing.yml
    @@ -48,3 +48,10 @@ cas.logout:
    +  path: '/admin/people/cas/create'
    

    maybe use "bulk_create" for the path instead

  5. +++ b/src/Form/CasUsersAdd.php
    @@ -0,0 +1,91 @@
    +    return 'cas_users_add';
    

    cas_users_bulk_add

  6. +++ b/src/Form/CasUsersAdd.php
    @@ -0,0 +1,91 @@
    +    if (user_load_by_name('cas_'. $account)) {
    

    This only checks if a local Drupal account exists with that username, but there's also the case where a Drupal account has a different username but has that CAS username associated with it. We need to check that as well.

  7. +++ b/src/Form/CasUsersAdd.php
    @@ -0,0 +1,91 @@
    +      drupal_set_message($message);
    

    this messaging should not go in the validate method.

  8. +++ b/src/Form/CasUsersAdd.php
    @@ -0,0 +1,91 @@
    +    $casUserManager = \Drupal::service('cas.user_manager');
    

    Form classes can use dependency injection, we should avoid using the global service container.

Shawn DeArmond’s picture

No! Drupal Usernames should NOT be strongly tied to CAS usernames. That's the whole point of the externalauth module. And the whole point of the separate cas users table in D7.

When a user logs in with CAS, they should be able to go to their user/%/edit page and change their username to whatever they want.

Nobody wants Drupal's "authored by" line to say: "Authored by sdearm on 9/27/16 at 11:00am". And yes, I know there are other ways around it through RealName module and whatnot, but we have the capacity to keep them abstracted, and I strongly recommend we do so.

However, the ExternalAuth module doesn't have the best error handling when it comes across duplicates, so we might have to work around that. Or get it fixed in externalauth.

Shabbir’s picture

Since it is unlikely for externalauth to remove the "provider_" from the username added to the db, How about when a user creates a cas user through bulk create form, the name entered will be the drupal and cas-username. We will trim the "cas_" from drupal username. This will also check if the user created through bulk form already exists or not. We can also add condition to check if the same cas-username also exists or not?

bkosborne’s picture

We are already removing the cas_ prefix from Drupal usernames.

Shawn, I agree that Drupal usernames and CAS usernames should not always be equal, I was just saying that the way this patch is written, they WILL be equal. If we just accept a username for each user on each line, we have to assume that the username entered is both the desired Drupal username and the CAS username to associate with that user.

An alternative is to allow admins to enter both the Drupal username and CAS username for each user in this form, perhaps in a CSV format.

We also have to consider other modules that may want to alter the Drupal username during registration. Do we fire the CAS event that allows other modules to both alter the Drupal username and alter the user entity before it's saved, perhaps to assign extra roles / attributes? If so, we have to keep in mind that there will be no CAS attributes available which other modules may expect to have when listening to these events.

metzlerd’s picture

We also have to consider other modules that may want to alter the Drupal username during registration. Do we fire the CAS event that allows other modules to both alter the Drupal username and alter the user entity before it's saved, perhaps to assign extra roles / attributes? If so, we have to keep in mind that there will be no CAS attributes available which other modules may expect to have when listening to these events.

I could be wrong, but my initial inclination not to fire it. It's the altered cas username that shows up in the user interfaces (e.g. user edit form), so it would be confusing for a site administrator to have to "know" how the other modules might be altering that.

We probably want to auto-assign the roles that are in the settings. We do need to make sure randomized passwords get set here and that the email address suffix gets attached, because technically these are required to "create a drupal account". I haven't yet been able to tell if these issues are being addressed in the code, but will try and do so when I have time.

Shawn DeArmond’s picture

I'm generally happy with the way this worked in Drupal 7:

  1. Fill out box with lots of CAS names. Submit.
  2. Lots of new users are created, each with the CAS name associated with the user.
  3. And the Drupal Username is the same as the CAS name.
  4. If CAS Attributes was enabled, it would fire and data would autofill from wherever (LDAP, Attributes, whatever).

Basically, the same as if the user logged in and the account was auto-created.

bkosborne’s picture

Shawn, there's one major difference, in that CAS attributes would actually not be available during this type of mass-registration. CAS attributes are obtained from the CAS server during the authentication process with the server. When we're mass registering users, we're not talking to the CAS server at all. This is how it works in D7 anyway.

I'm OK implementing the solution the same way we have in D7 though.

Shawn DeArmond’s picture

That's interesting. In our experience using the CAS Attributes module in D7, the attributes DO apply when bulk-adding CAS users, though we get the attributes from the LDAP module rather than our CAS Server. So you might be right, but in D7 anyway, CAS Attributes at least TRIES to apply the data, if it can get it. In the case of LDAP, it can get it.

devil2005’s picture

Hi,

I would like to test this but not in dev version of the module :(
This feature is ok ?

Thanks a lot

devil2005’s picture

Hello,

Could you push the feature to dev version ?

Thanks a lot

bkosborne’s picture

Not his can't be committed yet, there's some feedback I had on the patch that wasn't addressed. Maybe you can create a new version of the patch (with interdiff) that addresses the feedback from #22 if you feel comfortable doing so.

Otherwise, I'll try and clean it up myself when I get free time.

bkosborne’s picture

Category: Task » Feature request
emerham’s picture

Also, wouldn't this be better to use batch operations rather than a foreach loop when adding users? We have a custom module for this job that also allows the user to assign a role to the batch of users. It also does assume that the Drupal username would be the CAS username but we don't have a hard set on the Drupal username being the same and lookup the users by cas name and see if they exist. some like this:

existing_account = $current_account->getUidForCasUsername($account);
sher1’s picture

I used the latest patch and it created duplicate usernames, since there were some that already existed. Just so people know. It worked in that it bulk created the users but didn't check for dupes. Saved me a lot of time though.

joegraduate’s picture

Status: Needs work » Needs review
FileSize
6.06 KB
6.78 KB

The attached patch is a re-roll of #21 based on the current 8.x-1.x branch that also incorporates @bkosborne's feedback from #22 and introduces batch API processing for the actual user account creation and role provisioning like suggested by @emerham in #34.

The batch processing code added is largely based on the batch functions found in cas.batch.inc in the 7.x-1.x branch.

The bulk user & role creation functionality seem to be working for me in my local testing.

It's worth noting that it'd probably be necessary to dispatch the CasPreRegisterEvent inCasUserManager::register() (or something) in order to support the CAS attributes functionality described by @Shawn DeArmond (like 7.x-1.x does). Perhaps that could be a follow-up improvement?

bkosborne’s picture

Thanks for working on this! I'll review this week.

Yea the tricky part is dealing with the event. Fact is that the attributes from CAS will not be available during these registrations, because the users aren't authenticating with CAS. But I guess it makes sense to fire the event anyway in case other modules don't use the attributes anyway.

joegraduate’s picture

Attached is an updated version of the patch in #36.

Updates include:

  • Addressed coding standards issues identified in test bot results for #36.
  • Added logging for CAS user create operations.
  • Improved messages generated by batch functions.
joegraduate’s picture

More coding standards fixes.

joegraduate’s picture

Removed unused variable.

Sorry for all the extra revisions. I'm done working on this for now.

bkosborne’s picture

Assigned: Shawn DeArmond » Unassigned
Status: Needs review » Needs work

Sorry for the late review. I lost track of this.

  1. +++ b/cas.links.action.yml
    @@ -0,0 +1,6 @@
    +cas.users_add:
    +  title: 'Add CAS User(s)'
    +  route_name: cas.users_add
    +  appears_on:
    +    - entity.user.collection
    +  weight: 1
    

    Let's call this "Bulk Add CAS Users". That way it's clear that this is not THE only way to add CAS users, but is instead a method to bulk add them.

  2. +++ b/cas.routing.yml
    @@ -48,3 +48,10 @@ cas.logout:
    +cas.users_add:
    +  path: '/admin/people/cas/bulk_create'
    +  defaults:
    +    _form: '\Drupal\cas\Form\CasUsersAdd'
    +    _title: 'Add CAS Users'
    +  requirements:
    +    _permission: 'administer users'
    

    Likewise, the title here (and name of the form class).

  3. +++ b/src/CasBatch.php
    @@ -0,0 +1,139 @@
    +/**
    + * Class CasBatch.
    + *
    + * Provides CAS batch functions.
    + */
    +class CasBatch {
    

    This should be defined a a service so its dependencies can be passed into it. You can see how this is done via the services.yml file in the module and other service classes.

    Basically avoid using the \Drupal:: superglobal throughout the class. It makes unit testing easier.

  4. +++ b/src/CasBatch.php
    @@ -0,0 +1,139 @@
    +    if ($existing_uid = $casUserManager->getUidForCasUsername($cas_name)) {
    +      $existing_account = User::load($existing_uid);
    +      $url = $existing_account->toUrl()->toString();
    

    Instead of loading the user this way, inject the ExternalAuth service class and use that to load the user by username. Refer to the "login" and "register" methods in the CasUserManager service for more info.

  5. +++ b/src/CasBatch.php
    @@ -0,0 +1,139 @@
    +
    +    $account = $casUserManager->register($cas_name);
    +
    

    This should be wrapped in a try/catch, because the method may throw a CasLoginException

  6. +++ b/src/CasBatch.php
    @@ -0,0 +1,139 @@
    +    if (!$account) {
    +      $context['results']['messages']['error'][] = $cas_name;
    +      drupal_set_message(t(
    +        'Error occurred during account creation of CAS username %name.',
    +        ['%name' => $cas_name]
    +      ), 'error');
    +    }
    

    For all these sections where you're storing the CAS username in the $context for later, I'd suggest not using these drupal_set_message calls as well. In the userAddFinish method, you're already reporting the total number of failures/successes/etc. Just print out the list of names there

  7. +++ b/src/Form/CasUsersAdd.php
    @@ -0,0 +1,88 @@
    +    $roles = array_map(['\Drupal\Component\Utility\Html', 'escape'], user_role_names(TRUE));
    +    $form['account']['roles'] = [
    +      '#type' => 'checkboxes',
    +      '#title' => $this->t('Role(s)'),
    +      '#options' => $roles,
    +      '#description' => $this->t('These roles will be added to each new user.'),
    +      '#weight' => -9,
    +    ];
    

    I think that it should be indicated somehow which roles will be auto assigned from the "auto register users" settings section of the module. I could see site admins being confused about that. If they have it setup to auto assign the "foobar" role to users that register automatically via CAS, that role will ALSO be assigned during this process. Maybe we should update the main CAS settings form to indicate that as well...

This should ultimately have some tests too. I can look into doing that when I get time unless you want to take a crack at it. I want to test some possible cases where you bulk add users and the user already exists, and interesting cases where there are existing users that have a Drupal username that exists but their CAS username is different, and how that may interact with this batch functionality.

joegraduate’s picture

Assigned: Unassigned » joegraduate

Thanks for the feedback @bkosborne. I'll take a crack at making all the requested changes.

kclarkson’s picture

Is the last patch okay to use? Meaning it won't blow up my site ;)

fran seva’s picture

Hi @bkosborne - I'm working in this ticket because I want use this feature and reviewing your last review, where you propose wrap $casUserManager->register($cas_name) with try/catch, I see register() method is throwing CasLoginException when something goes wrong. Taking that into account, does it make sense wrap the register() call with try/catch?

 public function register($authname, array $property_values = []) {
    try {
      $property_values['pass'] = $this->randomPassword();
      $user = $this->externalAuth->register($authname, $this->provider, $property_values);
    }
    catch (ExternalAuthRegisterException $e) {
      throw new CasLoginException($e->getMessage());
    }
    return $user;
  }
bkosborne’s picture

Yes, the call to ->register in the patch should be wrapped in a try/catch to catch the possible CasLoginException that would be thrown. It should then add the account to the list of accounts that couldn't be registered

bkosborne’s picture

Assigned: joegraduate » bkosborne
Issue tags: -neworleans2016

I'm going to fix up that last patch and get this committed.

bkosborne’s picture

Status: Needs work » Needs review
FileSize
9.82 KB

Here we go. Addressed most of my original feedback in #41. Tests should pass, and I'll commit.

Status: Needs review » Needs work

The last submitted patch, 47: d8_addusers_ui-2724849-47.patch, failed testing. View results

bkosborne’s picture

Status: Needs work » Needs review
FileSize
9.93 KB

Oops, left a debug line in.

Status: Needs review » Needs work

The last submitted patch, 49: d8_addusers_ui-2724849-49.patch, failed testing. View results

bkosborne’s picture

Status: Needs work » Needs review
FileSize
9.88 KB

let's try that again

  • bkosborne committed 873fd79 on 8.x-1.x
    Issue #2724849 by joegraduate, Shawn DeArmond, bkosborne, Shabbir,...
bkosborne’s picture

Status: Needs review » Fixed

Thank you, everyone!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.