Get the user parameter in scalar context before checking permissions

[xcompass]

Problem

WeBWorK::Controller::param returns via a bare return when the requested parameter is not set:

return unless exists $c->{paramcache}{$name};

A bare return yields the empty list in list context, so passing the result straight into another call does not pass undef — it removes the argument entirely. In ProblemSets::can:

return $c->authz->hasPermissions($c->param('user'), 'access_instructor_tools')
    || $text ne DEFAULT_COURSE_INFO_TXT;

When there is no user parameter, hasPermissions receives a single argument and its argument check croaks:

hasPermissions called with 1 arguments instead of the expected 2: 'access_instructor_tools'
    at lib/WeBWorK/ContentGenerator/ProblemSets.pm line 33

can('info') is reachable without a user parameter, so an unauthenticated request to a course page returns a server error rather than rendering. We hit this on a production server from ordinary unauthenticated crawler traffic.

Change

Assign the parameter to a scalar first, which forces scalar context and is what the rest of this file already does (see initialize, which does my $user = $c->param('user');). An unset parameter is then passed through as undef and hasPermissions answers normally.

Scope

The same "pass param() directly as an argument" pattern appears at a number of other hasPermissions call sites, for example in Grades.pm, GatewayQuiz.pm, and Instructor/FileManager.pm. Those are only reached once a user parameter is set, so they do not currently fail, but they are fragile for the same reason. I have kept this change to the one call site that actually breaks; happy to submit a follow-up sweep if you would prefer the whole pattern addressed at once, or to instead make param return undef rather than the empty list in list context — though that would change behaviour for callers that legitimately want a list.

[drgrice1]

Please rewrite your entire message above without AI and explain things properly. I do not want to read the AI gibberish. I deal with that enough now in working with Claude code. This pull request will not be considered until you do this.

[drgrice1]

I do not see the need for this pull request. This page does not render without authentication, and in that case $c->param('user') will be set. It is set by the set_params method of the WeBWorK::Authen package.

In general, though there is a need to redo the $c->param('user') usage. It should not be using the param call at all for most places it is used. Instead $c->authen->{user_id} should be used.

[drgrice1]

If you can demonstrate that this pull request is actually needed (I have done extensive testing, and I don't see any way that the can('info') calls can ever occur for this module without authentication, and so I doubt it is.), then this should be changed to

Suggested change

	# Note that the user parameter must be obtained in scalar context. In list context the param method
	# returns the empty list when the parameter is not set, and that would silently remove the argument
	# from the hasPermissions call below, making it croak about being given too few arguments.
	my $user = $c->param('user');
	return $c->authz->hasPermissions($user, 'access_instructor_tools')
	return $c->authz->hasPermissions(scalar $c->param('user'), 'access_instructor_tools')

Adding scalar does the same thing with a much smaller footprint, and the lengthy comment is not needed.