WeBWorK 2.20 Release Candidate #2721
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Change key bindings for folding in PGCodeMirror.
…ames Fix rendering static images when checking answers in the PG problem editor.
Remove the special case in the `getThirdPartyAssetURL` method for MathQuill.
…intuitive. Previously when any of the dates were changed, the open, reduced scoring, close, and answer dates were ensured to be in the correct order by always changing the input later in the last to a later date if needed. This instead acts on the input that was modified. That input value is not changed from what the user just changed it to. Any earlier inputs in the list are set to the new date in the changed input if they are later than that date, and any later inputs in the list are set to the new date in the changed input if they are earlier than that date. This is to address issue #2581.
Previously the open date was never acted on, and so the for loop started at one which skipped the open date. Now it needs to be acted on.
When using the "Today" or "Now" buttons, the "changed" class is not being added to the input. When the handlers for those buttons are called, the "change" event needs to be triggered so that this happens.
A split toggle is implemented with two buttons. A button with the text (in this case the filter name) and a button with the caret for the dropdown menu. However, `student-nav-filter-selector` div only has one button and does not have the second button for the caret. This causes issues with the border of the button. Since the filter button has the `dropdown-toggle-split` class the `btn-group > .btn.dropdown-toggle-split` selector applies and the rounded borders on the right are removed. That makes the button look wrong. This just removes incorrect `dropdown-toggle-split` class that is causing the problem and makes it so the button is displayed correctly.
Change behavior when changing dates in a datepicker group to be more intuitive.
Add LTI configuration info to the LTI course map page.
…conf.dist file. The LMS course ID is not the issue. It is the LMS context id. That can be the same for two courses from different LMS's. So the `ConsumerKey` is used in that case to distinguish.
Make the student nav filter selector not split toggle.
Fix the comment about the LTI 1.1 consumer key in the authen_LTI_1_1.conf.dist file.
Fix a rounding error with SingleProblemGrader point values.
This updates the Shibboleth authentication module to fit into the new
scheme of the general webwork2 authentication process. The module is
set up to work just like all of the other up to date webwork2
authentication modules.
It has its own configuration file (conf/authen_shibboleth.conf.dist)
that should be used instead of adding a buch of variables to
localOverrides.conf. The include statement in localOverrides.conf
should be uncommented, and the dist file copied and modified.
Furthermore, relatively complete instructions on how to use the
authentication module are in the comments in the configuration file.
The variables in the configuration file are all the same as before,
except that there is one new one. That is the
`$shibboleth{bypass_query}`. Previously "bypassShib" was hard coded for
this purpose. Now that can be configured. If that variable is not set
(and for those using this module before it wouldn't be), then the bypass
parameter will not work. So this is the only real change from before.
The issues that were causing webwork2's session not to work have been
fixed. This means that proctored test access will work again.
The library browser, pg problem editor, and everything else that uses
the rpc endpoints will work correctly. There simply is nothing special
that the authentication module needs to do here, and most importantly it
needs to not do anything special (like reverting to the base
authentication module). The rpc enpoints now use the usual
authentication methods, and that does work with mod_shib.
Don't worry. The module is still strict and has warnings enabled. All modules that derive from `Mojo::Base` are. I forgot to remove these when changing to that, and those cause warnings since Mojo::Base disables the warnings for using signatures.
Currently the parameter works to sign in, but if you try to do anything after signing in, then you are redirected to sign in to the Shibboleth identity provider. To prevent that the parameter needs to be considered a persistent authentication parameter.
Check that `$ce->{LTIVersion}` is defined and not empty before using it.
This variable being initialized should ensure that the other LTI variables
used here are also initialized.
Fix uninitialized variable warning when not using LTI.
Fix (and completely revamp) the Shibboleth authentication module.
…ted users. This is to fix the issue discussed in https://webwork.maa.org/moodle/mod/forum/discuss.php?d=8565#p21514. If someone needs the institution roles, then the new $LTI{v1p3}{AllowInstitutionRoles} option defined in conf/authen_LTI_1_3 can be set to 1, and then those roles will also be considered.
For LTI 1.3 only consider roles in the context for automatically created users.
…partment.
This is so that evaluation of the configuration files gives better
messages.
Currently if a file is included from another included file then
exceptions in the inner included file are not shown, but rather the
exception from the outer included file is shown. For example, if
`include('authen_LTI.conf')` is called in `localOverrides.conf` which is
included from `defaults.config`, and `authen_LTI.conf` does not exist,
then the current message says `Failed to read include file
/opt/webwork/webwork2/conf/localOverrides.conf (has it been created from
the corresponding .dist file?): No such file or directory` and now it
says `Failed to read include file /opt/webwork/webwork2/conf/authen_LTI.conf
(has it been created from the corresponding .dist file?): No such file
or directory`. This was fixed by adding a local die signal handler to
the `include` method defined in the safe compartment. That pushes
messages onto a `state` array variable. When an exception occurs, only
the last message is shown which is the one from the inner inclusion
since that exception is pushed onto the array first (the exception from
the outer inclusion happens second as a result of the inner exception).
In addition if warnings occur inside an included file, then this
actually becomes an exception. The reason for this is that the global
warning signal handler defined in `lib/Mojolicious/WeBWorK.pm` is called
inside the safe compartment used for evaluating the configuration files.
This causes an exception when that handler attempts to access the
controller `stash` which is not defined in the safe compartment. Note
that this only occurs for a request, and not when the application first
loads because that doesn't go through the global warning signal handler.
Although, that isn't the exception you see because when that exception
occurs Mojolicious attempts to wrap that exception with a
`Mojo::Exception`. That causes another exception because that package is
also not shared to the safe compartment. So you get an exception about
the `new` method of the `Mojo::Exception` package not existing which is
completely uninformative. To fix this a local warning signal handler is
used that pushes messages into a warning array. Those warnings are
rebroadcast to the global warning signal handler after the safe
compartment evaluation of the configuration files is complete. A simple
way to test this is to add a `warn` statement to `localOverrides.conf`
and try to open a page in the browser.
…oblem.pm. Both the `JSON` module and `Mojo::JSON::encode_json` method are loaded in `lib/FormatRenderedProblem.pm`. This causes first request issued to the `render_rpc` endpoint after the server starts to issue the warning `Prototype mismatch: sub FormatRenderedProblem::encode_json ($) vs none at /usr/lib/x86_64-linux-gnu/perl-base/Exporter.pm line 63.` There is no need for both modules that both do essentially the same thing. The `Mojo::JSON::encode_json` method also UTF-8 encodes the data as was done before. Note that the only place the `JSON` module was used was for rendering the `raw` format.
…ment.
The `local $SIG{__DIE__}` handler causes an issue with the
`Mojo::Reactor::EV` package and leaves the app in an unrecoverable
state. So don't use that. Instead share a localized package array
variable. When `include` has errors it pushes a message onto that
array. The first thing on that array is used for the exception (so the
first exception that occurs).
PG still uses `JSON` for now, so this doesn't quite remove the dependency on the module. But PG can be switched also. Note that the module uses `Cpanel::JSON::XS` (if that module is installed) which is the recommended perl JSON encoding and decoding module. `Mojo::JSON` falls back to their own pure perl implementation if `Cpanel::JSON::XS` is not installed. Note that the `Mojo::JSON::encode_json` method UTF-8 encodes the output and is canonical (sorts object keys). The `Mojo::JSON::to_json` method does not UTF-8 encode the output, but is still canonical. The corresponding `Mojo::JSON::decode_json` decodes UTF-8 encoded JSON and `Mojo::JSON::from_json` method decodes non UTF-8 encoded json.
…o::JSON` as well.
…or-handling Rework the warning and exception handling in the config file safe compartment.
…onflict Fix an issue with conflicting JSON modules in use in FormatRenderedProblem.pm.
…ers. This ensures that only the allowed characters for a courseID are allowed in the URL path. This restriction occurs at the Mojolicious router level. This means that a request like `https://server.edu/webwork2/courseID%22%3E%3Cscript%3Ealert('hello')%3C/script%3E` is just sent to the vanilla "Page not found" page. Note that the URL above (or one like it) comes from a security scan that someone ran, and that this is NOT a security vulnerability. However, it is not handled the best. Currently the above URL results in numerous warnings about the use of an uninitialized value in string concatenation, and then eventually an exception because of a missing database table. None of that is harmful and a URL like that shown can not be manipulated to do anything harmful, but the warnings and exception can be cleaned up. That is dealt with by allowing routes to specify restrictive placeholders that are passed to the `any` or `under` method of the `Mojolicious::Routes::Route` object. For the courseID the restriction is that that portion of the URL must match `qr/[\w-]*/` which is the same restriction used for the courseID when creating a course. I also changed the problemID to use the built in `:num` placeholder type which is equivalent to what was being done before (i.e., `qr/\d+\`). By the way you can run `./bin/webwork2 routes -v` to see all webwork2 routes and the regular expressions that are used to match them. Furthermore, since the courseID can be specified in a request parameter (via the RPC endpoints), the `WeBWorK::CourseEnvironment` chops off any single quotes and everything after them that occur in the passed in `courseName` in the `$seedVars`. The problem is that when the seed variables are `reval`ed into the course environment safe compartment with `$safe->reval("\$$var = '$val';")`, any single quotes in `courseName` end the first single quote in that statement causing a syntax error. The exception from that is ignored because the errors from that `reval` are not caught, but it results in the `courseName` in the course environment being undefined. That is what causes all of the unininitialized warnings mentioned above, and furthermore the exception from the missing database table (because the login page is loaded and tries to look up guest users for the course, but the user table doesn't exist for this undefined course name). This is all the result of investigating the suspected vulnerability posted about in https://webwork.maa.org/moodle/mod/forum/discuss.php?d=8686. The information was emailed to [email protected], and relayed to @dlgin and myself. I have thoroughly analyzed and tested the suspected vulnerability, and can see that it is not. I believe the issue that caused the server to go down was simply the security scanning tool overwhelming the server with to many requests, and the server not being configured properly for rather meager memory limitations.
Sanitize the courseID from either the URL path or the request parameters.
The `use JSON` statement from before was removed in these files because I though the module was not used. However, these files used the methods in a different way and I missed it. So these files need the appropriate methods from `Mojo::JSON` imported.
Add a couple of `use Mojo:JSON` statements that are needed.
I bet you thought all of the random order gateway test issues were resolved. Nope. When a page change occurs in a random order test, the answers saved in the hidden inputs for the problems not on the current page are still not respecting the order. As a result the answers get moved around to incorrect problems each time that a page change occurs.
enhance doc2 for achievements
Fix random order page change issue.
Restore previous behavior of the "Scroll of Resurrection" achievement item.
This was a cut and paste error in #2655. I cut the similar code from `htdocs/js/DatePicker/datepicker.js` and needed to change the input variable name. Currently if you select a date on the import form no date is actually entered into the input, and console errors are output. Obviously that is fixed with this pull request.
Fix the import form date shift input on the sets manager page.
Currently the user achievements are obtained from the database in the user loop. This takes a long time when there are lots of users. So instead, this gets all user achievements for all users before the loop and references them by user id and then achievement id. Testing this on a course for 5000 users shows this gives a significant speed up on load time for the page. With the develop branch it takes around 25 seconds, and with this branch it takes around 3 seconds. Note that there were also two redundant queries (one that listed all achievements and then one that fetched all achievements on lines 49 and 50 in the develop branch) for which the data from those queries was not even used. Those were removed.
Instead of listing user achievements and then deleting them one by one in a loop, just delete them with one query using a `where` statement.
It turns out that none of the ContentGenerator controller objects are
being destroyed when a request finishes. So each hypnotoad process (or
morbo in development) has every ContentGenerator controller object for
every request it renders saved in memory until the process ends. That
means that everything it had a reference to is also saved in memory.
That includes a `WeBWorK::CourseEnvironment` instance, a
`WeBWorK::Authen` instance, a `WeBWorK::Authz` instance, and a
`WeBWorK::DB` instance (and everything it has a reference to).
Furthermore, even if the controller objects are destroyed and the
`WeBWorK::DB` instance with it, none of the `WeBWorK::DB::Schema`
instances (one for each table) are ever destroyed.
There are two things that cause these references to be kept when they
shouldn't be.
The first is the more obvious circular reference..
A `WeBWorK::Controller` object (from with the `WeBWorK::ContentGenerator`
modules derive) keeps a reference to a `WeBWorK::Authz` instance, and
that instance keeps a reference back to the controller. However, the
`WeBWorK::Authz` doesn't weaken the reference back to the controller.
That was my fault in the conversion to Mojolicious I commented out the
`weaken` statement that prevented this circular reference. That was
because in the initial conversion the controller didn't have a reference
to the `WeBWorK::Authz` instance, and so it was going out of scope and
causing problems. However, when the reference to that instance was
added that should have been uncommented.
Another case of this is that `WeBWorK::Authen::LTIAdvanced` and
`WeBWorK::Authen::LTIAdvantage` packages were keeping a circular
reference to the controller as well. The new methods in those packages
was just deleted so that they use the `WeBWorK::Authen` new method
which already does the right thing.
A third case occurs with the `WeBWorK::DB` instance and the
`WeBWorK::DB::Schema` instances both of which hold references to each
other.
The other thing that causes an extra reference to be kept is an
anonymous subroutine (or closure) using an instance. In this case Perl
forces the instance to be kept in scope for usage in the closure.
The global `$SIG{__WARN__}` handler defined in `Mojolicious::WeBWorK`
uses the `$c` controller instance, and that is what prevents the
`WeBWorK::ContentGenerator` modules from going out of scope. So that
instance in the `around_action` hook needs to be weakened.
For the `WeBWorK::DB::Schema::NewSQL::Std` and `WeBWorK::DB::Schema::NewSQL::Merge`
objects the issue is the `transform_table` and `transform_all` closures
for the sql abstract instances. Those prevent the schema objects from
going out of scope and so the `$self` in the `sql_init` methods where
those closures are defined needs to be weakened as well.
that the $SIG{__WARN__} handler is reset in the after_dispatch hook so
that the reference to the controller is released.
Fix memory leaks.
Improve loading time for the achievements leaderboard.
…-speed-tweak Speed up deleting of global user achievements.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It is time for this. We have talked about just using the develop branch, but I think it is nice to have a separate branch for the release candidate. It just keeps things separate in a nice way.
So as usual, re-target pull requests for this branch that you want to get into the release, and I will handle synchronizing those to develop as they are merged.