Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
For feature-parity with D7, we need a UI for batch-creation of CAS users, located at /admin/people/cas/create
Comment | File | Size | Author |
---|---|---|---|
#51 | d8_addusers_ui-2724849-51.patch | 9.88 KB | bkosborne |
|
Comments
Comment #2
joegraduateComment #3
joegraduateComment #4
Shawn DeArmond CreditAttribution: Shawn DeArmond as a volunteer commentedComment #5
Shawn DeArmond CreditAttribution: Shawn DeArmond as a volunteer commentedMaking this issue specific to re-creating the /admin/people/cas/create page in D8.
Comment #6
Shawn DeArmond CreditAttribution: Shawn DeArmond as a volunteer commentedThis 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.
Comment #7
Shawn DeArmond CreditAttribution: Shawn DeArmond as a volunteer commentedComment #8
bkosborneThanks guys. Looks like the form never made it in your patch Shawn =/
We can revisit this once the new parent issue is implemented.
Comment #9
Shawn DeArmond CreditAttribution: Shawn DeArmond as a volunteer commentedSorry!
Attached is the real patch.
Comment #10
Shawn DeArmond CreditAttribution: Shawn DeArmond as a volunteer commentedAdded another field in the form to add roles to the new users.
Comment #11
bkosborneThis is ready to be worked on now that externalauth was added.
Comment #12
bkosborneI'm not marking this as a requirement to get the D8 beta released. I see this has a nice-to-have at the moment.
Comment #13
metzlerd CreditAttribution: metzlerd as a volunteer commentedShould be marked as needs review unless I'm missing something.
Comment #14
Shawn DeArmond CreditAttribution: Shawn DeArmond as a volunteer commentedThe 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.
Comment #15
Shawn DeArmond CreditAttribution: Shawn DeArmond as a volunteer commentedForm submits and users are created.
Probably should use the batch api like the d7 version.
Also needs tests.
Comment #16
Shabbir CreditAttribution: Shabbir at Trigyn Technologies Ltd commented@Shawn, I applied your #15 patch to the latest git clone and found two issues:
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!
Comment #17
Shabbir CreditAttribution: Shabbir at Trigyn Technologies Ltd commentedI am adding a patch and interdiff which takes care of the two points stated by me in my previous comment. Hope it helps!
Comment #18
Nitesh Pawar CreditAttribution: Nitesh Pawar at Trigyn Technologies Ltd commentedComment #21
Shabbir CreditAttribution: Shabbir at Trigyn Technologies Ltd commentedSorry my bad, there was some issue in the patch which didn't allow to create the files. Here's the new patch!
Comment #22
bkosborneDoes 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.
let's keep this standardized and use cas.users_add
needs newline
same as above
maybe use "bulk_create" for the path instead
cas_users_bulk_add
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.
this messaging should not go in the validate method.
Form classes can use dependency injection, we should avoid using the global service container.
Comment #23
Shawn DeArmond CreditAttribution: Shawn DeArmond as a volunteer commentedNo! 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.
Comment #24
Shabbir CreditAttribution: Shabbir at Trigyn Technologies Ltd commentedSince 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?
Comment #25
bkosborneWe 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.
Comment #26
metzlerd CreditAttribution: metzlerd as a volunteer commentedI 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.
Comment #27
Shawn DeArmond CreditAttribution: Shawn DeArmond as a volunteer commentedI'm generally happy with the way this worked in Drupal 7:
Basically, the same as if the user logged in and the account was auto-created.
Comment #28
bkosborneShawn, 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.
Comment #29
Shawn DeArmond CreditAttribution: Shawn DeArmond as a volunteer commentedThat'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.
Comment #30
devil2005 CreditAttribution: devil2005 commentedHi,
I would like to test this but not in dev version of the module :(
This feature is ok ?
Thanks a lot
Comment #31
devil2005 CreditAttribution: devil2005 commentedHello,
Could you push the feature to dev version ?
Thanks a lot
Comment #32
bkosborneNot 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.
Comment #33
bkosborneComment #34
emerham CreditAttribution: emerham as a volunteer commentedAlso, 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:
Comment #35
sher1 CreditAttribution: sher1 as a volunteer and at Brigham Young University commentedI 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.
Comment #36
joegraduateThe 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?Comment #37
bkosborneThanks 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.
Comment #38
joegraduateAttached is an updated version of the patch in #36.
Updates include:
Comment #39
joegraduateMore coding standards fixes.
Comment #40
joegraduateRemoved unused variable.
Sorry for all the extra revisions. I'm done working on this for now.
Comment #41
bkosborneSorry for the late review. I lost track of this.
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.
Likewise, the title here (and name of the form class).
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.
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.
This should be wrapped in a try/catch, because the method may throw a CasLoginException
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
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.
Comment #42
joegraduateThanks for the feedback @bkosborne. I'll take a crack at making all the requested changes.
Comment #43
kclarkson CreditAttribution: kclarkson as a volunteer and commentedIs the last patch okay to use? Meaning it won't blow up my site ;)
Comment #44
fran seva CreditAttribution: fran seva as a volunteer and at Bluespark commentedHi @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?
Comment #45
bkosborneYes, 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
Comment #46
bkosborneI'm going to fix up that last patch and get this committed.
Comment #47
bkosborneHere we go. Addressed most of my original feedback in #41. Tests should pass, and I'll commit.
Comment #49
bkosborneOops, left a debug line in.
Comment #51
bkosbornelet's try that again
Comment #53
bkosborneThank you, everyone!