Teach 1975/ensure in development units are not visible to signed out users by alex-m-brown · Pull Request #66459 · code-dot-org/code-dot-org

alex-m-brown · 2025-06-11T18:29:20Z

Currently, with the new /courses/.../units/... URL structure, units were not being authorized correctly with authorize_resource. This PR adds a separate before action to manually authorize the selected unit.

Background

I found that the issue came down to CanCan authorizing the Unit model instead of the specific unit object with the new url structure. Based on my understanding, this is because the new urls no longer have the :id parameter to search for the unit by, which CanCan falls back to.

@script is set in :set_unit. The instance_name part of authorize_resource is telling it to reference the @script instance when authorizing. Through debugging I confirmed that @script was properly set when authorize_resource was called, it just wasn't being used to authorize. It wasn't until I added an id_param that it actually used @script. From my understanding, id_param tells CanCan what parameter to use to find the unit if @script is empty when it tries to authorize. With the new url structure, we don't get a param that could be used by that find method to identify a Unit.

Note: db_query_test.rb had to be updated because prior to this PR, the unit was rendering a no_access instead of the unit overview page. the changes in this PR have the side-effect of fixing the test, resulting in a larger query count.

Links

This solves 2 tickets:

Jira: TEACH-1975
Jira: TEACH-1974

Testing story

Added unit tests

…nto TEACH-1975/Ensure-in-development-units-are-not-visible-to-signed-out-users-

…e-not-visible-to-signed-out-users-

davidsbailey · 2025-06-12T00:01:35Z

+  # TEACH-1975 for more details).This solves the issue by explicitly calling authorize!
+  # on the @script if it is set.
+  private def authorize_script
+    authorize! params[:action].to_sym, @script || Unit


I think I just ran into a similar issue on my UI test PR, which involves some similar logic in the script levels controller. I don't see how this can work in all cases because cancan doesn't know about the published state of the UnitGroup, which is (should be?) a factor in whether the unit is visible to a user via show.

davidsbailey · 2025-06-12T00:01:44Z

can you please make sure you are testing these scenarios or something equivalent?

original unit group is in_development, modular unit is in beta, student CAN access /courses/modular/units/1

original unit group is in beta, modular unit is in_development, student can NOT access /courses/modular/units/1

hm, yeah, just testing manually the first thing i found which was csd-2025 in preview as the original course and focus-on-coding-2025 which is in development as the "modular" course. I was able to access courses/focus-on-coding-2025/units/1. It looks like we'll need to add an additional check on the unit group.

thoughts on authorizing the unit group in that same method?

private def authorize_script authorize! params[:action].to_sym, @course || UnitGroup authorize! params[:action].to_sym, @script || Unit end

nevermind. that only works when the modular course is "stricter" than the original course, for example, the original course is stable, but the modular course is in development.

I got as far as being not quite sure how to approach this -- I'm not sure cancan is well set up to authorize when multiple objects are needed. Will dig in more this morning.

We could use other auth methods outside of can can is that's what's needed. Not ideal, however, everything should be as simple as possible but not simpler.

thoughts on authorizing the unit group in that same method?

private def authorize_script authorize! params[:action].to_sym, @course || UnitGroup authorize! params[:action].to_sym, @script || Unit end

I think you are on the right track here --

authorize UnitGroup based on published state, etc

authorize Unit based on login_required but NOT published state (of unit or of original unit group)

thoughts?

davidsbailey

This solution looks great! One readability nitpick inline.

davidsbailey · 2025-06-13T16:11:52Z

-      else
-        false
      end
    end


davidsbailey · 2025-06-13T22:44:25Z

+      test_user_gets_response_for :show, response: :not_found, user: nil,
+                                  params: -> {{course_course_name: original_course.name, unit_position: unit_position, position: lesson_position}},
+                                  name: 'signed out user cannot view in-development original course'


I'm not sure if this is technically against any style rule, but I'm finding this formatting to be a bit hard to read, at least with github wrapping it at around 100 chars. in particular it's hard to quickly see the name or the difference in params. Would this style work for you?

code-dot-org/dashboard/test/controllers/api/v1/pd/course_facilitators_controller_test.rb

Lines 11 to 16 in b09525e

test_user_gets_response_for(

:index,

user: user_type,

response: response,

params: {course: Pd::Workshop::COURSES.first}

)

