Add editing user passwords to the add user and edit user pages.

[drgrice1]

The previous form for editing passwords has been removed. Now all user details can be set when a user is created, and edited on one page.

The password column is the last column on both pages.

When creating a user if the password column does not consist entirely of white space, then the password will be set to what is entered (trimming spaces from the ends of course). Otherwise a password entry in the database will not be created for that user.

When editing a user the password column will show "****" for users that have a password defined (including if the password is the empty string because you can't tell the difference between that an any other encrypted password), and will be empty if there is no password defined in the database for that user.

That column will also have a checkbox to the right of the password text input. If that checkbox is checked for a user when saving the edit and the text input contains non-whitespace then the password for the user will be add or updated. Note that a password consisting entirely of asterisks is no longer allowed, and even if the checkbox is checked, the password will not be set or updated if the entry consists entirely of asterisks. This is a precaution to prevent someone accidentally checking the checkbox and leaving the asterisks (and potentially deleting some of the asterisks as well).

If the checkbox is checked and the text input is empty (or contains only whitespace), then the password for the user will be deleted.

Since the table for editing users is now quite wide, and horizontal scrolling is often needed to see the password column, special css has been added for this table that makes the first column sticky. That means that the first column will always be visible, even if the table has been horizontally scrolled all the way to the right. This helps to keep track of which user you are setting the password for.

Note that the behavior of setting the password to the student id when a user is created has been removed. That behavior is not consistent with the new behavior of explicitly setting the password. This is also the case when importing users via a class list and for the addcourse script. That behavior is changed for consistency, but I suppose could be switched back if that is not desirable. Note that when creating the admin course using the distributed adminClasslist.lst file as is done when initially installing webwork, the "admin" user password will still be set to be "admin" because that encrypted password is already stored in the adminClasslist.lst file.

Note that this is built on top of https://github.com/openwebwork/webwork2/pull/2436 since it needs the structure added there.

This is potentially to resolve discussion #1785. This may be controversial since it changes the behavior of setting the password to be the student id when a user is created if that is provided.

[Alex-Jordan]

I was able to change a user's password to *****, so I think something about that is not working.

It's good to do this (combine the password tab with the edit and create users tabs) but some things about this feel complicated/nonintuitive. I know it's all described in the help content, but a person will really have to go there and read carefully to understand the purpose of the checkbox (as well as how it can be used to delete a password). That checkbox is playing double duty, sometimes indicating you are confirming you want to change/set a password, and sometimes indicating you want to delete a password entirely (which is different from setting it to empty).

Could the checkbox be a separate column and its purpose is only to delete passwords? Then the password column could use a placeholder attribute that says like password set or no password set. Putting any text in the field (thereby making placeholder text disappear) would by itself be confirmation that you really intend to change a password upon save. Then you could remove the special checking for asterisks.

Currently, there is no alert if you attempt to change a password to all white space or asterisks and click save. If you are familiar with what is supposed to happen, you don't need an alert. But it feels like an alert that something special happened would be helpful because this will often be unexpected behavior (to typical faculty who have no studied that help file). But this is all moot if the checkbox becomes only for deleting passwords.

you can't tell the difference between that an any other encrypted password

If you wanted to, you could compare the password hash to whatever the encrypted hash is for the empty string.

About what the password defaults to... I would like to retire how it defaults to the student ID. But also there is so much documentation/expectation out there that this is what happens. So I'm conflicted. What do you think about a config variable whose value could be user_id, first_name, last_name, student_id, or null? Or for anything else, literally make that the password? The distribution could come with having this be student_id for continuity, but in my case I'd change it to null.

[drgrice1]

I was able to change a user's password to *****, so I think something about that is not working.

There was a typo in the regular expression, and that is now fixed.

It's good to do this (combine the password tab with the edit and create users tabs) but some things about this feel complicated/nonintuitive. I know it's all described in the help content, but a person will really have to go there and read carefully to understand the purpose of the checkbox (as well as how it can be used to delete a password). That checkbox is playing double duty, sometimes indicating you are confirming you want to change/set a password, and sometimes indicating you want to delete a password entirely (which is different from setting it to empty).

Could the checkbox be a separate column and its purpose is only to delete passwords? Then the password column could use a placeholder attribute that says like password set or no password set. Putting any text in the field (thereby making placeholder text disappear) would by itself be confirmation that you really intend to change a password upon save. Then you could remove the special checking for asterisks.

I think that using a placeholder as you describe instead of the asterisks is a good idea. I will implement that instead, and make the checkbox for only deleting passwords. However, I would rather not make another column. The table is so wide already. If you add another column, then you need a header that would say (perhaps) "Delete Password". The header is the issue, as it is what will make the table wider. Although, the visible label issue for the checkbox as is or in another column without a header is troubling.

If you wanted to, you could compare the password hash to whatever the encrypted hash is for the empty string.

Yeah, I know, but that is messy to do here. Generally it will only apply to users with passwords set to the empty string before this pull request, since with this pull request the empty string will no longer be created as an encrypted hash in the database. Instead the database row will be deleted.

About what the password defaults to... I would like to retire how it defaults to the student ID. But also there is so much documentation/expectation out there that this is what happens. So I'm conflicted. What do you think about a config variable whose value could be user_id, first_name, last_name, student_id, or null? Or for anything else, literally make that the password? The distribution could come with having this be student_id for continuity, but in my case I'd change it to null.

I guess that could be done. I don't really like it, but I suppose it isn't that bad to implement.

[drgrice1]

I have switched to using a placeholder instead of the asterisks in this pull request now.

[dlglin]

I also have concerns about changing the behaviour when importing class list files. I worry about the config variable approach that @Alex-Jordan suggests, since the value of that variable would not be available to the instructor in the UI, so how would they know what their server is doing?

There are three scenarios that I'm thinking about related to class list uploads:

1. An instructor wants to copy users from another course and maintain the same password. Currently this works if you export a class list and then import into the new course, since it captures the hashed password.
2. An instructor wants to set a new password for users being added to the course via class list. Before this PR this is accomplished by leaving the password column empty in the .lst file and putting the desired password in the student ID column.
3. An instructor wants to add users, but not set passwords since authentication is handled by some other module (e.g. LDAP)

For 2. I'd rather be able to specify a plaintext password separate from the student ID, which would be a change in behaviour. If that were implemented, then there would need to be some way to specify whether the password in the class list file is hashed or not.

In all of this we also have to decide how committed we are to backward compatibility. One option (that I don't love) is putting a dropdown in the import form for the instructor to choose how to set passwords. The options could be "set the password to the student ID", "the password column contains hashed passwords", "the password column contains plain-text passwords", and "do not set a password for users (authentication is handled by an external module". The first and second options would allow people to use their existing .lst files without modification. Even I'm confused by the wording I just provided, so I don't know how usable that would be.

[drgrice1]

I made it so that the checkbox is not present if a user does not have a password. That is in the sense of a user not having a row in the database at all. The checkbox will still be there and it will say "password set" if the user has a password that happens to be a hash of the empty string. I still don't feel that it is really worth it to handle that in a special way since after this pull request goes in (assuming it does), that case will be for passwords set to the empty string before this pull request went in.

@dlglin: The scenarios 1 and 3 that you listed work with this pull request already, even without an option added. I have to say that scenario 2 is a hack that really should not be supported unless it is the case of using the student id. And to be honest, even that conceptually was never a good idea. It really is a security vulnerability. Better would be an option to generate random secure passwords. Those passwords could be reported to the instructor importing the users, and then that instructor would be responsible for securely getting the passwords to the users.

I don't see any way that scenarios 2 and 3 could work together without changing the class list format. At least not in a good way. I don't consider the drop down idea a good way, mostly because it relies on the instructor importing the class list to choose the correct option that matches what is in the file. That seems precarious. We could change the class list format and add an "unencrypted password" field to the csv format. For maximal backwards compatibility that would need to be the last field (although it makes a bit of an odd order). The current field order is student_id,last_name,first_name,status,comment,section,recitation,email_address,user_id,password,permission. So we would just make that student_id,last_name,first_name,status,comment,section,recitation,email_address,user_id,password,permission,unencrypted_password. Then whichever is set is used, probably giving priority to the "password" field. Then a drop down could be added that chooses a fallback field to use in the case that neither of those are provided.

I think that a course environment option for the default behavior when creating a user from the user interface could still be added. I actually thought that was the case that @Alex-Jordan was talking about, and not importing from a class list.

In addition there is the behavior that the addcourse script uses. The current behavior is to copy what importing from a class list in the user interface does (down to even duplicating the code).

[drgrice1]

There is now another change to this pull request.

There is a new course environment option $fallback_password_source. That can be one of 'user_id', 'first_name', 'last_name', 'student_id', or 0.

This option is used in two ways.

First, on the add users page and in the import classlist form there is a drop down menu that allows the instructor to select a "Fallback password source" from among the user_id, first_name, last_name, student_id, or none, and the initially selected option is whatever the $fallback_password_source is set to. On the add users page, if the password field is left empty, then the fallback source is used for the password assuming that field is provided. When importing a file, if the previous (encrypted) password field or new unencrypted password field are both not provided in the class list file, then the fallback source field is used (assuming it is provided).

Second, if the importClassList.pl or addcourse scripts are used, the $fallback_password_source is used directly to determine where to get the password assuming the (encrypted) password and unencrypted password fields are not set in the class list file.

As mentioned there is a new optional last column in the class list file that is the unencrypted password if set. Note that the old (encrypted) password field is used if it is given, then the unencrypted password field next, and if neither of those are given then the $fallback_password_source field is used.

Note that I also removed the "Add which new users?" with the options to select "any users" or "no users". That drop down doesn't make any sense. Why have an option to import a class list file, but not import any users? If that was meant to be a protection against accidentally importing a class list file, then the default selected option should have been "no users", but the default was "any users". Why in the hell would anyone choose to import a class list file, and intentionally select "no users"?

[pstaabp]

Overall, this is looking good. I noticed when editing a user (I was examining the lack of a password), then I think there's some CSS that needs to be tweaked between the User List and the text above it:

[pstaabp]

Looks okay now. I guess the other option is to not make it a caption of the table--is that done for accessibility?

[drgrice1]

Yeah, if it is a caption, then the semantics for accessibility are correct.