Migrate API controllers from check_for_auth_token to current_user

[scytacki]

Summary

• Add require_api_user! guard to API::APIController that returns JSON 401 for unauthenticated requests
• Migrate BookmarksController, TeacherClassesController, ExternalActivitiesController, and OfferingsController#create_for_external_activity from check_for_auth_token / auth_teacher to Devise's current_user
• Add characterization tests documenting auth behavior before and after migration
• Guest/unauthenticated requests now return 401 (previously 400 or 403 depending on controller) for controllers using require_api_user!

Behavior Changes

Controller	Old guest status	New guest status
BookmarksController	400	401
TeacherClassesController	400	401
ExternalActivitiesController	403	401
OfferingsController (other actions)	403 (unchanged)	403 (unchanged)

Test Plan

• All 120 controller specs pass (bookmarks, teacher_classes, external_activities, offerings)
• All 4 related policy specs pass
• Characterization tests verified against old code before migration
• Each controller migrated and tested individually

🤖 Generated with Claude Code

[Copilot]

Pull request overview

This PR migrates several API controllers away from check_for_auth_token to Devise’s current_user, standardizing unauthenticated handling via a new require_api_user! guard and updating specs/docs to reflect the new behavior.

Changes:

• Add require_api_user! to API::APIController and apply it to selected controllers to return JSON 401 for unauthenticated requests.
• Replace check_for_auth_token / auth_teacher usage with current_user in Bookmarks, TeacherClasses, ExternalActivities, and Offerings#create_for_external_activity.
• Update/add controller specs and add design + caller research documentation for the migration.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
rails/app/controllers/api/api_controller.rb	Introduces require_api_user! JSON 401 guard for API auth.
rails/app/controllers/api/v1/bookmarks_controller.rb	Switches bookmarks auth helper to use current_user + adds guard.
rails/app/controllers/api/v1/teacher_classes_controller.rb	Uses current_user, adds guards, and adds a teacher-only filter.
rails/app/controllers/api/v1/external_activities_controller.rb	Removes token parsing blocks and enforces login via require_api_user!.
rails/app/controllers/api/v1/offerings_controller.rb	Removes token parsing and uses current_user for teacher/admin checks.
rails/spec/controllers/api/v1/bookmarks_controller_spec.rb	Adds characterization coverage for 401 guest + 400 non-teacher behavior.
rails/spec/controllers/api/v1/teacher_clazzes_controller_spec.rb	Adds characterization for show guest/non-teacher + updates anonymous status expectations.
rails/spec/controllers/api/v1/external_activities_controller_spec.rb	Updates guest expectations to 401 for endpoints now guarded by require_api_user!.
rails/spec/controllers/api/v1/offerings_controller_spec.rb	Adds guest spec for create_for_external_activity (still 403 via Pundit).
docs/specs/2026-03-04-controller-migration-design.md	Documents the migration approach and intended status-code behavior.
docs/specs/2026-03-04-controller-migration-caller-research.md	Documents known callers and expected impact of status-code changes.
CLAUDE.md	Adds guidance about running commands via Docker instead of host.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[Copilot]

require_api_user! renders an error but doesn’t explicitly halt the filter/action. While Rails often stops processing after render, it’s safer (and clearer) to return immediately (e.g., return error(...) unless current_user) to avoid any chance of subsequent filters/actions running and causing double-render or nil-access issues.

Suggested change

	unless current_user
	error('You must be logged in to use this endpoint', 401)
	end
	end
	return error('You must be logged in to use this endpoint', 401) unless current_user
	end

[scytacki]

Both require_api_user! and require_teacher! are before_action callbacks. In Rails, calling render inside a before_action automatically halts the filter chain and prevents the action from executing — no explicit return is needed. This is standard Rails behavior (see ActionController filters docs). Adding return would be purely cosmetic.

[Copilot]

require_teacher! should explicitly halt after rendering the error (e.g., return error(...) unless ...). This avoids any possibility of the action continuing after an error response, which can lead to double-render exceptions or follow-on failures.

Suggested change

	unless current_user.portal_teacher
	error('You must be logged in as a teacher to use this endpoint')
	end
	return error('You must be logged in as a teacher to use this endpoint') unless current_user.portal_teacher

[scytacki]

Same as above — require_teacher! is a before_action, so render already halts the chain. No return needed.

[Copilot]

These specs use post for :update and :destroy, which doesn’t match the real routes/verbs (PUT/PATCH for update, DELETE for destroy). Even though controller specs can still invoke the action, using the correct HTTP verbs makes the characterization tests more faithful and reduces the chance of missing verb-specific behavior (e.g., middleware, routing constraints).

[scytacki]

These are controller specs (not request specs), so the HTTP verb is largely irrelevant — Rails invokes the action method directly regardless of the verb used. These are characterization tests verifying auth behavior, not routing. No change needed.

[Copilot]

This statement doesn’t match the behavior described in the PR summary/diffs: several endpoints now return 401 for unauthenticated requests due to require_api_user!. Suggest updating this line (and any related summary wording) to reflect the actual outcomes (e.g., 400→401 for unauthenticated, and in some cases 403 remains via Pundit).

Suggested change

	Section 6 Step 1 of the auth unification design calls for migrating four controllers from `check_for_auth_token` to `current_user`. This changes the HTTP status code for authentication failures from **400** to **403** on some actions (the `error()` helper defaults to 400; Pundit returns 403).
	Section 6 Step 1 of the auth unification design calls for migrating four controllers from `check_for_auth_token` to `require_api_user!`/`current_user`. This changes the HTTP status code for unauthenticated requests from **400** (via the `error()` helper) to **401**, while authorization failures continue to return **403** on some actions via Pundit.

[scytacki]

Fixed — updated the summary line and all "400→403" references throughout the doc to correctly say "400→401".

[Copilot]

The status-code table has an extra leading | on each row (|| ...), which can render incorrectly as an empty first column in Markdown. Consider changing each row to a single leading pipe (| ...) so it renders as a standard 3-column table.

[scytacki]

The table renders correctly — there are no double pipes. Each row has the standard | col1 | col2 | col3 | format. This appears to be a hallucination.

[dougmartin]

Looks good 👍