or this (from later in this PR)?

code-dot-org/dashboard/test/controllers/scripts_controller_test.rb

Lines 1619 to 1622 in 1217cef

test_user_gets_response_for(:show, response: :success, user: :teacher,

params: -> {{course_course_name: @pilot_course.name, position: 1}},

name: 'teacher without pilot access cannot view pilot unit'

) do

as an alternative, I could also see extracting original_path and modular_path param objects that could be passed in as params: -> {original_path} to make the lines a bit shorter.

sorry for the nit 😬

No apologies, I agree! I'll update it to make it more readable. thanks!

…e-not-visible-to-signed-out-users-

alex-m-brown added 6 commits June 10, 2025 16:32

authorize current unit

093af16

separate authorize script before action

c3c9376

add single unit course factores

e56c4ca

Merge branch 'TEACH-1678/add-new-factories-for-single-unit-courses' i…

d77791d

…nto TEACH-1975/Ensure-in-development-units-are-not-visible-to-signed-out-users-

use new url structure in in-development and pilot tests

3dc99a2

Merge branch 'staging' into TEACH-1975/Ensure-in-development-units-ar…

ae56b5b

…e-not-visible-to-signed-out-users-

alex-m-brown marked this pull request as ready for review June 11, 2025 22:16

alex-m-brown requested review from davidsbailey and daynew June 11, 2025 23:00

davidsbailey reviewed Jun 12, 2025

View reviewed changes

alex-m-brown added 4 commits June 12, 2025 13:39

use unit group when authorizing units

2ebd66c

fix unit group param

6dea48a

add more unit tests

4afee05

add nil check for ability

1217cef

davidsbailey mentioned this pull request Jun 13, 2025

move allthethings unit to be owned by original-allthethings-course #66458

Merged

alex-m-brown requested a review from davidsbailey June 13, 2025 17:56

davidsbailey approved these changes Jun 13, 2025

View reviewed changes

alex-m-brown added 4 commits June 16, 2025 12:39

make tests more readable

1ff5dde

Merge branch 'staging' into TEACH-1975/Ensure-in-development-units-ar…

452f876

…e-not-visible-to-signed-out-users-

update query count

2e038e6

update query count

f61216b

davidsbailey mentioned this pull request Jun 17, 2025

move curriculum UI tests onto modular courses #66462

Merged

alex-m-brown merged commit 28387ac into staging Jun 18, 2025
6 checks passed

alex-m-brown deleted the TEACH-1975/Ensure-in-development-units-are-not-visible-to-signed-out-users- branch June 18, 2025 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Teach 1975/ensure in development units are not visible to signed out users #66459

Teach 1975/ensure in development units are not visible to signed out users #66459
alex-m-brown merged 14 commits into
stagingfrom
TEACH-1975/Ensure-in-development-units-are-not-visible-to-signed-out-users-

alex-m-brown commented Jun 11, 2025 •

edited

Loading

Uh oh!

davidsbailey Jun 12, 2025

Uh oh!

davidsbailey Jun 12, 2025

Uh oh!

alex-m-brown Jun 12, 2025

Uh oh!

alex-m-brown Jun 12, 2025 •

edited

Loading

Uh oh!

alex-m-brown Jun 12, 2025 •

edited

Loading

Uh oh!

davidsbailey Jun 12, 2025

Uh oh!

davidsbailey Jun 12, 2025

Uh oh!

davidsbailey Jun 12, 2025

Uh oh!

davidsbailey left a comment

Uh oh!

davidsbailey Jun 13, 2025

Uh oh!

davidsbailey Jun 13, 2025

Uh oh!

alex-m-brown Jun 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	test_user_gets_response_for(
	:index,
	user: user_type,
	response: response,
	params: {course: Pd::Workshop::COURSES.first}
	)

	test_user_gets_response_for(:show, response: :success, user: :teacher,
	params: -> {{course_course_name: @pilot_course.name, position: 1}},
	name: 'teacher without pilot access cannot view pilot unit'
	) do

Conversation

alex-m-brown commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Background

Links

Testing story

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-m-brown Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-m-brown Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidsbailey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alex-m-brown commented Jun 11, 2025 •

edited

Loading

alex-m-brown Jun 12, 2025 •

edited

Loading

alex-m-brown Jun 12, 2025 •

edited

Loading