LTI account creation patches + discussion about aborted connections being reported by the database#1150
Conversation
…vel,
so if the test
defined( $ce->{userRoles}->{$ce->{LMSrolesToWeBWorKroles}->{$LTIroles[0]}} )
fails - the account will not get created with an undefined permission level,
which prevents logins from working. The most likely cause of such problems are
missing LMS roles which are not defined in %LMSrolesToWeBWorKroles (which
is usually defined in conf/authen_LTI.conf).
if the first one would fail.
|
Note that Sean Fitzpatrick reported in the WW forums having the same issue as originally motivated me to look into possible causes of a missing permission level for an LTI created account. @dlglin reported also having similar problems after an attempt to change to a different database "server" and that reverting to the prior database server seems to have resolved the problem. He raised the suspicion that the connection to the database dropped or something else caused a transaction to be lost. In my case, the problems were seen with mariadb 10.4 as a Docker container, using a somewhat old image. |
|
@taniwallach we were getting students without permission levels due to aborted DB connections. Can you check the error log for mariadb and see if there are any errors due to aborted connections? |
There are certainly many aborted connection. The SQL status variables give a count. However, it could be that WW is just sloppy about closing connections. The extent of the problems encountered does not seem close to the number of aborted connections. It turns out that the mariadb container logs to Docker's log handler. So to see the logs one would use and I dumped the logs from docker into a file, so I could look more carefully and count things. There are many discussions about there being many such warning messages. |
|
I tried using The data all looked fine, although WireShark showed the funky bridge packet handing as re-transmissions. It seems that the problem may be an issue with the Perl DBD driver we are using... and it seems that using the newer Details below: There is a mention of many aborted connections in perl5-dbi/DBD-MariaDB#151 (comment) after an upgrade from As a result, I decided to try out the "new" Perl DBD driver for MariaDB and mySQL:
The stated reason for the fork was "The goal of our effort was to fix MariaDB compatibility, Perl Unicode support and security related bugs." This new driver is packaged for Ubuntu 20.04, but will require some changes to use it in WW (ex. it always uses utf8, so does not recognize the special settings used to force utf8 in the old driver). The new Perl module is available as a package for Ubuntu 20.04, but not for 18.04. Last night, I managed to get my development server to use that DBD module and Ubuntu 20.04. It has far less load, and in the past 18 hours only showed aborted connections of the "timeout" type and none "error reading communication packets". Those are also more infrequent, and I only see a single clump of 11 warnings - all a single 5 minute period. They all look like That seems to suggest that there may be some problem with how the older I set up my main production server to use the new Note: I also made some changes to the Modifications needed to work with the
|
taniwallach
changed the title
|
I have switched my test server to DBD::MariaDB, and right now we're still seeing the "Aborted connection" errors. I'll report back if we discover anything else about this. |
In several weeks of use in production, I have not seen any more aborted connections due to I still see some "rare" aborted connections, but only those for |
taniwallach
requested review from
Alex-Jordan and
drgrice1
and removed request for
drgrice1 and
mgage
taniwallach
added
NeedsTesting
drgrice1
left a comment
drgrice1
left a comment
There was a problem hiding this comment.
I think that this pull request is okay, but I am not entirely sure about it. Looking at the discussion, it seems that the actual problem is not entirely understood, and that this pull request implements a work around.
I guess it will be okay for now, but we should look deeper into the issue with database connections being dropped.
The PR is an attempt to make sure that LTI will at least create an account with student level privileges, when the configured conversion table for LMS role to WeBWorK privilege level fails to provide a result. It is not likely to cause problems beyond possibly creating student mode accounts for special classes of LMS authenticated users. It is possible that the missing permission level could also be caused by a database connection failure between initial account creation and the permission level being set. Since these are separate transactions with the database - this is certainly possible. However, you are correct that the cause of the database connection errors is not well undestood. The discussion about the database connection issues is somewhat orthogonal to the changes in the PR itself, and really belong in:
I can report that in over 3 months of using I do still see some "rare" aborted connections, but only those for Thus, at least in my setting: MariaDB 10.4 in a Docker container on the same host as WeBWorK in Docker - the dropped connection issue is no longer occurring, while it was happening with the "older"
|
taniwallach
added
the
Do Not Merge Yet
|
I tested this by merging it into develop (locally) and then a real student account in D2L (the LMS) successfully triggered account creating in WeBWorK. I deleted my admin account in the WW course and tried to enter from the LMS to see what happened. I get the error screen indicating it will not create professor level accounts. Same as in the past. (Side note: this error screen is "frightening". Sometimes a guest instructor or staff is in the LMS course and tries to use the WeBWorK link, and sees this. Is a friendlier screen for this easy to implement?) I commented out the roles that map to "professor" from LTI config. Then I entered from the LMS with my instructor level LMS account. It created a student level account for me in WW, which is the intent of this PR. I'm not sure how I feel about creating student accounts in the WW course this way. For one thing, if we are simply rejecting account creation for a too-high LMS account, why not also reject account creation for an unrecognized role? And if we are creating accounts for one or both situations (too-high LMS roles or unrecognized LMS roles) did you consider making the WW account guest level instead of student? The instructor of the WW course could elevate permissions later to whatever is appropriate. |
|
@Alex-Jordan: You are expressing the same concern that I had with this. That is why I stated that I was not certain about this pull request. I think more consideration is needed. |
|
I would be comfortable approving this if the unrecognized LMS roles become "guest" accounts in WeBWorK instead of "student". And for consistency, I would like to see "too-high" LMS roles also get account creation but with "guest" WeBWorK permissions, but that could be done separately. Yes, that still leaves things not fully understood, but it seems like a good change anyway. |
|
@Alex-Jordan: I think that sounds reasonable. |
Alex-Jordan
left a comment
Alex-Jordan
left a comment
There was a problem hiding this comment.
@taniwallach Is it OK with you that the unclassified new accounts be given guest status instead of student? My thoughts are:
(1) we don't really think this is responsible for the instances that have occurred, right? We think that is because of dropped connections, in which case the new people still have no permission level.
(2) At my school, it is common to have staff join the LMS course. If they follow the LTI link, this could make them be a "student" in WW. Their name will be inserted among all the users when grades are managed. And it's not obvious how to find them and weed them out. But if they have "guest" status, it will be easier to identify them.
Also if you don't object to that change, could you go ahead and make it so that users with too-high permission level are treated the same way? From time to time, an instructor has ended up with a WW course they are not enrolled in. Or a faculty mentor is in the D2L course and would like to enter the WW course. But if they try, they are recognized as having too-high permissions, and they get an error screen that gives the wrong impression that something is broken. I think these situations could allow entry, and soon after that, an admin or professor would elevate their status.
No - I just was to try to minimize creation failures and broken accounts. Could you make a PR to my branch with the change? We might want to make it more visible to a user that their account is a guest account, and that if they need a student account - they should write the professor, as otherwise some legitimate users may be confused by what does not work.
I suspect that most of the problems were probably dropped connections, but the data to tell does not exist. (Once some security expert recommended keeping several days of network sniffing data for all servers somewhere, and that sort of thing might have been helpful in debugging this issue.)
A good point.
If it can wait a bit - yes. (Currently a bit overloaded with unscheduled work demands.) If someone has a chance to PR to my branch a patch sooner, I'll merge it.
Makes sense. Again - I think a "banner" indicator for "guest status" might help this approach work more smoothly. |
|
It's worth noting that the current behaviour for users without a permission record is to assign them a permission level of student. This happens the first time that the user record is accessed in the Classlist Editor: webwork2/lib/WeBWorK/ContentGenerator/Instructor/UserList2.pm Lines 454 to 465 in 03eca4c |
|
@dlglin: Although it is also worth noting that if one is creating a user via the class list editor (UserList2.pm), then the user is being intentionally created. If a user is created in the manner that this pull request is implementing, that user is not necessarily intentional. |
|
@drgrice1 That code in UserList2.pm runs on all users regardless of how they were created. |
|
This solved a different issue and needs work. |
taniwallach
removed
Do Not Merge Yet
5 participants
Update: The discussion thread gets into matters related to aborted connections being reported in the database logs.
See the forum post: https://webwork.maa.org/moodle/mod/forum/discuss.php?d=4958
I ran into an issue where a few LTI created accounts did not have a permission level set. Those accounts were reporting when login attempts were made a
WeBWorK errorsomething like below (pulled out of the Apacheerror.log):When I viewed the problematic accounts in the class list editor, the system automatically set a permission level, and reported
added missing permission level for user(triggered inlib/WeBWorK/ContentGenerator/Instructor/UserList2.pmandlib/WeBWorK/ContentGenerator/Instructor/UserList.pm). That resolved the problem.It is not totally clear how an account was created in the few such cases without any permission level set.
I suspected that these students may have had a non-standard LMS role at the time they first accessed the LTI link. The most likely cause of such a problem would be missing LMS roles which are not defined in
%LMSrolesToWeBWorKrolesa conversion table defined as a hash inconf/authen_LTI.conf(and possibly modified elsewhere).However, I tried to recreate such an error by modifying the conversion table for an old course, and got a different error when attempting to have LTI create an account, and no account was created (apparently due to the
croak). Thus, it is not clear that a missing entry in the conversion table would lead to a missing permission level.The
Use of uninitialized value in hash element at /opt/webwork/webwork2/lib/WeBWorK/Authen/LTIAdvanced.pm line 618.message is addressed in the second commit of the PR, by doing thedefinedtest in 2 stages.In any case, I am proposing to add a fallback setting for
$LTI_webwork_permissionLevelasstudentlevel so if the testfails during the account creation process, the code will fall back to creating an account with
studentlevel access. To this end thecroakis replaced bywarn. With the patch and an intentionally damaged %LMSrolesToWeBWorKroles set in a course, account creation succeeded and a student account was created.It is not clear if this fix will actually address the original problem of preventing an account from being created with an undefined permission level, which prevents logins from working, as I could not trigger that behavior (before applying the patch) but it will certainly reduce account creation issues in other cases.