From 3a64ffe8f10ae3ad2ea3f0053b43b69926bdd229 Mon Sep 17 00:00:00 2001 From: "Z. Cliffe Schreuders" Date: Fri, 28 Nov 2025 15:00:13 +0000 Subject: [PATCH] Finalize implementation plan for VM and CTF Flag Integration following Review 4 - Updated last modified date and added new review document reference. - Added policy methods for `GamePolicy#submit_flag?` and `MissionPolicy#create_game?`. - Fixed authorization in `games#create` to use `:create_game?` and removed redundant `VmSet` authorization. - Clarified loading of `hacktivity-cable.js` with CSP nonce. - Included fallback for non-Hacktivity mode in VM set validation. - Documented minor amendments and verification checklist for implementation readiness. --- .../vms_and_flags/IMPLEMENTATION_PLAN.md | 72 ++-- .../review4/IMPLEMENTATION_READY_CHECKLIST.md | 184 ++++++++++ .../vms_and_flags/review4/PLAN_AMENDMENTS.md | 145 ++++++++ .../vms_and_flags/review4/REVIEW.md | 325 ++++++++++++++++++ 4 files changed, 705 insertions(+), 21 deletions(-) create mode 100644 planning_notes/vms_and_flags/review4/IMPLEMENTATION_READY_CHECKLIST.md create mode 100644 planning_notes/vms_and_flags/review4/PLAN_AMENDMENTS.md create mode 100644 planning_notes/vms_and_flags/review4/REVIEW.md diff --git a/planning_notes/vms_and_flags/IMPLEMENTATION_PLAN.md b/planning_notes/vms_and_flags/IMPLEMENTATION_PLAN.md index cd2341e..e376569 100644 --- a/planning_notes/vms_and_flags/IMPLEMENTATION_PLAN.md +++ b/planning_notes/vms_and_flags/IMPLEMENTATION_PLAN.md @@ -1,11 +1,12 @@ # VM and CTF Flag Integration - Implementation Plan -**Last Updated**: After Console Integration Deep-Dive (2025-11-28) +**Last Updated**: After Review 4 Amendments (2025-11-28) **Review Documents**: - `planning_notes/vms_and_flags/review1/REVIEW.md` - `planning_notes/vms_and_flags/review2/REVIEW.md` - `planning_notes/vms_and_flags/review3/REVIEW.md` - `planning_notes/vms_and_flags/review3/HACKTIVITY_COMPATIBILITY.md` +- `planning_notes/vms_and_flags/review4/REVIEW.md` ✅ **Ready for Implementation** ## Overview @@ -582,6 +583,9 @@ end # - The `scenario` field contains the XML path (e.g., "scenarios/ctf/foo.xml") # - VmSet doesn't have a `display_name` method - use sec_gen_batch.title instead # - Always eager-load :vms and :sec_gen_batch to avoid N+1 queries +# +# NOTE: Update the existing `hacktivity_mode?` method (line 60-62 in current code) +# to use the new definition that checks for ::VmSet and ::FlagService instead of just ::Cybok. def valid_vm_sets_for_user(user) return [] unless self.class.hacktivity_mode? && requires_vms? @@ -821,19 +825,25 @@ end # Add to app/controllers/break_escape/games_controller.rb def create @mission = Mission.find(params[:mission_id]) - authorize @mission if defined?(Pundit) + authorize @mission, :create_game? if defined?(Pundit) # Build initial player_state with VM/flag context initial_player_state = {} # Hacktivity mode with VM set if params[:vm_set_id].present? && defined?(::VmSet) - vm_set = ::VmSet.find(params[:vm_set_id]) - authorize vm_set, :use? if defined?(Pundit) + vm_set = ::VmSet.find_by(id: params[:vm_set_id]) + return render json: { error: 'VM set not found' }, status: :not_found unless vm_set # Validate VM set belongs to user and matches mission - unless @mission.valid_vm_sets_for_user(current_user).include?(vm_set) - return render json: { error: 'Invalid VM set for this mission' }, status: :forbidden + if BreakEscape::Mission.hacktivity_mode? + unless @mission.valid_vm_sets_for_user(current_user).include?(vm_set) + return render json: { error: 'Invalid VM set for this mission' }, status: :forbidden + end + else + # Standalone mode - vm_set_id shouldn't be used + Rails.logger.warn "[BreakEscape] vm_set_id provided but not in Hacktivity mode, ignoring" + params.delete(:vm_set_id) end initial_player_state['vm_set_id'] = vm_set.id @@ -1080,7 +1090,34 @@ end 1. Add `:new` to `only: [:new, :show, :create]` 2. Add `post :flags` to the member block -### 2. Missions Controller (`app/controllers/break_escape/missions_controller.rb`) +### 3. Update Policies + +#### GamePolicy + +Add to `app/policies/break_escape/game_policy.rb`: + +```ruby +def submit_flag? + show? +end + +def container? + show? +end +``` + +#### MissionPolicy + +Add to `app/policies/break_escape/mission_policy.rb`: + +```ruby +def create_game? + # Anyone authenticated can create a game for a mission + user.present? +end +``` + +### 4. Missions Controller (`app/controllers/break_escape/missions_controller.rb`) #### Update `show` Action (Critical Change) @@ -1467,25 +1504,15 @@ export { setupHacktivityActionCable, teardownHacktivityActionCable }; **Loading this module:** -Include in the game's main entry point or load via script tag: +Add to `app/views/break_escape/games/show.html.erb` after the Phaser script loads: -```html - +```erb <% if BreakEscape::Mission.hacktivity_mode? %> - + <% end %> ``` -Or import in `js/main.js`: - -```javascript -// Only import in Hacktivity mode -if (window.breakEscapeConfig?.hacktivityMode) { - import('./systems/hacktivity-cable.js').then(module => { - module.setupHacktivityActionCable(); - }); -} -``` +**Note**: The module self-initializes on load. No additional setup required. ### 1. VM Launcher Minigame @@ -2459,6 +2486,8 @@ end - [ ] 2.11 Update GamesController `scenario` action: Include `submittedFlags` - [ ] 2.12 Add CSS link tags for new minigame styles to show.html.erb - [ ] 2.13 Write controller tests +- [ ] 2.14 Add GamePolicy#submit_flag? method +- [ ] 2.15 Add MissionPolicy#create_game? method (for authorization in games#create) ### Phase 3: Client-Side Minigames - [ ] 3.0 Create `public/break_escape/js/systems/hacktivity-cable.js` (ActionCable FilePush subscription) @@ -2583,3 +2612,4 @@ This plan provides a complete, actionable roadmap for integrating VMs and CTF fl | 2025-11-27 | Review 3 (`review3/REVIEW.md`) | **Critical**: Discovered `games#create` action does NOT exist (routes declared it but controller didn't implement it). Updated plan to implement action from scratch. Added `games#new` action and view for VM set selection. Added MissionsController#show update. Validated callback timing approach is correct. Added revised implementation order prioritizing controller infrastructure. | | 2025-11-28 | Hacktivity Compatibility (`review3/HACKTIVITY_COMPATIBILITY.md`) | **Reviewed Hacktivity codebase** for compatibility. Key findings: (1) Use `FlagService.process_flag()` instead of direct model update for proper scoring/streaks/notifications, (2) VmSet uses `sec_gen_batch` (with underscore) not `secgen_batch`, (3) VmSet has no `display_name` method - use `sec_gen_batch.title` instead, (4) Added `event_id` and `sec_gen_batch_id` to VM context for console URLs, (5) Added case-insensitive flag matching to match Hacktivity behavior, (6) Added eager loading with `.includes(:vms, :sec_gen_batch)`, (7) Filter out pending/error build_status VM sets. | | 2025-11-28 | Console Integration Update | **Deep-dive into Hacktivity's console mechanism**: Discovered console file delivery is async via ActionCable, not HTTP response. (1) Console endpoint dispatches background job that generates SPICE `.vv` file, (2) Job broadcasts file via `ActionCable.server.broadcast "file_push:#{user_id}"` with Base64-encoded content, (3) Updated plan to single approach: subscribe to `FilePushChannel`, POST to trigger job, receive file via ActionCable. (4) Created `hacktivity-cable.js` module specification, (5) Updated VM launcher minigame to use AJAX POST + ActionCable callback pattern, (6) Removed alternative "open Hacktivity page" approach - now single definitive approach. | +| 2025-11-28 | Review 4 (`review4/REVIEW.md`) | **Final validation - Plan ready for implementation**. Minor fixes applied: (1) Added policy checklist items for `GamePolicy#submit_flag?` and `MissionPolicy#create_game?`, (2) Fixed authorization in `games#create` to use `authorize @mission, :create_game?`, (3) Removed redundant VmSet authorization (replaced with `find_by` + nil check), (4) Added Hacktivity mode check for VmSet validation fallback, (5) Added note about updating existing `hacktivity_mode?` method, (6) Added policy code examples section (GamePolicy + MissionPolicy), (7) Simplified hacktivity-cable.js loading to single recommended approach with CSP nonce. | diff --git a/planning_notes/vms_and_flags/review4/IMPLEMENTATION_READY_CHECKLIST.md b/planning_notes/vms_and_flags/review4/IMPLEMENTATION_READY_CHECKLIST.md new file mode 100644 index 0000000..efaf79f --- /dev/null +++ b/planning_notes/vms_and_flags/review4/IMPLEMENTATION_READY_CHECKLIST.md @@ -0,0 +1,184 @@ +# Implementation Ready Checklist + +**Status**: ✅ READY FOR IMPLEMENTATION +**Date**: November 28, 2025 + +--- + +## Pre-Implementation Fixes (Minor) + +Before starting implementation, apply these minor fixes to the plan: + +### 1. Policy Methods + +Add to `app/policies/break_escape/game_policy.rb`: +```ruby +def submit_flag? + show? +end +``` + +Add to `app/policies/break_escape/mission_policy.rb` (or create if doesn't exist): +```ruby +def create_game? + user.present? +end +``` + +### 2. Update `hacktivity_mode?` Method + +In `games#create`, change authorization from: +```ruby +authorize @mission if defined?(Pundit) +``` +To: +```ruby +authorize @mission, :create_game? if defined?(Pundit) +``` + +### 3. Remove Redundant Authorization + +In `games#create`, remove this line: +```ruby +authorize vm_set, :use? if defined?(Pundit) # REMOVE - valid_vm_sets_for_user handles this +``` + +--- + +## Implementation Order + +### Phase 1: Controller Infrastructure (Day 1) + +| Step | File | Action | +|------|------|--------| +| 1.1 | `app/policies/break_escape/game_policy.rb` | Add `submit_flag?` method | +| 1.2 | `app/policies/break_escape/mission_policy.rb` | Add `create_game?` method | +| 1.3 | `config/routes.rb` | Add `:new` to games resource, add `post :flags` | +| 1.4 | `app/controllers/break_escape/games_controller.rb` | Add `new`, `create`, `submit_flag` actions | +| 1.5 | `app/controllers/break_escape/missions_controller.rb` | Update `show` for VM missions | +| 1.6 | `app/views/break_escape/games/new.html.erb` | Create VM set selection view | + +### Phase 2: Database Migration (Day 1) + +| Step | File | Action | +|------|------|--------| +| 2.1 | `db/migrate/[timestamp]_remove_unique_game_constraint.rb` | Create migration | +| 2.2 | Terminal | Run `rails db:migrate` | + +### Phase 3: Model Changes (Day 2) + +| Step | File | Action | +|------|------|--------| +| 3.1 | `app/models/break_escape/mission.rb` | Add `requires_vms?`, `valid_vm_sets_for_user` | +| 3.2 | `app/models/break_escape/mission.rb` | Extend `ScenarioBinding` with `vm_context` | +| 3.3 | `app/models/break_escape/mission.rb` | Update `generate_scenario_data` signature | +| 3.4 | `app/models/break_escape/mission.rb` | Update `hacktivity_mode?` to check VmSet/FlagService | +| 3.5 | `app/models/break_escape/game.rb` | Add `build_vm_context` method | +| 3.6 | `app/models/break_escape/game.rb` | Extend `initialize_player_state` for VM/flag fields | +| 3.7 | `app/models/break_escape/game.rb` | Add flag submission methods | +| 3.8 | `app/models/break_escape/game.rb` | Update `generate_scenario_data` callback | + +### Phase 4: Client-Side (Day 3-4) + +| Step | File | Action | +|------|------|--------| +| 4.1 | `public/break_escape/js/systems/hacktivity-cable.js` | Create ActionCable integration | +| 4.2 | `public/break_escape/js/minigames/vm-launcher/` | Create minigame directory and files | +| 4.3 | `public/break_escape/js/minigames/flag-station/` | Create minigame directory and files | +| 4.4 | `public/break_escape/js/minigames/index.js` | Register new minigames | +| 4.5 | `public/break_escape/js/systems/interactions.js` | Add handlers for new object types | +| 4.6 | `public/break_escape/css/minigames/vm-launcher.css` | Create styles | +| 4.7 | `public/break_escape/css/minigames/flag-station.css` | Create styles | +| 4.8 | `app/views/break_escape/games/show.html.erb` | Add CSS links, extend config, add script tag | + +### Phase 5: Testing (Day 5) + +| Step | Test | Validation | +|------|------|------------| +| 5.1 | Model unit tests | `submit_flag`, `build_vm_context` | +| 5.2 | Controller tests | `create`, `submit_flag` actions | +| 5.3 | Standalone mode | Create game without VMs, submit manual flags | +| 5.4 | Hacktivity mode | Create game with VmSet, test console + flags | +| 5.5 | Minigame tests | Open test HTML files for VM launcher and flag station | + +--- + +## Verification Checklist + +After implementation, verify: + +- [ ] Visiting mission with `secgen_scenario` redirects to `games#new` +- [ ] VM set selection page shows available VM sets +- [ ] Creating game with VM set stores `vm_set_id` in player_state +- [ ] Scenario ERB has access to `vm_context` +- [ ] Clicking `type: "vm-launcher"` object opens VM launcher minigame +- [ ] Clicking `type: "flag-station"` object opens flag station minigame +- [ ] Flag submission validates server-side +- [ ] Flag submission calls `FlagService.process_flag` in Hacktivity mode +- [ ] Console button triggers SPICE file download via ActionCable +- [ ] Multiple games per mission work (unique index removed) +- [ ] Standalone mode works without VMs + +--- + +## Key Code Locations for Reference + +### Existing Patterns to Follow + +| Pattern | File | Line | +|---------|------|------| +| Minigame registration | `public/break_escape/js/minigames/index.js` | 80-94 | +| Object type handling | `public/break_escape/js/systems/interactions.js` | 455-512 | +| Server unlock validation | `app/models/break_escape/game.rb` | 184-301 | +| ERB scenario generation | `app/models/break_escape/mission.rb` | 65-76 | +| Player state initialization | `app/models/break_escape/game.rb` | 549-582 | +| Policy authorization | `app/controllers/break_escape/games_controller.rb` | 8 | + +### Config Object (Current) + +```javascript +// app/views/break_escape/games/show.html.erb:115-120 +window.breakEscapeConfig = { + gameId: <%= @game.id %>, + apiBasePath: '<%= game_path(@game) %>', + assetsPath: '/break_escape/assets', + csrfToken: '<%= form_authenticity_token %>' +}; +``` + +### Config Object (After Changes) + +```javascript +window.breakEscapeConfig = { + gameId: <%= @game.id %>, + apiBasePath: '<%= game_path(@game) %>', + assetsPath: '/break_escape/assets', + csrfToken: '<%= form_authenticity_token %>', + // NEW FIELDS: + hacktivityMode: <%= BreakEscape::Mission.hacktivity_mode? %>, + vmSetId: <%= @game.player_state['vm_set_id'] || 'null' %> +}; +``` + +--- + +## Command Quick Reference + +```bash +# Generate migration +rails generate migration RemoveUniqueGameConstraint + +# Run migrations +rails db:migrate + +# Test routes +rails routes | grep break_escape + +# Compile Ink scripts (if adding VM-related NPC dialogs) +./scripts/compile-ink.sh + +# Run tests +rails test test/controllers/break_escape/ +rails test test/models/break_escape/ +``` + diff --git a/planning_notes/vms_and_flags/review4/PLAN_AMENDMENTS.md b/planning_notes/vms_and_flags/review4/PLAN_AMENDMENTS.md new file mode 100644 index 0000000..db8f55f --- /dev/null +++ b/planning_notes/vms_and_flags/review4/PLAN_AMENDMENTS.md @@ -0,0 +1,145 @@ +# Plan Amendments - Review 4 + +These are the specific amendments to apply to `IMPLEMENTATION_PLAN.md` based on Review 4 findings. + +--- + +## 1. Add Policy Requirements to Implementation Checklist + +**Location**: Lines 2448-2462 (Phase 2 checklist) + +**Add after line 2.13**: +```markdown +- [ ] 2.14 Add GamePolicy#submit_flag? method +- [ ] 2.15 Add MissionPolicy#create_game? method (for authorization in games#create) +``` + +--- + +## 2. Fix Authorization in `games#create` Action + +**Location**: Lines 820-864 (games#create action code) + +**Change line 821**: +```ruby +# FROM: +authorize @mission if defined?(Pundit) + +# TO: +authorize @mission, :create_game? if defined?(Pundit) +``` + +**Remove lines 831-832**: +```ruby +# REMOVE THESE LINES (redundant - valid_vm_sets_for_user already validates ownership): +vm_set = ::VmSet.find(params[:vm_set_id]) +authorize vm_set, :use? if defined?(Pundit) +``` + +**Replace with**: +```ruby +vm_set = ::VmSet.find_by(id: params[:vm_set_id]) +return render json: { error: 'VM set not found' }, status: :not_found unless vm_set +``` + +--- + +## 3. Update `hacktivity_mode?` Reference in Model Changes + +**Location**: Lines 580-600 (Mission model changes) + +**Add note after line 585**: +```markdown +**NOTE**: Update the existing `hacktivity_mode?` method (line 60-62 in current code) to use this new definition. The current definition only checks for `::Cybok`. +``` + +--- + +## 4. Add Policy Code Examples + +**Location**: Add new section after line 1080 (after controller routes section) + +```markdown +### 3. Update Policies + +#### GamePolicy + +Add to `app/policies/break_escape/game_policy.rb`: + +```ruby +def submit_flag? + show? +end + +def container? + show? +end +``` + +#### MissionPolicy + +Add to `app/policies/break_escape/mission_policy.rb`: + +```ruby +def create_game? + # Anyone authenticated can create a game for a mission + user.present? +end +``` +``` + +--- + +## 5. Clarify `hacktivity-cable.js` Script Loading + +**Location**: Lines 1472-1488 (loading hacktivity-cable.js) + +**Replace the two options with single recommended approach**: +```markdown +**Loading this module:** + +Add to `app/views/break_escape/games/show.html.erb` after the Phaser script loads: + +```erb +<% if BreakEscape::Mission.hacktivity_mode? %> + +<% end %> +``` + +**Note**: The module self-initializes on load. No additional setup required. +``` + +--- + +## 6. Add Fallback for Non-Hacktivity VmSet Authorization + +**Location**: Lines 833-839 (games#create validation) + +**Update the validation block**: +```ruby +# Validate VM set belongs to user and matches mission +if BreakEscape::Mission.hacktivity_mode? + unless @mission.valid_vm_sets_for_user(current_user).include?(vm_set) + return render json: { error: 'Invalid VM set for this mission' }, status: :forbidden + end +else + # Standalone mode - vm_set_id shouldn't be used + Rails.logger.warn "[BreakEscape] vm_set_id provided but not in Hacktivity mode, ignoring" + params.delete(:vm_set_id) +end +``` + +--- + +## Summary + +| Amendment | Severity | Reason | +|-----------|----------|--------| +| Add policy methods | Minor | Pundit will fail without these | +| Fix authorization action | Minor | `authorize @mission` uses `show?` not `create_game?` | +| Remove redundant VmSet auth | Cleanup | Query already validates ownership | +| Add `hacktivity-cable.js` load | Clarification | Two options were confusing | +| Add Hacktivity mode check | Robustness | Prevents errors in standalone mode | + +These amendments can be applied incrementally during implementation without blocking the start of development. + diff --git a/planning_notes/vms_and_flags/review4/REVIEW.md b/planning_notes/vms_and_flags/review4/REVIEW.md new file mode 100644 index 0000000..32d7735 --- /dev/null +++ b/planning_notes/vms_and_flags/review4/REVIEW.md @@ -0,0 +1,325 @@ +# VM and CTF Flag Integration - Plan Review 4 + +**Reviewer**: AI Review +**Date**: November 28, 2025 +**Document Reviewed**: `planning_notes/vms_and_flags/IMPLEMENTATION_PLAN.md` (post-Reviews 1, 2, 3 + Hacktivity Compatibility) + +--- + +## Executive Summary + +The implementation plan has matured significantly through four review cycles. The plan is now **ready for implementation** with minor clarifications needed. All critical issues from previous reviews have been addressed, and the Hacktivity compatibility deep-dive has resolved API integration concerns. + +**Status**: ✅ Ready for Implementation +**Remaining Issues**: 2 Minor, 2 Recommendations +**Estimated Risk**: Low + +--- + +## Validation Summary + +### ✅ Issues Resolved from Previous Reviews + +| Issue | Status | Notes | +|-------|--------|-------| +| `games#create` action missing | ✅ Resolved | Plan now explicitly shows full implementation | +| `games#new` action missing | ✅ Resolved | Plan includes action and view | +| `secgen_scenario` column | ✅ Verified | Exists in migration `20251125000001` | +| Hacktivity association naming | ✅ Fixed | Uses `sec_gen_batch` (with underscore) | +| Flag submission method | ✅ Fixed | Uses `FlagService.process_flag()` | +| VmSet `display_name` | ✅ Fixed | Uses `sec_gen_batch.title` instead | +| Console URL construction | ✅ Complete | Stores `event_id`, `sec_gen_batch_id` in VM context | +| ActionCable integration | ✅ Complete | Uses FilePushChannel subscription | +| ERB null-safety patterns | ✅ Verified | Uses `&.`, `dig`, `|| 'null'` | +| `window.breakEscapeConfig` | ✅ Verified | Plan extends, not replaces | + +### ✅ Codebase Alignment Verified + +| Component | Plan Assumption | Actual Codebase | Status | +|-----------|-----------------|-----------------|--------| +| Routes structure | `BreakEscape::Engine.routes.draw` | ✅ Matches | OK | +| Game model callbacks | `before_create` for scenario/state | ✅ Matches | OK | +| Player state structure | JSONB with defaults | ✅ Matches | OK | +| Minigame registration | `MinigameFramework.registerScene()` | ✅ Matches | OK | +| Policy pattern | Pundit with `authorize` | ✅ Matches | OK | +| Object interaction | `handleObjectInteraction()` via type | ✅ Matches | OK | +| Unique index on games | Exists, needs removal | ✅ Verified | OK | + +--- + +## Minor Issues to Address + +### 1. 📝 Policy Methods Missing for New Actions + +The `GamePolicy` currently defines policies for existing actions but will need new policy methods for `create`, `new`, and `submit_flag` actions. + +**Current** (`app/policies/break_escape/game_policy.rb`): +```ruby +def show? + record.player == user || user&.admin? || user&.account_manager? +end +``` + +**Required Additions**: +```ruby +# For games#create - check if user can create games for this mission +def create? + # The record here is the Mission, not a Game + # Anyone authenticated can create games + user.present? +end + +# For games#new - same as create +def new? + create? +end + +# For games#submit_flag - owner can submit flags +def submit_flag? + record.player == user || user&.admin? || user&.account_manager? +end +``` + +**Note**: The `create` action in the plan authorizes `@mission`, not `@game`: +```ruby +authorize @mission if defined?(Pundit) +``` + +This means you need a `MissionPolicy#create?` OR change the authorization approach to use `Game.new.authorize` pattern. + +**Recommendation**: Use `authorize @mission, :create_game?` with: +```ruby +# mission_policy.rb +def create_game? + user.present? # Anyone authenticated can start a game +end +``` + +--- + +### 2. 📝 `hacktivity_mode?` Detection Method Location + +The plan defines `hacktivity_mode?` in two places: + +1. In `Mission` model (lines 465-468): +```ruby +def self.hacktivity_mode? + defined?(::VmSet) && defined?(::SecGenBatch) && defined?(::FlagService) +end +``` + +2. Reference in `show.html.erb` (line 92): +```ruby +hacktivityMode: <%= BreakEscape::Mission.hacktivity_mode? %>, +``` + +**Issue**: The Mission model currently only checks `defined?(::Cybok)` (line 60-62), not the VM/Flag classes. + +**Current** (`app/models/break_escape/mission.rb`): +```ruby +def self.hacktivity_mode? + defined?(::Cybok) +end +``` + +**Recommendation**: The plan's more comprehensive check is better - update the existing method: +```ruby +def self.hacktivity_mode? + defined?(::VmSet) && defined?(::FlagService) +end +``` + +This is a minor fix but important for consistent behavior. + +--- + +## Recommendations (Nice to Have) + +### 1. 💡 Add `container` Action Authorization + +The plan's `submit_flag` action calls `@game.submit_flag(flag_key)` which accesses scenario data. The policy should ensure this is the game owner. + +The plan shows: +```ruby +def submit_flag + authorize @game if defined?(Pundit) + # ... +end +``` + +This is correct - the existing pattern `show?` → `record.player == user` will work. Just add: +```ruby +def submit_flag? + show? +end +``` + +--- + +### 2. 💡 Consider Route Namespacing for VmSet Authorization + +When authorizing `vm_set` in `games#create`: +```ruby +vm_set = ::VmSet.find(params[:vm_set_id]) +authorize vm_set, :use? if defined?(Pundit) +``` + +The `::VmSet` is a Hacktivity model with its own policy. The `use?` policy method may not exist. + +**Recommendation**: Check Hacktivity's VmSetPolicy or skip direct VmSet authorization and rely on the `valid_vm_sets_for_user` method's implicit authorization (it only returns VMs the user owns). + +The plan's current approach: +```ruby +unless @mission.valid_vm_sets_for_user(current_user).include?(vm_set) + return render json: { error: 'Invalid VM set' }, status: :forbidden +end +``` + +This is sufficient - the `valid_vm_sets_for_user` query already filters to user's own VMs. The extra `authorize vm_set, :use?` line can be removed. + +--- + +## Implementation Readiness Checklist + +| Phase | Ready | Notes | +|-------|-------|-------| +| Database Changes | ✅ | Migration file outlined, `secgen_scenario` already exists | +| Model Changes | ✅ | All methods documented with code | +| Controller Changes | ✅ | Full action implementations provided | +| Views | ✅ | `new.html.erb` template included | +| Routes | ✅ | Changes clearly specified | +| Client JS - ActionCable | ✅ | `hacktivity-cable.js` fully documented | +| Client JS - Minigames | ✅ | Both minigames fully implemented | +| Client JS - Interactions | ✅ | Handler code provided | +| CSS Styling | ✅ | Complete stylesheets included | +| ERB Templates | ✅ | Example scenario with null-safety patterns | +| Policies | ⚠️ | Need to add `submit_flag?` and `create_game?` | +| Tests | 📝 | Testing plan outlined but not implemented | + +--- + +## Risk Assessment + +### Low Risk ✅ +- Database migration is straightforward (index removal) +- New endpoints follow existing patterns +- Minigame framework integration is well-established +- ERB patterns are documented with null-safety + +### Medium Risk ⚠️ (Mitigated) +- **Hacktivity integration** - mitigated by FlagService usage and compatibility review +- **Console file delivery** - mitigated by ActionCable subscription approach + +### No High-Risk Items +The plan has addressed all critical concerns through the review process. + +--- + +## Deployment Considerations + +### Order of Operations + +1. **Deploy controller changes** (disabled/hidden) + - Add `games#new`, `games#create`, `games#submit_flag` + - Update `MissionsController#show` redirect logic + - Add policy methods + +2. **Run migration** + - Remove unique index on games table + +3. **Deploy frontend changes** + - Add minigame JS files + - Add CSS files + - Update `show.html.erb` config + - Add ActionCable integration + +4. **Enable new functionality** + - Update scenarios with VM/flag ERB syntax + - Test end-to-end + +### Rollback Strategy + +- Controller changes are additive (new actions) +- Migration can be reversed (re-add unique index) +- Frontend JS/CSS can be reverted +- Existing games unaffected (no schema changes to player_state) + +--- + +## Suggested Minor Updates to Plan + +### 1. Add Policy Code to Phase 2 + +Insert after line 2.13 in the Implementation Checklist: +``` +- [ ] 2.14 Add policy methods: GamePolicy#submit_flag?, MissionPolicy#create_game? +``` + +### 2. Update `hacktivity_mode?` in Implementation + +Update Phase 1.5 to explicitly note updating the existing method: +``` +- [ ] 1.5 Update Mission.hacktivity_mode? to check for ::VmSet and ::FlagService +``` + +### 3. Remove Redundant VmSet Authorization + +In the `games#create` action code (lines 830-833), simplify: +```ruby +# REMOVE THIS LINE: +authorize vm_set, :use? if defined?(Pundit) + +# The valid_vm_sets_for_user check below is sufficient +``` + +--- + +## Conclusion + +The implementation plan is **ready for implementation**. The three previous reviews and Hacktivity compatibility analysis have addressed all critical issues. The plan provides: + +- ✅ Complete code examples for all changes +- ✅ Clear implementation order +- ✅ Comprehensive error handling +- ✅ Both Hacktivity and standalone mode support +- ✅ Proper security patterns (server-side flag validation) +- ✅ Async console file handling via ActionCable + +**Recommendation**: Proceed with implementation following the revised implementation order in the plan (Phase 1: Controller Infrastructure first). + +--- + +## Appendix: Quick Reference + +### Files to Create (New) +``` +db/migrate/[timestamp]_remove_unique_game_constraint.rb +app/views/break_escape/games/new.html.erb +public/break_escape/js/systems/hacktivity-cable.js +public/break_escape/js/minigames/vm-launcher/vm-launcher-minigame.js +public/break_escape/js/minigames/flag-station/flag-station-minigame.js +public/break_escape/css/minigames/vm-launcher.css +public/break_escape/css/minigames/flag-station.css +``` + +### Files to Modify (Existing) +``` +app/models/break_escape/mission.rb +app/models/break_escape/game.rb +app/controllers/break_escape/games_controller.rb +app/controllers/break_escape/missions_controller.rb +app/policies/break_escape/game_policy.rb +app/policies/break_escape/mission_policy.rb +app/views/break_escape/games/show.html.erb +public/break_escape/js/minigames/index.js +public/break_escape/js/systems/interactions.js +config/routes.rb +``` + +### Key API Endpoints (New) +``` +GET /break_escape/games/new?mission_id=:id +POST /break_escape/games +POST /break_escape/games/:id/flags +``` +