mirror of
https://github.com/cliffe/BreakEscape.git
synced 2026-02-20 13:50:46 +00:00
Simplify NPC unlock to use standard unlock flow
PROBLEM: Previous implementation had unnecessary complexity with npcUnlockedTargets tracking. NPC unlocks should just work like any other unlock method. SOLUTION: 1. Removed npcUnlockedTargets tracking (not needed) 2. NPC unlocks now use standard unlockedRooms/unlockedObjects arrays 3. Updated validate_unlock to check if already unlocked FIRST: - If in player_state unlocked list → grant access - If method='unlocked' → verify against scenario data locked field - Otherwise → validate normally This fixes the race condition issue: - NPC calls unlock API with method='npc' - Server validates NPC has permission - Server adds to unlockedRooms (normal unlock) - Later when player opens door, client sends method='unlocked' - Server sees it's already in unlockedRooms OR unlocked in scenario → grants access Changes: - app/models/break_escape/game.rb: Remove npc_unlock_target!/npc_unlocked?, check unlocked state first - app/controllers/break_escape/games_controller.rb: Remove npc_unlock_target! calls - test/integration/unlock_system_test.rb: Update tests for simplified approach All 37 tests passing, 122 assertions
This commit is contained in:
Z. Cliffe Schreuders
@@ -241,8 +241,6 @@ module BreakEscape
|
||||
ActiveRecord::Base.transaction do
|
||||
if target_type == 'door'
|
||||
@game.unlock_room!(target_id)
|
||||
# For NPC unlocks, also track in npcUnlockedTargets for persistent state
|
||||
@game.npc_unlock_target!(target_id) if method == 'npc'
|
||||
|
||||
room_data = @game.filtered_room_data(target_id)
|
||||
|
||||
@@ -254,8 +252,6 @@ module BreakEscape
|
||||
else
|
||||
# Object/container unlock
|
||||
@game.unlock_object!(target_id)
|
||||
# For NPC unlocks, also track in npcUnlockedTargets for persistent state
|
||||
@game.npc_unlock_target!(target_id) if method == 'npc'
|
||||
|
||||
# Find the unlocked object and return its contents if it's a container
|
||||
object_data = find_object_in_scenario(target_id)
|
||||
|
||||
@@ -46,17 +46,6 @@ module BreakEscape
|
||||
player_state['unlockedObjects']&.include?(object_id)
|
||||
end
|
||||
|
||||
# NPC unlock management
|
||||
def npc_unlock_target!(target_id)
|
||||
player_state['npcUnlockedTargets'] ||= []
|
||||
player_state['npcUnlockedTargets'] << target_id unless player_state['npcUnlockedTargets'].include?(target_id)
|
||||
save!
|
||||
end
|
||||
|
||||
def npc_unlocked?(target_id)
|
||||
player_state['npcUnlockedTargets']&.include?(target_id)
|
||||
end
|
||||
|
||||
# Inventory management
|
||||
def add_inventory_item!(item)
|
||||
player_state['inventory'] ||= []
|
||||
@@ -142,23 +131,6 @@ module BreakEscape
|
||||
room = room_data(room_id)&.deep_dup
|
||||
return nil unless room
|
||||
|
||||
# Merge NPC unlock state: If NPC unlocked this room, mark it as unlocked
|
||||
if npc_unlocked?(room_id)
|
||||
Rails.logger.info "[BreakEscape] Room #{room_id} was unlocked by NPC, marking as unlocked"
|
||||
room['locked'] = false
|
||||
end
|
||||
|
||||
# Merge NPC unlock state for objects/containers in this room
|
||||
if room['objects'].present?
|
||||
room['objects'].each do |obj|
|
||||
obj_id = obj['id'] || obj['name']
|
||||
if obj_id && npc_unlocked?(obj_id)
|
||||
Rails.logger.info "[BreakEscape] Object #{obj_id} was unlocked by NPC, marking as unlocked"
|
||||
obj['locked'] = false
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
# Remove ONLY the 'requires' field (the solution) and locked 'contents'
|
||||
# Keep lockType, locked, observations visible to client
|
||||
filter_requires_and_contents_recursive(room)
|
||||
@@ -171,20 +143,24 @@ module BreakEscape
|
||||
Rails.logger.info "[BreakEscape] validate_unlock: type=#{target_type}, id=#{target_id}, attempt=#{attempt}, method=#{method}"
|
||||
|
||||
if target_type == 'door'
|
||||
room = room_data(target_id)
|
||||
return false unless room
|
||||
|
||||
# SECURITY: Only allow 'unlocked' method if door is ACTUALLY unlocked in server data
|
||||
# Client cannot be trusted - must validate against server state
|
||||
if method == 'unlocked' && !room['locked']
|
||||
Rails.logger.info "[BreakEscape] Door is unlocked in server data, granting access"
|
||||
# Check if already unlocked in player state (grants access regardless of method)
|
||||
if room_unlocked?(target_id)
|
||||
Rails.logger.info "[BreakEscape] Door already unlocked in player state, granting access"
|
||||
return true
|
||||
end
|
||||
|
||||
# SECURITY: Reject 'unlocked' method for locked doors (client bypass attempt)
|
||||
if method == 'unlocked' && room['locked']
|
||||
Rails.logger.warn "[BreakEscape] SECURITY VIOLATION: Client sent method='unlocked' for LOCKED door #{target_id}"
|
||||
return false
|
||||
room = room_data(target_id)
|
||||
return false unless room
|
||||
|
||||
# Handle method='unlocked' - verify against scenario data
|
||||
if method == 'unlocked'
|
||||
if !room['locked']
|
||||
Rails.logger.info "[BreakEscape] Door is unlocked in scenario data, granting access"
|
||||
return true
|
||||
else
|
||||
Rails.logger.warn "[BreakEscape] SECURITY VIOLATION: Client sent method='unlocked' for LOCKED door: #{target_id}"
|
||||
return false
|
||||
end
|
||||
end
|
||||
|
||||
# NPC unlock: Validate NPC has been encountered and has permission to unlock this door
|
||||
@@ -205,6 +181,12 @@ module BreakEscape
|
||||
false
|
||||
end
|
||||
else
|
||||
# Check if already unlocked in player state (grants access regardless of method)
|
||||
if object_unlocked?(target_id)
|
||||
Rails.logger.info "[BreakEscape] Object already unlocked in player state, granting access"
|
||||
return true
|
||||
end
|
||||
|
||||
# Find object in all rooms - check both id and name
|
||||
scenario_data['rooms'].each do |_room_id, room_data|
|
||||
object = room_data['objects']&.find { |obj|
|
||||
@@ -214,17 +196,15 @@ module BreakEscape
|
||||
if object
|
||||
Rails.logger.info "[BreakEscape] Found object: id=#{object['id']}, name=#{object['name']}, locked=#{object['locked']}, requires=#{object['requires']}"
|
||||
|
||||
# SECURITY: Only allow 'unlocked' method if object is ACTUALLY unlocked in server data
|
||||
# Client cannot be trusted - must validate against server state
|
||||
if method == 'unlocked' && !object['locked']
|
||||
Rails.logger.info "[BreakEscape] Object is unlocked in server data, granting access"
|
||||
return true
|
||||
end
|
||||
|
||||
# SECURITY: Reject 'unlocked' method for locked objects (client bypass attempt)
|
||||
if method == 'unlocked' && object['locked']
|
||||
Rails.logger.warn "[BreakEscape] SECURITY VIOLATION: Client sent method='unlocked' for LOCKED object #{target_id}"
|
||||
return false
|
||||
# Handle method='unlocked' - verify against scenario data
|
||||
if method == 'unlocked'
|
||||
if !object['locked']
|
||||
Rails.logger.info "[BreakEscape] Object is unlocked in scenario data, granting access"
|
||||
return true
|
||||
else
|
||||
Rails.logger.warn "[BreakEscape] SECURITY VIOLATION: Client sent method='unlocked' for LOCKED object: #{target_id}"
|
||||
return false
|
||||
end
|
||||
end
|
||||
|
||||
# NPC unlock: Validate NPC has been encountered and has permission to unlock this object
|
||||
@@ -335,7 +315,6 @@ module BreakEscape
|
||||
end
|
||||
|
||||
self.player_state['encounteredNPCs'] ||= []
|
||||
self.player_state['npcUnlockedTargets'] ||= []
|
||||
self.player_state['globalVariables'] ||= {}
|
||||
self.player_state['biometricSamples'] ||= []
|
||||
self.player_state['biometricUnlocks'] ||= []
|
||||
|
||||
File diff suppressed because it is too large
Load Diff
Binary file not shown.
@@ -832,10 +832,10 @@ module BreakEscape
|
||||
end
|
||||
|
||||
# =============================================================================
|
||||
# NPC UNLOCK PERSISTENT STATE TESTS
|
||||
# NPC UNLOCK TESTS
|
||||
# =============================================================================
|
||||
|
||||
test "NPC unlock is tracked in npcUnlockedTargets" do
|
||||
test "NPC unlock adds door to unlockedRooms" do
|
||||
# Set up NPC with unlock permission
|
||||
@game.scenario_data['rooms']['lobby']['npcs'] = [
|
||||
{
|
||||
@@ -858,15 +858,13 @@ module BreakEscape
|
||||
assert_response :success
|
||||
@game.reload
|
||||
|
||||
# Verify tracked in both unlockedRooms and npcUnlockedTargets
|
||||
# Verify tracked in unlockedRooms (normal unlock)
|
||||
assert_includes @game.player_state['unlockedRooms'], 'office_pin',
|
||||
"NPC unlock should add room to unlockedRooms"
|
||||
assert_includes @game.player_state['npcUnlockedTargets'], 'office_pin',
|
||||
"NPC unlock should track target in npcUnlockedTargets for persistent state"
|
||||
end
|
||||
|
||||
test "filtered_room_data marks NPC-unlocked door as unlocked (race condition fix)" do
|
||||
# Set up a locked room that will be unlocked by NPC
|
||||
test "already-unlocked door accepts method='unlocked'" do
|
||||
# Set up a locked room
|
||||
@game.scenario_data['rooms']['ceo'] = {
|
||||
'type' => 'office',
|
||||
'locked' => true,
|
||||
@@ -893,19 +891,19 @@ module BreakEscape
|
||||
}
|
||||
assert_response :success
|
||||
|
||||
# Now load the room (simulating loading after NPC unlock)
|
||||
get room_game_url(@game, room_id: 'ceo')
|
||||
assert_response :success
|
||||
# Later, client tries to open the already-unlocked door with method='unlocked'
|
||||
post unlock_game_url(@game), params: {
|
||||
targetType: 'door',
|
||||
targetId: 'ceo',
|
||||
attempt: nil,
|
||||
method: 'unlocked'
|
||||
}
|
||||
|
||||
json = JSON.parse(@response.body)
|
||||
room_data = json['room']
|
||||
|
||||
# The room should be marked as unlocked even though scenario data has locked: true
|
||||
assert_equal false, room_data['locked'],
|
||||
"Room should be marked unlocked when NPC has unlocked it (fixes race condition)"
|
||||
assert_response :success,
|
||||
"Already-unlocked door should accept method='unlocked' (fixes race condition)"
|
||||
end
|
||||
|
||||
test "filtered_room_data marks NPC-unlocked container as unlocked" do
|
||||
test "already-unlocked container accepts method='unlocked'" do
|
||||
# Set up a locked container
|
||||
@game.scenario_data['rooms']['lobby']['objects'] = [
|
||||
{
|
||||
@@ -937,25 +935,16 @@ module BreakEscape
|
||||
}
|
||||
assert_response :success
|
||||
|
||||
# Now load the room (simulating loading after NPC unlock)
|
||||
get room_game_url(@game, room_id: 'lobby')
|
||||
assert_response :success
|
||||
# Later, client tries to open the already-unlocked container with method='unlocked'
|
||||
post unlock_game_url(@game), params: {
|
||||
targetType: 'object',
|
||||
targetId: 'npc_safe',
|
||||
attempt: nil,
|
||||
method: 'unlocked'
|
||||
}
|
||||
|
||||
json = JSON.parse(@response.body)
|
||||
room_data = json['room']
|
||||
safe = room_data['objects'].find { |obj| obj['id'] == 'npc_safe' }
|
||||
|
||||
assert_not_nil safe, "Safe should be in room data"
|
||||
assert_equal false, safe['locked'],
|
||||
"Container should be marked unlocked when NPC has unlocked it"
|
||||
end
|
||||
|
||||
test "npcUnlockedTargets is initialized in existing game player_state" do
|
||||
# Verify that the existing game (created in setup) has npcUnlockedTargets initialized
|
||||
assert_not_nil @game.player_state['npcUnlockedTargets'],
|
||||
"npcUnlockedTargets should be initialized in existing game"
|
||||
assert @game.player_state['npcUnlockedTargets'].is_a?(Array),
|
||||
"npcUnlockedTargets should be an array"
|
||||
assert_response :success,
|
||||
"Already-unlocked container should accept method='unlocked'"
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
Reference in New Issue
Block a user