diff --git a/otdfctl/e2e/migrate-namespaced-policy.bats b/otdfctl/e2e/migrate-namespaced-policy.bats index 71753cd06d..39ee38e6ed 100644 --- a/otdfctl/e2e/migrate-namespaced-policy.bats +++ b/otdfctl/e2e/migrate-namespaced-policy.bats @@ -9,44 +9,79 @@ # can pollute these migration assertions. # CI should run this tag in a separate invocation, then run the remaining suite # with this tag filtered out. +# +# Paths intentionally covered here: +# - action-scope migration creates only namespaced action targets, including +# custom-action fanout across namespaces from RR and trigger anchors +# - standard read action resolves to the canonical namespaced target where +# downstream migrations depend on it +# - SCS migration handles single-namespace placement, cross-namespace fanout, +# and reuse of an already-existing canonical target +# - subject-mapping migration rewrites action and SCS dependencies correctly +# and can reuse an already-existing canonical target +# - registered-resource migration rewrites action bindings and reuses canonical +# targets when they already exist +# - obligation-trigger migration rewrites action dependencies and reuses +# canonical targets when they already exist +# - combined all-scope migration creates one target per supported object type +# - every covered scope verifies idempotent reruns and that legacy source +# objects remain in place after migration +# +# Paths that are not in these e2e tests: +# - planner-only or dry-run output, summary formatting, and status bucket +# assertions such as create/already_migrated/existing_standard/unresolved +# - unresolved or interactive-review flows, especially conflicting registered +# resources and manual namespace selection +# - unused legacy objects being skipped entirely by planning and execution +# - subject mappings with multiple actions +# - RRs with multiple action-attribute values load "${BATS_LIB_PATH}/bats-support/load.bash" load "${BATS_LIB_PATH}/bats-assert/load.bash" run_otdfctl_migrate() { run ./otdfctl --host http://localhost:8080 --with-client-creds-file ./creds.json migrate "$@" + assert_success } run_otdfctl_action() { run ./otdfctl --host http://localhost:8080 --with-client-creds-file ./creds.json policy actions "$@" + assert_success } run_otdfctl_sm() { run ./otdfctl --host http://localhost:8080 --with-client-creds-file ./creds.json policy subject-mappings "$@" + assert_success } run_otdfctl_scs() { run ./otdfctl --host http://localhost:8080 --with-client-creds-file ./creds.json policy scs "$@" + assert_success } run_otdfctl_registered_resources() { run ./otdfctl --host http://localhost:8080 --with-client-creds-file ./creds.json policy registered-resources "$@" + assert_success } run_otdfctl_registered_resource_values() { run ./otdfctl --host http://localhost:8080 --with-client-creds-file ./creds.json policy registered-resources values "$@" + assert_success } run_otdfctl_obligations() { run ./otdfctl --host http://localhost:8080 --with-client-creds-file ./creds.json policy obligations "$@" + assert_success } run_otdfctl_obligation_values() { run ./otdfctl --host http://localhost:8080 --with-client-creds-file ./creds.json policy obligations values "$@" + assert_success } run_otdfctl_obligation_triggers() { run ./otdfctl --host http://localhost:8080 --with-client-creds-file ./creds.json policy obligations triggers "$@" + assert_success } sql_escape_literal() { @@ -190,6 +225,25 @@ create_legacy_subject_mapping() { printf -v "$result_var" '%s' "$created_subject_mapping_id" } +create_namespaced_subject_mapping() { + local result_var="$1" + local namespace_id="$2" + local attribute_value_id="$3" + local action_id="$4" + local subject_condition_set_id="$5" + shift 5 + + run ./otdfctl --host http://localhost:8080 --with-client-creds-file ./creds.json policy subject-mappings create --namespace "$namespace_id" --attribute-value-id "$attribute_value_id" --action "$action_id" --subject-condition-set-id "$subject_condition_set_id" "$@" --json + assert_success + + local created_subject_mapping_id + created_subject_mapping_id=$(echo "$output" | jq -r '.id // empty') + assert_not_equal "$created_subject_mapping_id" "" + + track_subject_mapping_id "$created_subject_mapping_id" + printf -v "$result_var" '%s' "$created_subject_mapping_id" +} + create_global_registered_resource() { local result_var="$1" local resource_name="$2" @@ -324,11 +378,12 @@ lookup_namespaced_action_id() { local action_name="$2" local namespace_id="$3" - run sh -c "./otdfctl $HOST $WITH_CREDS policy actions get --name \"$action_name\" --namespace \"$namespace_id\" --json" - assert_success + local looked_up_action_json + run_otdfctl_action get --name "$action_name" --namespace "$namespace_id" --json + looked_up_action_json="$output" local looked_up_action_id - looked_up_action_id=$(echo "$output" | jq -r '.id // empty') + looked_up_action_id=$(echo "$looked_up_action_json" | jq -r '.id // empty') assert_not_equal "$looked_up_action_id" "" printf -v "$result_var" '%s' "$looked_up_action_id" @@ -341,26 +396,31 @@ namespace_state_json() { local scs_total local registered_resources_total local obligation_triggers_total + local actions_json + local subject_mappings_json + local scs_json + local registered_resources_json + local obligation_triggers_json - run ./otdfctl $HOST $WITH_CREDS policy actions list --namespace "$namespace_filter" --limit 1 --offset 0 --json - assert_success - actions_total=$(echo "$output" | jq -r '.pagination.total // 0') + run_otdfctl_action list --namespace "$namespace_filter" --limit 1 --offset 0 --json + actions_json="$output" + actions_total=$(echo "$actions_json" | jq -r '.pagination.total // 0') - run ./otdfctl $HOST $WITH_CREDS policy subject-mappings list --namespace "$namespace_filter" --limit 1 --offset 0 --json - assert_success - subject_mappings_total=$(echo "$output" | jq -r '.pagination.total // 0') + run_otdfctl_sm list --namespace "$namespace_filter" --limit 1 --offset 0 --json + subject_mappings_json="$output" + subject_mappings_total=$(echo "$subject_mappings_json" | jq -r '.pagination.total // 0') - run ./otdfctl $HOST $WITH_CREDS policy scs list --namespace "$namespace_filter" --limit 1 --offset 0 --json - assert_success - scs_total=$(echo "$output" | jq -r '.pagination.total // 0') + run_otdfctl_scs list --namespace "$namespace_filter" --limit 1 --offset 0 --json + scs_json="$output" + scs_total=$(echo "$scs_json" | jq -r '.pagination.total // 0') - run ./otdfctl $HOST $WITH_CREDS policy registered-resources list --namespace "$namespace_filter" --limit 1 --offset 0 --json - assert_success - registered_resources_total=$(echo "$output" | jq -r '.pagination.total // 0') + run_otdfctl_registered_resources list --namespace "$namespace_filter" --limit 1 --offset 0 --json + registered_resources_json="$output" + registered_resources_total=$(echo "$registered_resources_json" | jq -r '.pagination.total // 0') - run ./otdfctl $HOST $WITH_CREDS policy obligations triggers list --namespace "$namespace_filter" --limit 1 --offset 0 --json - assert_success - obligation_triggers_total=$(echo "$output" | jq -r '.pagination.total // 0') + run_otdfctl_obligation_triggers list --namespace "$namespace_filter" --limit 1 --offset 0 --json + obligation_triggers_json="$output" + obligation_triggers_total=$(echo "$obligation_triggers_json" | jq -r '.pagination.total // 0') jq -cn \ --argjson actions "$actions_total" \ @@ -422,8 +482,12 @@ assert_namespace_state_delta() { subject_mapping_json_by_migrated_from() { local namespace_filter="$1" local source_mapping_id="$2" + local subject_mappings_json + + run_otdfctl_sm list --namespace "$namespace_filter" --limit 100 --offset 0 --json + subject_mappings_json="$output" - ./otdfctl $HOST $WITH_CREDS policy subject-mappings list --namespace "$namespace_filter" --limit 100 --offset 0 --json \ + echo "$subject_mappings_json" \ | jq -cer --arg source_mapping_id "$source_mapping_id" ' [ (.subject_mappings // [])[] @@ -435,8 +499,11 @@ subject_mapping_json_by_migrated_from() { subject_mapping_id_by_migrated_from() { local namespace_filter="$1" local source_mapping_id="$2" + local migrated_mapping_id - subject_mapping_json_by_migrated_from "$namespace_filter" "$source_mapping_id" | jq -r '.id // empty' + migrated_mapping_id=$(subject_mapping_json_by_migrated_from "$namespace_filter" "$source_mapping_id" | jq -r '.id // empty') + assert_not_equal "$migrated_mapping_id" "" + printf '%s\n' "$migrated_mapping_id" } assert_subject_mapping_created_in_namespace() { @@ -464,14 +531,13 @@ assert_subject_mapping_created_in_namespace() { local expected_action_target_id expected_action_target_id=$(action_id_by_name_in_namespace "$action_name" "$namespace_id") - assert_not_equal "$expected_action_target_id" "" local expected_scs_target_id expected_scs_target_id=$(scs_id_by_migrated_from "$namespace_id" "$source_scs_id") - assert_not_equal "$expected_scs_target_id" "" local source_mapping_json - source_mapping_json=$(./otdfctl $HOST $WITH_CREDS policy subject-mappings get --id "$source_mapping_id" --json) + run_otdfctl_sm get --id "$source_mapping_id" --json + source_mapping_json="$output" local created_mapping_json created_mapping_json=$(subject_mapping_json_by_migrated_from "$namespace_id" "$source_mapping_id") @@ -499,10 +565,12 @@ assert_subject_mapping_already_migrated_in_namespace() { assert_not_equal "$existing_mapping_id" "" local source_mapping_json - source_mapping_json=$(./otdfctl $HOST $WITH_CREDS policy subject-mappings get --id "$source_mapping_id" --json) + run_otdfctl_sm get --id "$source_mapping_id" --json + source_mapping_json="$output" local existing_mapping_json - existing_mapping_json=$(./otdfctl $HOST $WITH_CREDS policy subject-mappings get --id "$existing_mapping_id" --json) + run_otdfctl_sm get --id "$existing_mapping_id" --json + existing_mapping_json="$output" assert_equal "$(echo "$existing_mapping_json" | jq -r '.id // empty')" "$existing_mapping_id" assert_equal "$(echo "$existing_mapping_json" | jq -r '.namespace.id')" "$namespace_id" @@ -517,7 +585,8 @@ assert_legacy_subject_mapping_still_exists() { assert_not_equal "$source_mapping_id" "" local legacy_mapping_json - legacy_mapping_json=$(./otdfctl $HOST $WITH_CREDS policy subject-mappings get --id "$source_mapping_id" --json) + run_otdfctl_sm get --id "$source_mapping_id" --json + legacy_mapping_json="$output" assert_equal "$(echo "$legacy_mapping_json" | jq -r '.id // empty')" "$source_mapping_id" assert_equal "$(echo "$legacy_mapping_json" | jq -r '.namespace.id // empty')" "" @@ -535,16 +604,24 @@ assert_no_subject_mappings_in_namespace() { obligation_trigger_json_by_id() { local trigger_id="$1" local namespace_filter="$2" + local triggers_json - ./otdfctl $HOST $WITH_CREDS policy obligations triggers list --namespace "$namespace_filter" --limit 100 --offset 0 --json \ + run_otdfctl_obligation_triggers list --namespace "$namespace_filter" --limit 100 --offset 0 --json + triggers_json="$output" + + echo "$triggers_json" \ | jq -cer --arg trigger_id "$trigger_id" '(.triggers // [])[] | select(.id == $trigger_id)' } obligation_trigger_json_by_migrated_from() { local namespace_filter="$1" local source_trigger_id="$2" + local triggers_json + + run_otdfctl_obligation_triggers list --namespace "$namespace_filter" --limit 100 --offset 0 --json + triggers_json="$output" - ./otdfctl $HOST $WITH_CREDS policy obligations triggers list --namespace "$namespace_filter" --limit 100 --offset 0 --json \ + echo "$triggers_json" \ | jq -cer --arg source_trigger_id "$source_trigger_id" ' [ (.triggers // [])[] @@ -556,8 +633,11 @@ obligation_trigger_json_by_migrated_from() { obligation_trigger_id_by_migrated_from() { local namespace_filter="$1" local source_trigger_id="$2" + local migrated_trigger_id - obligation_trigger_json_by_migrated_from "$namespace_filter" "$source_trigger_id" | jq -r '.id // empty' + migrated_trigger_id=$(obligation_trigger_json_by_migrated_from "$namespace_filter" "$source_trigger_id" | jq -r '.id // empty') + assert_not_equal "$migrated_trigger_id" "" + printf '%s\n' "$migrated_trigger_id" } assert_metadata_labels_preserved() { @@ -578,21 +658,29 @@ action_json_by_name_in_namespace() { local action_name="$1" local namespace_filter="$2" - ./otdfctl $HOST $WITH_CREDS policy actions get --name "$action_name" --namespace "$namespace_filter" --json + run_otdfctl_action get --name "$action_name" --namespace "$namespace_filter" --json + printf '%s\n' "$output" } action_id_by_name_in_namespace() { local action_name="$1" local namespace_filter="$2" + local action_id - action_json_by_name_in_namespace "$action_name" "$namespace_filter" | jq -r '.id // empty' + action_id=$(action_json_by_name_in_namespace "$action_name" "$namespace_filter" | jq -r '.id // empty') + assert_not_equal "$action_id" "" + printf '%s\n' "$action_id" } scs_json_by_migrated_from() { local namespace_filter="$1" local source_scs_id="$2" + local scs_json - ./otdfctl $HOST $WITH_CREDS policy scs list --namespace "$namespace_filter" --limit 100 --offset 0 --json \ + run_otdfctl_scs list --namespace "$namespace_filter" --limit 100 --offset 0 --json + scs_json="$output" + + echo "$scs_json" \ | jq -cer --arg source_scs_id "$source_scs_id" ' [ (.subject_condition_sets // [])[] @@ -604,15 +692,22 @@ scs_json_by_migrated_from() { scs_id_by_migrated_from() { local namespace_filter="$1" local source_scs_id="$2" + local migrated_scs_id - scs_json_by_migrated_from "$namespace_filter" "$source_scs_id" | jq -r '.id // empty' + migrated_scs_id=$(scs_json_by_migrated_from "$namespace_filter" "$source_scs_id" | jq -r '.id // empty') + assert_not_equal "$migrated_scs_id" "" + printf '%s\n' "$migrated_scs_id" } registered_resource_json_by_migrated_from() { local namespace_filter="$1" local source_resource_id="$2" + local resources_json + + run_otdfctl_registered_resources list --namespace "$namespace_filter" --limit 100 --offset 0 --json + resources_json="$output" - ./otdfctl $HOST $WITH_CREDS policy registered-resources list --namespace "$namespace_filter" --limit 100 --offset 0 --json \ + echo "$resources_json" \ | jq -cer --arg source_resource_id "$source_resource_id" ' [ (.resources // [])[] @@ -624,15 +719,22 @@ registered_resource_json_by_migrated_from() { registered_resource_id_by_migrated_from() { local namespace_filter="$1" local source_resource_id="$2" + local migrated_resource_id - registered_resource_json_by_migrated_from "$namespace_filter" "$source_resource_id" | jq -r '.id // empty' + migrated_resource_id=$(registered_resource_json_by_migrated_from "$namespace_filter" "$source_resource_id" | jq -r '.id // empty') + assert_not_equal "$migrated_resource_id" "" + printf '%s\n' "$migrated_resource_id" } registered_resource_value_json_by_migrated_from() { local resource_id="$1" local source_value_id="$2" + local resource_values_json + + run_otdfctl_registered_resource_values list --resource "$resource_id" --limit 100 --offset 0 --json + resource_values_json="$output" - ./otdfctl $HOST $WITH_CREDS policy registered-resources values list --resource "$resource_id" --limit 100 --offset 0 --json \ + echo "$resource_values_json" \ | jq -cer --arg source_value_id "$source_value_id" ' [ (.values // [])[] @@ -644,25 +746,29 @@ registered_resource_value_json_by_migrated_from() { registered_resource_value_id_by_migrated_from() { local resource_id="$1" local source_value_id="$2" + local migrated_resource_value_id - registered_resource_value_json_by_migrated_from "$resource_id" "$source_value_id" | jq -r '.id // empty' + migrated_resource_value_id=$(registered_resource_value_json_by_migrated_from "$resource_id" "$source_value_id" | jq -r '.id // empty') + assert_not_equal "$migrated_resource_value_id" "" + printf '%s\n' "$migrated_resource_value_id" } assert_action_absent_in_namespace() { local action_name="$1" local namespace_filter="$2" - run sh -c "./otdfctl $HOST $WITH_CREDS policy actions get --name \"$action_name\" --namespace \"$namespace_filter\" --json" + run ./otdfctl --host http://localhost:8080 --with-client-creds-file ./creds.json policy actions get --name "$action_name" --namespace "$namespace_filter" --json assert_failure } assert_scs_absent_in_namespace() { local source_scs_id="$1" local namespace_filter="$2" + local scs_list_json - run sh -c "./otdfctl $HOST $WITH_CREDS policy scs list --namespace \"$namespace_filter\" --limit 100 --offset 0 --json" - assert_success - assert_equal "$(echo "$output" | jq -r --arg source_scs_id "$source_scs_id" '[(.subject_condition_sets // [])[] | select((.metadata.labels.migrated_from // "") == $source_scs_id)] | length')" "0" + run_otdfctl_scs list --namespace "$namespace_filter" --limit 100 --offset 0 --json + scs_list_json="$output" + assert_equal "$(echo "$scs_list_json" | jq -r --arg source_scs_id "$source_scs_id" '[(.subject_condition_sets // [])[] | select((.metadata.labels.migrated_from // "") == $source_scs_id)] | length')" "0" } registered_resource_values_signature() { @@ -735,13 +841,13 @@ assert_action_already_migrated_in_namespace() { local namespace_id="$2" local existing_action_id="$3" - assert_not_equal "$existing_action_id" "" - local existing_action_json - existing_action_json=$(./otdfctl $HOST $WITH_CREDS policy actions get --id "$existing_action_id" --json) + run_otdfctl_action get --id "$existing_action_id" --json + existing_action_json="$output" assert_equal "$(echo "$existing_action_json" | jq -r '.id // empty')" "$existing_action_id" assert_equal "$(echo "$existing_action_json" | jq -r '.namespace.id')" "$namespace_id" + assert_equal "$(echo "$existing_action_json" | jq -r '.name')" "$action_name" } assert_custom_action_created_in_namespace() { @@ -750,7 +856,8 @@ assert_custom_action_created_in_namespace() { local namespace_id="$3" local source_action_json - source_action_json=$(./otdfctl $HOST $WITH_CREDS policy actions get --id "$source_action_id" --json) + run_otdfctl_action get --id "$source_action_id" --json + source_action_json="$output" local created_action_json created_action_json=$(action_json_by_name_in_namespace "$action_name" "$namespace_id") @@ -776,7 +883,8 @@ assert_legacy_custom_action_still_exists() { assert_not_equal "$action_name" "" local legacy_action_json - legacy_action_json=$(./otdfctl $HOST $WITH_CREDS policy actions get --id "$action_id" --json) + run_otdfctl_action get --id "$action_id" --json + legacy_action_json="$output" assert_equal "$(echo "$legacy_action_json" | jq -r '.id // empty')" "$action_id" assert_equal "$(echo "$legacy_action_json" | jq -r '.name')" "$action_name" @@ -788,7 +896,8 @@ assert_scs_created_in_namespace() { local namespace_id="$2" local source_scs_json - source_scs_json=$(./otdfctl $HOST $WITH_CREDS policy scs get --id "$source_scs_id" --json) + run_otdfctl_scs get --id "$source_scs_id" --json + source_scs_json="$output" local created_scs_json created_scs_json=$(scs_json_by_migrated_from "$namespace_id" "$source_scs_id") @@ -831,10 +940,10 @@ assert_registered_resource_created_in_namespace() { local expected_action_target_id expected_action_target_id=$(action_id_by_name_in_namespace "$action_name" "$namespace_id") - assert_not_equal "$expected_action_target_id" "" local source_resource_json - source_resource_json=$(./otdfctl $HOST $WITH_CREDS policy registered-resources get --id "$source_resource_id" --json) + run_otdfctl_registered_resources get --id "$source_resource_id" --json + source_resource_json="$output" local created_resource_json created_resource_json=$(registered_resource_json_by_migrated_from "$namespace_id" "$source_resource_id") @@ -852,7 +961,8 @@ assert_registered_resource_created_in_namespace() { assert_not_equal "$(echo "$created_resource_json" | jq -r '.metadata.labels.migration_run // empty')" "" local source_resource_value_json - source_resource_value_json=$(./otdfctl $HOST $WITH_CREDS policy registered-resources values get --id "$source_value_id" --json) + run_otdfctl_registered_resource_values get --id "$source_value_id" --json + source_resource_value_json="$output" local created_resource_value_json created_resource_value_json=$(registered_resource_value_json_by_migrated_from "$created_resource_id" "$source_value_id") @@ -877,13 +987,13 @@ assert_registered_resource_already_migrated_in_namespace() { local namespace_id="$2" local existing_resource_id="$3" - assert_not_equal "$existing_resource_id" "" - local source_resource_json - source_resource_json=$(./otdfctl $HOST $WITH_CREDS policy registered-resources get --id "$source_resource_id" --json) + run_otdfctl_registered_resources get --id "$source_resource_id" --json + source_resource_json="$output" local existing_resource_json - existing_resource_json=$(./otdfctl $HOST $WITH_CREDS policy registered-resources get --id "$existing_resource_id" --json) + run_otdfctl_registered_resources get --id "$existing_resource_id" --json + existing_resource_json="$output" assert_equal "$(echo "$existing_resource_json" | jq -r '.id // empty')" "$existing_resource_id" assert_equal "$(echo "$existing_resource_json" | jq -r '.namespace.id')" "$namespace_id" @@ -897,7 +1007,8 @@ assert_registered_resource_value_uses_action() { local attribute_value_id="$3" local value_json - value_json=$(./otdfctl $HOST $WITH_CREDS policy registered-resources values get --id "$value_id" --json) + run_otdfctl_registered_resource_values get --id "$value_id" --json + value_json="$output" assert_equal "$(echo "$value_json" | jq -r '.action_attribute_values | length')" "1" assert_equal "$(echo "$value_json" | jq -r '.action_attribute_values[0].action.id')" "$expected_action_id" @@ -910,18 +1021,17 @@ assert_legacy_registered_resource_still_exists() { local resource_name="$3" local resource_value="$4" - assert_not_equal "$source_resource_id" "" - assert_not_equal "$source_value_id" "" - local legacy_resource_json - legacy_resource_json=$(./otdfctl $HOST $WITH_CREDS policy registered-resources get --id "$source_resource_id" --json) + run_otdfctl_registered_resources get --id "$source_resource_id" --json + legacy_resource_json="$output" assert_equal "$(echo "$legacy_resource_json" | jq -r '.id // empty')" "$source_resource_id" assert_equal "$(echo "$legacy_resource_json" | jq -r '.name')" "$resource_name" assert_equal "$(echo "$legacy_resource_json" | jq -r '.namespace.id // empty')" "" local legacy_resource_value_json - legacy_resource_value_json=$(./otdfctl $HOST $WITH_CREDS policy registered-resources values get --id "$source_value_id" --json) + run_otdfctl_registered_resource_values get --id "$source_value_id" --json + legacy_resource_value_json="$output" assert_equal "$(echo "$legacy_resource_value_json" | jq -r '.id // empty')" "$source_value_id" assert_equal "$(echo "$legacy_resource_value_json" | jq -r '.value')" "$resource_value" @@ -951,7 +1061,6 @@ assert_obligation_trigger_created_in_namespace() { local expected_action_target_id expected_action_target_id=$(action_id_by_name_in_namespace "$action_name" "$namespace_id") - assert_not_equal "$expected_action_target_id" "" local source_trigger_json source_trigger_json=$(obligation_trigger_json_by_id "$source_trigger_id" "$namespace_id") @@ -1000,7 +1109,8 @@ assert_obligation_trigger_already_migrated_in_namespace() { assert_not_equal "$existing_action_id" "" local existing_action_json - existing_action_json=$(./otdfctl $HOST $WITH_CREDS policy actions get --id "$existing_action_id" --json) + run_otdfctl_action get --id "$existing_action_id" --json + existing_action_json="$output" assert_equal "$(echo "$existing_action_json" | jq -r '.id // empty')" "$existing_action_id" assert_equal "$(echo "$existing_action_json" | jq -r '.namespace.id')" "$namespace_id" @@ -1032,13 +1142,13 @@ assert_scs_already_migrated_in_namespace() { local namespace_id="$2" local existing_scs_id="$3" - assert_not_equal "$existing_scs_id" "" - local source_scs_json - source_scs_json=$(./otdfctl $HOST $WITH_CREDS policy scs get --id "$source_scs_id" --json) + run_otdfctl_scs get --id "$source_scs_id" --json + source_scs_json="$output" local existing_scs_json - existing_scs_json=$(./otdfctl $HOST $WITH_CREDS policy scs get --id "$existing_scs_id" --json) + run_otdfctl_scs get --id "$existing_scs_id" --json + existing_scs_json="$output" assert_equal "$(echo "$existing_scs_json" | jq -r '.id // empty')" "$existing_scs_id" assert_equal "$(echo "$existing_scs_json" | jq -r '.namespace.id')" "$namespace_id" @@ -1048,10 +1158,9 @@ assert_scs_already_migrated_in_namespace() { assert_legacy_scs_still_exists() { local source_scs_id="$1" - assert_not_equal "$source_scs_id" "" - local legacy_scs_json - legacy_scs_json=$(./otdfctl $HOST $WITH_CREDS policy scs get --id "$source_scs_id" --json) + run_otdfctl_scs get --id "$source_scs_id" --json + legacy_scs_json="$output" assert_equal "$(echo "$legacy_scs_json" | jq -r '.id // empty')" "$source_scs_id" assert_equal "$(echo "$legacy_scs_json" | jq -r '.namespace.id // empty')" "" @@ -1124,10 +1233,11 @@ setup_file() { assert_not_equal "$ATTR_B_ID" "" assert_not_equal "$ATTR_B_VAL_1_ID" "" - run sh -c "./otdfctl $HOST $WITH_CREDS policy actions get --name read --json" - assert_success + local global_read_json + run_otdfctl_action get --name read --json + global_read_json="$output" export GLOBAL_READ_ID - GLOBAL_READ_ID=$(echo "$output" | jq -r '.id // empty') + GLOBAL_READ_ID=$(echo "$global_read_json" | jq -r '.id // empty') assert_not_equal "$GLOBAL_READ_ID" "" } @@ -1213,15 +1323,30 @@ teardown_file() { unset TRACKED_SCS_IDS TRACKED_SUBJECT_MAPPING_IDS TRACKED_OBLIGATION_TRIGGER_IDS } -# Asserts action-scope migration only creates the required custom action -# target, does not create unrelated namespaced objects as a side effect, and is -# idempotent on rerun. -@test "migrate namespaced-policy actions creates only required custom actions" { +# Asserts action-scope migration can fan out one legacy custom action into +# multiple namespaces when registered-resource and obligation-trigger anchors +# reference it across those namespaces, does not create unrelated namespaced +# objects as a side effect, and is idempotent on rerun. +@test "migrate namespaced-policy actions fans out custom actions from RR and trigger anchors" { local custom_action_name="${TEST_PREFIX}-download" - local shared_scs='[{"condition_groups":[{"conditions":[{"operator":1,"subject_external_values":["'"${TEST_PREFIX}"'-engineering"],"subject_external_selector_value":".org.name"}],"boolean_operator":1}]}]' + local shared_scs='[{"condition_groups":[{"conditions":[{"operator":1,"subject_external_values":["'"${TEST_PREFIX}"'-shared"],"subject_external_selector_value":".org.name"}],"boolean_operator":1}]}]' local custom_action_labels=(--label "test_case=actions" --label "fixture=${TEST_PREFIX}-custom-action") + local rr_a_name="${TEST_PREFIX}-repo-a" + local rr_a_value="${TEST_PREFIX}-repo-a-main" + local rr_a_labels=(--label "test_case=actions" --label "fixture=${TEST_PREFIX}-rr-a") + local rr_a_value_labels=(--label "test_case=actions" --label "fixture=${TEST_PREFIX}-rr-a-value") + local obligation_b_name="${TEST_PREFIX}-notify-b" + local obligation_b_value="${TEST_PREFIX}-notify-b-default" + local trigger_b_client_id="${TEST_PREFIX}-client-b" + local trigger_b_labels=(--label "test_case=actions" --label "fixture=${TEST_PREFIX}-trigger-b") local custom_action_id local shared_scs_id + local read_anchor_mapping_id + local rr_a_id + local rr_a_value_id + local obligation_b_id + local obligation_b_value_id + local trigger_b_id local ns_a_state_before local ns_b_state_before local ns_a_state_after @@ -1229,17 +1354,13 @@ teardown_file() { create_global_action custom_action_id "$custom_action_name" "${custom_action_labels[@]}" create_global_scs shared_scs_id "$shared_scs" + create_legacy_subject_mapping read_anchor_mapping_id "$ATTR_A_VAL_1_ID" "$GLOBAL_READ_ID" "$shared_scs_id" + create_global_registered_resource rr_a_id "$rr_a_name" "${rr_a_labels[@]}" + create_registered_resource_value rr_a_value_id "$rr_a_id" "$rr_a_value" --action-attribute-value "$custom_action_id;$ATTR_A_VAL_2_ID" "${rr_a_value_labels[@]}" - # These anchor subject mappings stay legacy/global. Their target namespace - # should be derived from the referenced attribute value during migration. - local ignored_mapping_id - create_legacy_subject_mapping ignored_mapping_id "$ATTR_A_VAL_1_ID" "$GLOBAL_READ_ID" "$shared_scs_id" - # Subject mappings remain the most direct action-scope anchor here even with - # dedicated registered-resource and obligation-trigger migration coverage - # below, because this test is focused on action placement rather than - # downstream object rewriting. - create_legacy_subject_mapping ignored_mapping_id "$ATTR_A_VAL_2_ID" "$custom_action_id" "$shared_scs_id" - create_legacy_subject_mapping ignored_mapping_id "$ATTR_B_VAL_1_ID" "$GLOBAL_READ_ID" "$shared_scs_id" + create_namespaced_obligation obligation_b_id "$NS_B_ID" "$obligation_b_name" --label "test_case=actions" --label "fixture=${TEST_PREFIX}-obligation-b" + create_obligation_value obligation_b_value_id "$obligation_b_id" "$obligation_b_value" --label "test_case=actions" --label "fixture=${TEST_PREFIX}-obligation-b-value" + create_legacy_obligation_trigger trigger_b_id "$ATTR_B_VAL_1_ID" "$custom_action_id" "$obligation_b_value_id" --client-id "$trigger_b_client_id" "${trigger_b_labels[@]}" ns_a_state_before=$(namespace_state_json "$NS_A_ID") ns_b_state_before=$(namespace_state_json "$NS_B_ID") @@ -1251,18 +1372,23 @@ teardown_file() { ns_b_state_after=$(namespace_state_json "$NS_B_ID") assert_namespace_state_delta "$ns_a_state_before" "$ns_a_state_after" 1 0 0 0 0 - assert_namespace_state_delta "$ns_b_state_before" "$ns_b_state_after" 0 0 0 0 0 + assert_namespace_state_delta "$ns_b_state_before" "$ns_b_state_after" 1 0 0 0 0 assert_custom_action_created_in_namespace "$custom_action_name" "$custom_action_id" "$NS_A_ID" - assert_action_absent_in_namespace "$custom_action_name" "$NS_B_ID" + assert_custom_action_created_in_namespace "$custom_action_name" "$custom_action_id" "$NS_B_ID" assert_legacy_custom_action_still_exists "$custom_action_id" "$custom_action_name" + assert_legacy_scs_still_exists "$shared_scs_id" + assert_legacy_subject_mapping_still_exists "$ATTR_A_VAL_1_ID" "$read_anchor_mapping_id" + assert_legacy_registered_resource_still_exists "$rr_a_id" "$rr_a_value_id" "$rr_a_name" "$rr_a_value" + assert_legacy_obligation_trigger_still_exists "$trigger_b_id" "$NS_B_ID" "$ATTR_B_VAL_1_ID" "$custom_action_id" "$obligation_b_value_id" "$trigger_b_client_id" # Re-running the same migration should be idempotent. No namespace-scoped # counts should change on the second pass. - local custom_action_target_id - custom_action_target_id=$(action_id_by_name_in_namespace "$custom_action_name" "$NS_A_ID") - assert_not_equal "$custom_action_target_id" "" + local custom_action_ns_a_target_id + local custom_action_ns_b_target_id + custom_action_ns_a_target_id=$(action_id_by_name_in_namespace "$custom_action_name" "$NS_A_ID") + custom_action_ns_b_target_id=$(action_id_by_name_in_namespace "$custom_action_name" "$NS_B_ID") ns_a_state_before="$ns_a_state_after" ns_b_state_before="$ns_b_state_after" @@ -1276,8 +1402,8 @@ teardown_file() { assert_namespace_state_delta "$ns_a_state_before" "$ns_a_state_after" 0 0 0 0 0 assert_namespace_state_delta "$ns_b_state_before" "$ns_b_state_after" 0 0 0 0 0 - assert_action_already_migrated_in_namespace "$custom_action_name" "$NS_A_ID" "$custom_action_target_id" - assert_action_absent_in_namespace "$custom_action_name" "$NS_B_ID" + assert_action_already_migrated_in_namespace "$custom_action_name" "$NS_A_ID" "$custom_action_ns_a_target_id" + assert_action_already_migrated_in_namespace "$custom_action_name" "$NS_B_ID" "$custom_action_ns_b_target_id" } # Asserts SCS-scope migration creates missing namespaced SCS targets, reuses an @@ -1334,8 +1460,6 @@ teardown_file() { local single_namespace_target_id fanout_ns_a_target_id=$(scs_id_by_migrated_from "$NS_A_ID" "$fanout_scs_id") single_namespace_target_id=$(scs_id_by_migrated_from "$NS_A_ID" "$single_namespace_scs_id") - assert_not_equal "$fanout_ns_a_target_id" "" - assert_not_equal "$single_namespace_target_id" "" ns_a_state_before="$ns_a_state_after" ns_b_state_before="$ns_b_state_after" @@ -1355,23 +1479,28 @@ teardown_file() { assert_scs_absent_in_namespace "$single_namespace_scs_id" "$NS_B_ID" } -# Asserts subject-mapping migration creates namespaced mappings, rewrites action -# and SCS dependencies to the correct target IDs, preserves source metadata on -# the migrated subject mappings, and is idempotent on rerun. -@test "migrate namespaced-policy subject-mappings rewrites action and scs dependencies" { +# Asserts subject-mapping migration creates missing namespaced mappings, +# rewrites both custom-action and standard-action dependencies to the correct +# target IDs, reuses an already-migrated canonical mapping when present, +# rewrites SCS dependencies, preserves source metadata on created mappings, and +# is idempotent on rerun. +@test "migrate namespaced-policy subject-mappings rewrite dependencies and reuse canonical targets" { local custom_action_name="${TEST_PREFIX}-download" local sm_a_scs='[{"condition_groups":[{"conditions":[{"operator":1,"subject_external_values":["'"${TEST_PREFIX}"'-sm-a"],"subject_external_selector_value":".org.name"}],"boolean_operator":1}]}]' local sm_b_scs='[{"condition_groups":[{"conditions":[{"operator":1,"subject_external_values":["'"${TEST_PREFIX}"'-sm-b"],"subject_external_selector_value":".team.name"}],"boolean_operator":1}]}]' local custom_action_labels=(--label "test_case=subject-mappings" --label "fixture=${TEST_PREFIX}-custom-action") local sm_a_scs_labels=(--label "test_case=subject-mappings" --label "fixture=${TEST_PREFIX}-sm-a-scs") local sm_b_scs_labels=(--label "test_case=subject-mappings" --label "fixture=${TEST_PREFIX}-sm-b-scs") - local mapping_a_labels=(--label "test_case=subject-mappings" --label "fixture=${TEST_PREFIX}-mapping-a") - local mapping_b_labels=(--label "test_case=subject-mappings" --label "fixture=${TEST_PREFIX}-mapping-b") + local mapping_custom_labels=(--label "test_case=subject-mappings" --label "fixture=${TEST_PREFIX}-mapping-custom") + local mapping_standard_labels=(--label "test_case=subject-mappings" --label "fixture=${TEST_PREFIX}-mapping-standard") local custom_action_id local sm_a_scs_id local sm_b_scs_id - local mapping_a_id - local mapping_b_id + local mapping_custom_id + local mapping_standard_id + local ns_b_read_action_id + local existing_sm_b_scs_id + local existing_mapping_standard_id local ns_a_state_before local ns_b_state_before local ns_a_state_after @@ -1381,8 +1510,12 @@ teardown_file() { create_global_scs sm_a_scs_id "$sm_a_scs" "${sm_a_scs_labels[@]}" create_global_scs sm_b_scs_id "$sm_b_scs" "${sm_b_scs_labels[@]}" - create_legacy_subject_mapping mapping_a_id "$ATTR_A_VAL_1_ID" "$custom_action_id" "$sm_a_scs_id" "${mapping_a_labels[@]}" - create_legacy_subject_mapping mapping_b_id "$ATTR_B_VAL_1_ID" "$GLOBAL_READ_ID" "$sm_b_scs_id" "${mapping_b_labels[@]}" + create_legacy_subject_mapping mapping_custom_id "$ATTR_A_VAL_1_ID" "$custom_action_id" "$sm_a_scs_id" "${mapping_custom_labels[@]}" + create_legacy_subject_mapping mapping_standard_id "$ATTR_B_VAL_1_ID" "$GLOBAL_READ_ID" "$sm_b_scs_id" "${mapping_standard_labels[@]}" + + lookup_namespaced_action_id ns_b_read_action_id "read" "$NS_B_ID" + create_namespaced_scs existing_sm_b_scs_id "$NS_B_ID" "$sm_b_scs" + create_namespaced_subject_mapping existing_mapping_standard_id "$NS_B_ID" "$ATTR_B_VAL_1_ID" "$ns_b_read_action_id" "$existing_sm_b_scs_id" --label "test_case=subject-mappings" --label "fixture=${TEST_PREFIX}-existing-mapping-standard" ns_a_state_before=$(namespace_state_json "$NS_A_ID") ns_b_state_before=$(namespace_state_json "$NS_B_ID") @@ -1394,27 +1527,25 @@ teardown_file() { ns_b_state_after=$(namespace_state_json "$NS_B_ID") assert_namespace_state_delta "$ns_a_state_before" "$ns_a_state_after" 1 1 1 0 0 - assert_namespace_state_delta "$ns_b_state_before" "$ns_b_state_after" 0 1 1 0 0 + assert_namespace_state_delta "$ns_b_state_before" "$ns_b_state_after" 0 0 0 0 0 - assert_subject_mapping_created_in_namespace "$mapping_a_id" "$NS_A_ID" "$ATTR_A_VAL_1_ID" "$custom_action_name" "$custom_action_id" "create" "$sm_a_scs_id" - assert_subject_mapping_created_in_namespace "$mapping_b_id" "$NS_B_ID" "$ATTR_B_VAL_1_ID" "read" "$GLOBAL_READ_ID" "existing_standard" "$sm_b_scs_id" + assert_subject_mapping_created_in_namespace "$mapping_custom_id" "$NS_A_ID" "$ATTR_A_VAL_1_ID" "$custom_action_name" "$custom_action_id" "create" "$sm_a_scs_id" + assert_standard_action_resolved_in_namespace "read" "$NS_B_ID" + assert_scs_already_migrated_in_namespace "$sm_b_scs_id" "$NS_B_ID" "$existing_sm_b_scs_id" + assert_subject_mapping_already_migrated_in_namespace "$mapping_standard_id" "$NS_B_ID" "$existing_mapping_standard_id" - assert_legacy_subject_mapping_still_exists "$ATTR_A_VAL_1_ID" "$mapping_a_id" - assert_legacy_subject_mapping_still_exists "$ATTR_B_VAL_1_ID" "$mapping_b_id" + assert_legacy_subject_mapping_still_exists "$ATTR_A_VAL_1_ID" "$mapping_custom_id" + assert_legacy_subject_mapping_still_exists "$ATTR_B_VAL_1_ID" "$mapping_standard_id" # Re-running the same migration should be idempotent. The custom action, # migrated SCS targets, and migrated subject mappings should all resolve as # already_migrated on the second pass. Standard read remains existing_standard. local custom_action_target_id local sm_a_scs_target_id - local sm_b_scs_target_id - local mapping_a_target_id - local mapping_b_target_id + local mapping_custom_target_id custom_action_target_id=$(action_id_by_name_in_namespace "$custom_action_name" "$NS_A_ID") sm_a_scs_target_id=$(scs_id_by_migrated_from "$NS_A_ID" "$sm_a_scs_id") - sm_b_scs_target_id=$(scs_id_by_migrated_from "$NS_B_ID" "$sm_b_scs_id") - mapping_a_target_id=$(subject_mapping_id_by_migrated_from "$NS_A_ID" "$mapping_a_id") - mapping_b_target_id=$(subject_mapping_id_by_migrated_from "$NS_B_ID" "$mapping_b_id") + mapping_custom_target_id=$(subject_mapping_id_by_migrated_from "$NS_A_ID" "$mapping_custom_id") ns_a_state_before="$ns_a_state_after" ns_b_state_before="$ns_b_state_after" @@ -1431,9 +1562,9 @@ teardown_file() { assert_action_already_migrated_in_namespace "$custom_action_name" "$NS_A_ID" "$custom_action_target_id" assert_standard_action_resolved_in_namespace "read" "$NS_B_ID" assert_scs_already_migrated_in_namespace "$sm_a_scs_id" "$NS_A_ID" "$sm_a_scs_target_id" - assert_scs_already_migrated_in_namespace "$sm_b_scs_id" "$NS_B_ID" "$sm_b_scs_target_id" - assert_subject_mapping_already_migrated_in_namespace "$mapping_a_id" "$NS_A_ID" "$mapping_a_target_id" - assert_subject_mapping_already_migrated_in_namespace "$mapping_b_id" "$NS_B_ID" "$mapping_b_target_id" + assert_scs_already_migrated_in_namespace "$sm_b_scs_id" "$NS_B_ID" "$existing_sm_b_scs_id" + assert_subject_mapping_already_migrated_in_namespace "$mapping_custom_id" "$NS_A_ID" "$mapping_custom_target_id" + assert_subject_mapping_already_migrated_in_namespace "$mapping_standard_id" "$NS_B_ID" "$existing_mapping_standard_id" } # Asserts registered-resource migration creates missing namespaced targets, diff --git a/otdfctl/migrations/namespacedpolicy/canonical.go b/otdfctl/migrations/namespacedpolicy/canonical.go index b30fc3e2c2..38b6711210 100644 --- a/otdfctl/migrations/namespacedpolicy/canonical.go +++ b/otdfctl/migrations/namespacedpolicy/canonical.go @@ -13,6 +13,24 @@ type registeredResourceValueCanonical struct { Value string `json:"value"` ActionAttributeValues []string `json:"action_attribute_values"` } +type canonicalSubjectSetEntry struct { + ConditionGroups []canonicalConditionGroupEntry `json:"condition_groups"` +} + +type canonicalConditionGroupEntry struct { + Conditions []canonicalConditionEntry `json:"conditions"` + BooleanOperator int32 `json:"boolean_operator"` +} + +type canonicalConditionEntry struct { + Selector string `json:"selector"` + Operator int32 `json:"operator"` + Values []string `json:"values"` +} + +type canonicalRequestContextEntry struct { + ClientID string `json:"client_id"` +} func actionCanonicalEqual(source, target *policy.Action) bool { s := canonicalActionName(source) @@ -162,25 +180,6 @@ func canonicalObligationTriggerContext(contexts []*policy.RequestContext) string return string(encoded) } -type canonicalSubjectSetEntry struct { - ConditionGroups []canonicalConditionGroupEntry `json:"condition_groups"` -} - -type canonicalConditionGroupEntry struct { - Conditions []canonicalConditionEntry `json:"conditions"` - BooleanOperator int32 `json:"boolean_operator"` -} - -type canonicalConditionEntry struct { - Selector string `json:"selector"` - Operator int32 `json:"operator"` - Values []string `json:"values"` -} - -type canonicalRequestContextEntry struct { - ClientID string `json:"client_id"` -} - func normalizeSubjectSet(ss *policy.SubjectSet) canonicalSubjectSetEntry { groups := make([]canonicalConditionGroupEntry, 0, len(ss.GetConditionGroups())) for _, cg := range ss.GetConditionGroups() { diff --git a/otdfctl/migrations/namespacedpolicy/canonical_test.go b/otdfctl/migrations/namespacedpolicy/canonical_test.go index 8ecc3141ce..13ffaccf3f 100644 --- a/otdfctl/migrations/namespacedpolicy/canonical_test.go +++ b/otdfctl/migrations/namespacedpolicy/canonical_test.go @@ -292,3 +292,341 @@ func protoCloneTrigger(trigger *policy.ObligationTrigger) *policy.ObligationTrig Metadata: trigger.GetMetadata(), } } + +func TestActionCanonicalEqual(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + left *policy.Action + right *policy.Action + want bool + }{ + {name: "both nil", left: nil, right: nil, want: false}, + {name: "nil left, populated right", left: nil, right: &policy.Action{Name: "decrypt"}, want: false}, + {name: "both blank names", left: &policy.Action{Name: " "}, right: &policy.Action{Name: ""}, want: false}, + {name: "equal ignoring case and whitespace", left: &policy.Action{Name: " Decrypt "}, right: &policy.Action{Name: "DECRYPT"}, want: true}, + {name: "different names", left: &policy.Action{Name: "decrypt"}, right: &policy.Action{Name: "read"}, want: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, actionCanonicalEqual(tc.left, tc.right)) + }) + } +} + +func canonicalTestSCS(values ...string) *policy.SubjectConditionSet { + return &policy.SubjectConditionSet{ + SubjectSets: []*policy.SubjectSet{ + {ConditionGroups: []*policy.ConditionGroup{ + { + Conditions: []*policy.Condition{ + { + SubjectExternalSelectorValue: ".role", + Operator: policy.SubjectMappingOperatorEnum_SUBJECT_MAPPING_OPERATOR_ENUM_IN, + SubjectExternalValues: values, + }, + }, + BooleanOperator: policy.ConditionBooleanTypeEnum_CONDITION_BOOLEAN_TYPE_ENUM_AND, + }, + }}, + }, + } +} + +func TestSubjectConditionSetCanonicalEqualSkipsNilChildren(t *testing.T) { + t.Parallel() + + clean := canonicalTestSCS("admin") + cleanGroup := clean.GetSubjectSets()[0].GetConditionGroups()[0] + + noisyGroup := &policy.SubjectConditionSet{ + SubjectSets: []*policy.SubjectSet{ + {ConditionGroups: []*policy.ConditionGroup{nil, cleanGroup}}, + }, + } + noisyCondition := &policy.SubjectConditionSet{ + SubjectSets: []*policy.SubjectSet{ + {ConditionGroups: []*policy.ConditionGroup{ + { + Conditions: []*policy.Condition{ + nil, + { + SubjectExternalSelectorValue: ".role", + Operator: policy.SubjectMappingOperatorEnum_SUBJECT_MAPPING_OPERATOR_ENUM_IN, + SubjectExternalValues: []string{"admin"}, + }, + }, + BooleanOperator: policy.ConditionBooleanTypeEnum_CONDITION_BOOLEAN_TYPE_ENUM_AND, + }, + }}, + }, + } + + tests := []struct { + name string + left *policy.SubjectConditionSet + right *policy.SubjectConditionSet + want bool + }{ + {name: "both nil", left: nil, right: nil, want: false}, + {name: "nil condition group is skipped", left: noisyGroup, right: clean, want: true}, + {name: "nil condition within a group is skipped", left: noisyCondition, right: clean, want: true}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, subjectConditionSetCanonicalEqual(tc.left, tc.right)) + }) + } +} + +func TestSubjectMappingCanonicalEqual(t *testing.T) { + t.Parallel() + + scs := canonicalTestSCS("admin") + fqn := "https://example.com/attr/c/value/s" + + equivalentLeft := &policy.SubjectMapping{ + Id: "sm-left", + AttributeValue: &policy.Value{Fqn: fqn}, + Actions: []*policy.Action{{Name: "Read"}, {Name: "WRITE"}}, + SubjectConditionSet: scs, + } + equivalentRight := &policy.SubjectMapping{ + Id: "sm-right", + AttributeValue: &policy.Value{Fqn: " " + fqn + " "}, + Actions: []*policy.Action{{Name: "write"}, {Name: "read"}}, + SubjectConditionSet: scs, + } + + differentActionsLeft := &policy.SubjectMapping{ + AttributeValue: &policy.Value{Fqn: fqn}, + Actions: []*policy.Action{{Name: "read"}}, + SubjectConditionSet: scs, + } + differentActionsRight := &policy.SubjectMapping{ + AttributeValue: &policy.Value{Fqn: fqn}, + Actions: []*policy.Action{{Name: "write"}}, + SubjectConditionSet: scs, + } + + missingFQN := &policy.SubjectMapping{ + SubjectConditionSet: scs, + Actions: []*policy.Action{{Name: "read"}}, + } + missingSCS := &policy.SubjectMapping{ + AttributeValue: &policy.Value{Fqn: fqn}, + Actions: []*policy.Action{{Name: "read"}}, + } + complete := &policy.SubjectMapping{ + AttributeValue: &policy.Value{Fqn: fqn}, + Actions: []*policy.Action{{Name: "read"}}, + SubjectConditionSet: scs, + } + + tests := []struct { + name string + left *policy.SubjectMapping + right *policy.SubjectMapping + want bool + }{ + {name: "both nil", left: nil, right: nil, want: false}, + {name: "equal ignoring id, case, whitespace, action order", left: equivalentLeft, right: equivalentRight, want: true}, + {name: "different actions are not equal", left: differentActionsLeft, right: differentActionsRight, want: false}, + {name: "missing attribute value fqn never equals", left: missingFQN, right: complete, want: false}, + {name: "missing subject condition set never equals", left: missingSCS, right: complete, want: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, subjectMappingCanonicalEqual(tc.left, tc.right)) + }) + } +} + +func TestObligationTriggerCanonicalEqual(t *testing.T) { + t.Parallel() + + base := &policy.ObligationTrigger{ + AttributeValue: &policy.Value{Fqn: "https://example.com/attr/c/value/s"}, + ObligationValue: &policy.ObligationValue{Fqn: "https://example.com/obligation/o/value/notify"}, + Action: &policy.Action{Name: "decrypt"}, + } + + equivalentLeft := protoCloneTrigger(base) + equivalentLeft.Id = "ot-left" + equivalentLeft.Action = &policy.Action{Name: "Decrypt"} + equivalentLeft.AttributeValue = &policy.Value{Fqn: " " + base.GetAttributeValue().GetFqn() + " "} + + equivalentRight := protoCloneTrigger(base) + equivalentRight.Id = "ot-right" + equivalentRight.Action = &policy.Action{Name: "DECRYPT"} + equivalentRight.ObligationValue = &policy.ObligationValue{Fqn: " " + base.GetObligationValue().GetFqn() + " "} + + diffActionLeft := protoCloneTrigger(base) + diffActionLeft.Action = &policy.Action{Name: "read"} + diffActionRight := protoCloneTrigger(base) + diffActionRight.Action = &policy.Action{Name: "write"} + + missingAttr := protoCloneTrigger(base) + missingAttr.AttributeValue = &policy.Value{} + + missingAction := protoCloneTrigger(base) + missingAction.Action = &policy.Action{} + + missingObligation := protoCloneTrigger(base) + missingObligation.ObligationValue = &policy.ObligationValue{} + + tests := []struct { + name string + left *policy.ObligationTrigger + right *policy.ObligationTrigger + want bool + }{ + {name: "both nil", left: nil, right: nil, want: false}, + {name: "equal ignoring id, case, whitespace", left: equivalentLeft, right: equivalentRight, want: true}, + {name: "different action names are not equal", left: diffActionLeft, right: diffActionRight, want: false}, + {name: "missing attribute value fqn never equals", left: missingAttr, right: base, want: false}, + {name: "missing action name never equals", left: missingAction, right: base, want: false}, + {name: "missing obligation value fqn never equals", left: missingObligation, right: base, want: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, obligationTriggerCanonicalEqual(tc.left, tc.right)) + }) + } +} + +func TestRegisteredResourceCanonicalEqual(t *testing.T) { + t.Parallel() + + clean := testRegisteredResource( + "resource-clean", + "documents", + testRegisteredResourceValue( + "prod", + testActionAttributeValue( + "", + "read", + testAttributeValue("https://example.com/attr/c/value/p", nil), + ), + ), + ) + + withNilValuesAndAAVs := &policy.RegisteredResource{ + Name: "documents", + Values: []*policy.RegisteredResourceValue{ + nil, + { + Value: "prod", + ActionAttributeValues: []*policy.RegisteredResourceValue_ActionAttributeValue{ + nil, + { + Action: &policy.Action{Name: "read"}, + AttributeValue: &policy.Value{Fqn: "https://example.com/attr/c/value/p"}, + }, + }, + }, + }, + } + + withSentinelAAV := &policy.RegisteredResource{ + Name: "documents", + Values: []*policy.RegisteredResourceValue{ + { + Value: "prod", + ActionAttributeValues: []*policy.RegisteredResourceValue_ActionAttributeValue{ + {Action: &policy.Action{}, AttributeValue: &policy.Value{}}, + { + Action: &policy.Action{Name: "read"}, + AttributeValue: &policy.Value{Fqn: "https://example.com/attr/c/value/p"}, + }, + }, + }, + }, + } + + differentValue := testRegisteredResource( + "resource-other", + "documents", + testRegisteredResourceValue( + "dev", + testActionAttributeValue( + "", + "read", + testAttributeValue("https://example.com/attr/c/value/p", nil), + ), + ), + ) + + differentName := testRegisteredResource( + "resource-renamed", + "reports", + testRegisteredResourceValue( + "prod", + testActionAttributeValue( + "", + "read", + testAttributeValue("https://example.com/attr/c/value/p", nil), + ), + ), + ) + + differentAttributeValue := testRegisteredResource( + "resource-other", + "documents", + testRegisteredResourceValue( + "prod", + testActionAttributeValue( + "", + "read", + testAttributeValue("https://example.com/attr/c/value/other", nil), + ), + ), + ) + + differentAction := testRegisteredResource( + "resource-other", + "documents", + testRegisteredResourceValue( + "prod", + testActionAttributeValue( + "", + "write", + testAttributeValue("https://example.com/attr/c/value/p", nil), + ), + ), + ) + + emptyName := &policy.RegisteredResource{Name: " "} + + tests := []struct { + name string + left *policy.RegisteredResource + right *policy.RegisteredResource + want bool + }{ + {name: "both nil", left: nil, right: nil, want: false}, + {name: "nil value and nil AAV are skipped", left: withNilValuesAndAAVs, right: clean, want: true}, + {name: "AAV with empty action and attribute is skipped", left: withSentinelAAV, right: clean, want: true}, + {name: "different values are not equal", left: clean, right: differentValue, want: false}, + {name: "different resource names are not equal", left: clean, right: differentName, want: false}, + {name: "different AAV attribute value FQNs are not equal", left: clean, right: differentAttributeValue, want: false}, + {name: "different AAV action names are not equal", left: clean, right: differentAction, want: false}, + {name: "empty name never equals", left: emptyName, right: clean, want: false}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, registeredResourceCanonicalEqual(tc.left, tc.right)) + }) + } +} diff --git a/otdfctl/migrations/namespacedpolicy/derived.go b/otdfctl/migrations/namespacedpolicy/derived.go index 727e0c0f79..72f8ed70d9 100644 --- a/otdfctl/migrations/namespacedpolicy/derived.go +++ b/otdfctl/migrations/namespacedpolicy/derived.go @@ -17,9 +17,8 @@ type DerivedTargets struct { } type DerivedAction struct { - Source *policy.Action - References []*ActionReference - Targets []*policy.Namespace + Source *policy.Action + Targets []*policy.Namespace } type DerivedSubjectConditionSet struct { @@ -48,7 +47,6 @@ type targetDeriver struct { namespaceByID map[string]*policy.Namespace namespaceByFQN map[string]*policy.Namespace actionTargetsByID map[string]*namespaceAccumulator - actionRefsByID map[string]*actionReferenceAccumulator scsTargetsByID map[string]*namespaceAccumulator } @@ -70,6 +68,9 @@ func deriveTargets(retrieved *Retrieved, namespaces []*policy.Namespace) (*Deriv } for _, mapping := range retrieved.Candidates.SubjectMappings { + if mapping == nil { + continue + } item, err := deriver.deriveSubjectMapping(mapping) if err != nil { return nil, err @@ -79,6 +80,9 @@ func deriveTargets(retrieved *Retrieved, namespaces []*policy.Namespace) (*Deriv } for _, resource := range retrieved.Candidates.RegisteredResources { + if resource == nil { + continue + } item, err := deriver.deriveRegisteredResource(resource) if err != nil { if errors.Is(err, errSkipRegisteredResource) { @@ -91,6 +95,9 @@ func deriveTargets(retrieved *Retrieved, namespaces []*policy.Namespace) (*Deriv } for _, trigger := range retrieved.Candidates.ObligationTriggers { + if trigger == nil { + continue + } item, err := deriver.deriveObligationTrigger(trigger) if err != nil { return nil, err @@ -100,17 +107,23 @@ func deriveTargets(retrieved *Retrieved, namespaces []*policy.Namespace) (*Deriv } for _, action := range retrieved.Candidates.Actions { - item, err := deriver.deriveAction(action) - if err != nil { - return nil, err + if action == nil { + continue + } + item := deriver.deriveAction(action) + if item == nil { + continue } derived.Actions = append(derived.Actions, item) } for _, scs := range retrieved.Candidates.SubjectConditionSets { - item, err := deriver.deriveSubjectConditionSet(scs) - if err != nil { - return nil, err + if scs == nil { + continue + } + item := deriver.deriveSubjectConditionSet(scs) + if item == nil { + continue } derived.SubjectConditionSets = append(derived.SubjectConditionSets, item) } @@ -138,7 +151,6 @@ func newTargetDeriver(namespaces []*policy.Namespace) *targetDeriver { namespaceByID: namespaceByID, namespaceByFQN: namespaceByFQN, actionTargetsByID: make(map[string]*namespaceAccumulator), - actionRefsByID: make(map[string]*actionReferenceAccumulator), scsTargetsByID: make(map[string]*namespaceAccumulator), } } @@ -156,9 +168,6 @@ func (d *targetDeriver) deriveSubjectMapping(mapping *policy.SubjectMapping) (*D func (d *targetDeriver) deriveRegisteredResource(resource *policy.RegisteredResource) (*DerivedRegisteredResource, error) { item := &DerivedRegisteredResource{Source: resource} - if resource == nil { - return nil, fmt.Errorf("%w: registered resource is empty", ErrUndeterminedTargetMapping) - } namespaceRef, ok := registeredResourceNamespaceRef(resource) if !ok { @@ -196,40 +205,35 @@ func (d *targetDeriver) deriveObligationTrigger(trigger *policy.ObligationTrigge return item, nil } -func (d *targetDeriver) deriveAction(action *policy.Action) (*DerivedAction, error) { - item := &DerivedAction{ - Source: action, - } - if action == nil { - return nil, fmt.Errorf("%w: empty action candidate", ErrUndeterminedTargetMapping) - } - if refs := d.actionRefsByID[action.GetId()]; refs != nil { - item.References = refs.slice() - } +// deriveAction returns nil when the action has no observed referencing +// subject mapping, registered resource, or obligation trigger in scope — an +// orphan action has no target namespace to migrate to and is silently skipped +// rather than carried through as an empty-targets ResolvedAction. +func (d *targetDeriver) deriveAction(action *policy.Action) *DerivedAction { targets := d.targets(d.actionTargetsByID[action.GetId()]) if len(targets) == 0 { - if len(item.References) > 0 { - return nil, fmt.Errorf("%w: no target namespaces were discovered for action %q", ErrUndeterminedTargetMapping, action.GetId()) - } - return item, nil + return nil } - item.Targets = targets - return item, nil + return &DerivedAction{ + Source: action, + Targets: targets, + } } -func (d *targetDeriver) deriveSubjectConditionSet(scs *policy.SubjectConditionSet) (*DerivedSubjectConditionSet, error) { - item := &DerivedSubjectConditionSet{Source: scs} - if scs == nil { - return nil, fmt.Errorf("%w: empty subject condition set candidate", ErrUndeterminedTargetMapping) - } +// deriveSubjectConditionSet returns nil when the SCS has no referencing +// subject mapping in scope — a legacy SCS that isn't being migrated is silently +// skipped rather than treated as a retrieval invariant violation. +func (d *targetDeriver) deriveSubjectConditionSet(scs *policy.SubjectConditionSet) *DerivedSubjectConditionSet { targets := d.targets(d.scsTargetsByID[scs.GetId()]) if len(targets) == 0 { - return nil, fmt.Errorf("%w: no target namespaces were discovered for subject condition set %q", ErrUndeterminedTargetMapping, scs.GetId()) + return nil } - item.Targets = targets - return item, nil + return &DerivedSubjectConditionSet{ + Source: scs, + Targets: targets, + } } func (d *targetDeriver) observeSubjectMapping(item *DerivedSubjectMapping) { @@ -239,7 +243,6 @@ func (d *targetDeriver) observeSubjectMapping(item *DerivedSubjectMapping) { for _, action := range item.Source.GetActions() { d.addActionTarget(action.GetId(), item.Target) - d.addActionReference(action.GetId(), ActionReferenceKindSubjectMapping, item.Source.GetId(), item.Target) } if scsID := item.Source.GetSubjectConditionSet().GetId(); scsID != "" { @@ -255,7 +258,6 @@ func (d *targetDeriver) observeRegisteredResource(item *DerivedRegisteredResourc for _, value := range item.Source.GetValues() { for _, aav := range value.GetActionAttributeValues() { d.addActionTarget(aav.GetAction().GetId(), item.Target) - d.addActionReference(aav.GetAction().GetId(), ActionReferenceKindRegisteredResource, item.Source.GetId(), item.Target) } } } @@ -266,7 +268,6 @@ func (d *targetDeriver) observeObligationTrigger(item *DerivedObligationTrigger) } d.addActionTarget(item.Source.GetAction().GetId(), item.Target) - d.addActionReference(item.Source.GetAction().GetId(), ActionReferenceKindObligationTrigger, item.Source.GetId(), item.Target) } func (d *targetDeriver) addActionTarget(actionID string, namespace *policy.Namespace) { @@ -282,24 +283,6 @@ func (d *targetDeriver) addActionTarget(actionID string, namespace *policy.Names targets.add(namespace) } -func (d *targetDeriver) addActionReference(actionID string, kind ActionReferenceKind, id string, namespace *policy.Namespace) { - if actionID == "" || id == "" || kind == "" { - return - } - - references := d.actionRefsByID[actionID] - if references == nil { - references = newActionReferenceAccumulator() - d.actionRefsByID[actionID] = references - } - - references.add(&ActionReference{ - Kind: kind, - ID: id, - Namespace: namespace, - }) -} - func (d *targetDeriver) addSubjectConditionSetTarget(scsID string, namespace *policy.Namespace) { if scsID == "" || namespace == nil { return @@ -456,45 +439,3 @@ func (a *namespaceAccumulator) slice() []*policy.Namespace { return append([]*policy.Namespace(nil), a.items...) } - -type actionReferenceAccumulator struct { - items []*ActionReference - seen map[string]struct{} -} - -func newActionReferenceAccumulator() *actionReferenceAccumulator { - return &actionReferenceAccumulator{ - seen: make(map[string]struct{}), - } -} - -func (a *actionReferenceAccumulator) add(reference *ActionReference) { - if a == nil || reference == nil || reference.Kind == "" || reference.ID == "" { - return - } - key := actionReferenceKey(reference) - if key == "" { - return - } - if _, ok := a.seen[key]; ok { - return - } - a.seen[key] = struct{}{} - a.items = append(a.items, reference) -} - -func (a *actionReferenceAccumulator) slice() []*ActionReference { - if a == nil { - return nil - } - - return append([]*ActionReference(nil), a.items...) -} - -func actionReferenceKey(reference *ActionReference) string { - if reference == nil { - return "" - } - - return string(reference.Kind) + "|" + reference.ID + "|" + namespaceRefKey(reference.Namespace) -} diff --git a/otdfctl/migrations/namespacedpolicy/derived_test.go b/otdfctl/migrations/namespacedpolicy/derived_test.go index 59f17f989a..9f61e4f7a9 100644 --- a/otdfctl/migrations/namespacedpolicy/derived_test.go +++ b/otdfctl/migrations/namespacedpolicy/derived_test.go @@ -77,11 +77,6 @@ func TestDeriveTargetsCollectsTargetsAndReferencesFromDependencies(t *testing.T) require.Len(t, derived.Actions, 1) require.Len(t, derived.Actions[0].Targets, 1) assert.Equal(t, namespace.GetId(), derived.Actions[0].Targets[0].GetId()) - assert.ElementsMatch(t, []string{ - "subject_mapping|mapping-1", - "registered_resource|resource-1", - "obligation_trigger|trigger-1", - }, actionReferenceKindsAndIDs(derived.Actions[0].References)) require.Len(t, derived.SubjectConditionSets, 1) require.Len(t, derived.SubjectConditionSets[0].Targets, 1) @@ -191,14 +186,273 @@ func TestDeriveTargetsSkipsRegisteredResourceWithoutActionAttributeValues(t *tes assert.Empty(t, derived.RegisteredResources) } -func actionReferenceKindsAndIDs(references []*ActionReference) []string { - keys := make([]string, 0, len(references)) - for _, reference := range references { - if reference == nil { - continue - } - keys = append(keys, string(reference.Kind)+"|"+reference.ID) +func TestDeriveTargetsSkipsNilSubjectMappingCandidate(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{Id: "ns-1", Fqn: "https://example.com"} + // A nil entry in Candidates.SubjectMappings is a retrieval artifact and + // must be silently dropped, not halt the whole loop. + derived, err := deriveTargets( + &Retrieved{ + Scopes: []Scope{ScopeSubjectMappings}, + Candidates: Candidates{ + SubjectMappings: []*policy.SubjectMapping{ + nil, + { + Id: "mapping-1", + AttributeValue: testAttributeValue( + "https://example.com/attr/classification/value/secret", + namespace, + ), + SubjectConditionSet: &policy.SubjectConditionSet{Id: "scs-1"}, + Actions: []*policy.Action{{Id: "action-1", Name: "decrypt"}}, + }, + }, + }, + }, + []*policy.Namespace{namespace}, + ) + require.NoError(t, err) + require.Len(t, derived.SubjectMappings, 1) + assert.Equal(t, "mapping-1", derived.SubjectMappings[0].Source.GetId()) +} + +func TestDeriveTargetsSkipsNilObligationTriggerCandidate(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{Id: "ns-1", Fqn: "https://example.com"} + // A nil entry in Candidates.ObligationTriggers is a retrieval artifact + // and must be silently dropped, not halt the whole loop. + derived, err := deriveTargets( + &Retrieved{ + Scopes: []Scope{ScopeObligationTriggers}, + Candidates: Candidates{ + ObligationTriggers: []*policy.ObligationTrigger{ + nil, + { + Id: "trigger-1", + Action: &policy.Action{Id: "action-1", Name: "decrypt"}, + ObligationValue: &policy.ObligationValue{ + Id: "ov-1", + Fqn: "https://example.com/obl/notify/value/email", + Obligation: &policy.Obligation{Namespace: namespace}, + }, + }, + }, + }, + }, + []*policy.Namespace{namespace}, + ) + require.NoError(t, err) + require.Len(t, derived.ObligationTriggers, 1) + assert.Equal(t, "trigger-1", derived.ObligationTriggers[0].Source.GetId()) +} + +func TestDeriveTargetsSkipsNilActionCandidate(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{Id: "ns-1", Fqn: "https://example.com"} + // A nil entry in Candidates.Actions is treated as a retrieval artifact and + // silently dropped rather than aborting the loop. The surviving action is + // referenced by a subject mapping so it has an observable target namespace + // (otherwise deriveAction would also drop it as an orphan). + derived, err := deriveTargets( + &Retrieved{ + Scopes: []Scope{ScopeActions, ScopeSubjectMappings}, + Candidates: Candidates{ + Actions: []*policy.Action{ + nil, + {Id: "action-1", Name: "decrypt"}, + }, + SubjectMappings: []*policy.SubjectMapping{ + { + Id: "mapping-1", + AttributeValue: testAttributeValue( + "https://example.com/attr/classification/value/secret", + namespace, + ), + SubjectConditionSet: &policy.SubjectConditionSet{Id: "scs-1"}, + Actions: []*policy.Action{{Id: "action-1", Name: "decrypt"}}, + }, + }, + }, + }, + []*policy.Namespace{namespace}, + ) + require.NoError(t, err) + require.Len(t, derived.Actions, 1) + assert.Equal(t, "action-1", derived.Actions[0].Source.GetId()) +} + +func TestDeriveTargetsSkipsActionsWithNoReferencingDependency(t *testing.T) { + t.Parallel() + + // An orphan action — one no in-scope subject mapping, registered resource, + // or obligation trigger refers to — has no derivable target namespace. + // Treat it as "nothing to migrate" and drop it at derive time rather than + // carrying it through the resolver as a zero-Results ResolvedAction. + derived, err := deriveTargets( + &Retrieved{ + Scopes: []Scope{ScopeActions}, + Candidates: Candidates{ + Actions: []*policy.Action{ + {Id: "action-orphan", Name: "decrypt"}, + }, + }, + }, + nil, + ) + require.NoError(t, err) + assert.Empty(t, derived.Actions) +} + +func TestDeriveTargetsSkipsNilSubjectConditionSetCandidate(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{Id: "ns-1", Fqn: "https://example.com"} + // Include a surviving SCS referenced by a subject mapping so the slice + // isn't empty — the nil must be dropped in place, not abort the loop. + derived, err := deriveTargets( + &Retrieved{ + Scopes: []Scope{ScopeSubjectConditionSets, ScopeSubjectMappings}, + Candidates: Candidates{ + SubjectConditionSets: []*policy.SubjectConditionSet{ + nil, + {Id: "scs-1"}, + }, + SubjectMappings: []*policy.SubjectMapping{ + { + Id: "mapping-1", + AttributeValue: testAttributeValue( + "https://example.com/attr/classification/value/secret", + namespace, + ), + SubjectConditionSet: &policy.SubjectConditionSet{Id: "scs-1"}, + Actions: []*policy.Action{{Id: "action-1", Name: "decrypt"}}, + }, + }, + }, + }, + []*policy.Namespace{namespace}, + ) + require.NoError(t, err) + require.Len(t, derived.SubjectConditionSets, 1) + assert.Equal(t, "scs-1", derived.SubjectConditionSets[0].Source.GetId()) +} + +func TestDeriveTargetsSkipsSubjectConditionSetWithNoReferencingDependency(t *testing.T) { + t.Parallel() + + // A legacy SCS that no in-scope subject mapping points at has no + // derivable target. Treat it as "nothing to migrate" and drop it, rather + // than erroring and halting the whole plan. + derived, err := deriveTargets( + &Retrieved{ + Scopes: []Scope{ScopeSubjectConditionSets}, + Candidates: Candidates{ + SubjectConditionSets: []*policy.SubjectConditionSet{ + {Id: "scs-1"}, + }, + }, + }, + nil, + ) + require.NoError(t, err) + assert.Empty(t, derived.SubjectConditionSets) +} + +func TestDeriveTargetsSkipsNilRegisteredResourceCandidate(t *testing.T) { + t.Parallel() + + derived, err := deriveTargets( + &Retrieved{ + Scopes: []Scope{ScopeRegisteredResources}, + Candidates: Candidates{ + RegisteredResources: []*policy.RegisteredResource{nil}, + }, + }, + nil, + ) + require.NoError(t, err) + assert.Empty(t, derived.RegisteredResources) +} + +func TestDeriveTargetsResolvesNamespaceByFQNWhenIDMissing(t *testing.T) { + t.Parallel() + + targetNamespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", } + // AttributeValue has no nested Attribute.Namespace, forcing + // namespaceFromAttributeValue to produce an {Fqn-only} reference from + // the parsed FQN. resolveNamespace must fall back from the empty ID + // lookup to the FQN lookup and return the full namespace record. + derived, err := deriveTargets( + &Retrieved{ + Scopes: []Scope{ScopeSubjectMappings}, + Candidates: Candidates{ + SubjectMappings: []*policy.SubjectMapping{ + { + Id: "mapping-1", + AttributeValue: &policy.Value{ + Fqn: "https://example.com/attr/classification/value/secret", + }, + }, + }, + }, + }, + []*policy.Namespace{targetNamespace}, + ) + require.NoError(t, err) + require.Len(t, derived.SubjectMappings, 1) + assert.Equal(t, targetNamespace.GetId(), derived.SubjectMappings[0].Target.GetId()) + assert.Equal(t, targetNamespace.GetFqn(), derived.SubjectMappings[0].Target.GetFqn()) +} + +func TestDeriveTargetsFailsWhenNamespaceRefNotFound(t *testing.T) { + t.Parallel() + + derived, err := deriveTargets( + &Retrieved{ + Scopes: []Scope{ScopeSubjectMappings}, + Candidates: Candidates{ + SubjectMappings: []*policy.SubjectMapping{ + { + Id: "mapping-1", + AttributeValue: &policy.Value{ + Fqn: "https://missing.example.com/attr/foo/value/bar", + }, + }, + }, + }, + }, + []*policy.Namespace{ + {Id: "ns-1", Fqn: "https://example.com"}, + }, + ) + require.Error(t, err) + assert.Nil(t, derived) + require.ErrorIs(t, err, ErrMissingTargetNamespace) + assert.Contains(t, err.Error(), `subject mapping "mapping-1"`) + assert.Contains(t, err.Error(), "missing.example.com") +} + +func TestNamespaceAccumulatorDeduplicatesByRef(t *testing.T) { + t.Parallel() + + // Dedup is load-bearing: an action referenced from multiple observers + // targeting the same namespace must resolve to a single target, not N — + // this drives single-vs-multi-namespace branching downstream. + acc := newNamespaceAccumulator() + acc.add(&policy.Namespace{Id: "ns-1", Fqn: "https://example.com"}) + acc.add(&policy.Namespace{Id: "ns-1", Fqn: "https://example.com"}) // same identifier, different struct + acc.add(&policy.Namespace{Id: "ns-2", Fqn: "https://other.example.com"}) + acc.add(nil) + acc.add(&policy.Namespace{}) // empty key — must be skipped - return keys + got := acc.slice() + require.Len(t, got, 2) + assert.Equal(t, "ns-1", got[0].GetId()) + assert.Equal(t, "ns-2", got[1].GetId()) } diff --git a/otdfctl/migrations/namespacedpolicy/finalize_plan.go b/otdfctl/migrations/namespacedpolicy/finalize_plan.go index 2377c46970..e4723ecbc6 100644 --- a/otdfctl/migrations/namespacedpolicy/finalize_plan.go +++ b/otdfctl/migrations/namespacedpolicy/finalize_plan.go @@ -2,15 +2,13 @@ package namespacedpolicy import ( "errors" - - "github.com/opentdf/platform/protocol/go/policy" ) var ErrNilResolvedTargets = errors.New("planner resolved state is required") // finalizePlan converts the fully resolved graph into the current Plan shape. // This is the last planner stage before artifact building/execution wiring. -func finalizePlan(resolved *ResolvedTargets, namespaces []*policy.Namespace) (*Plan, error) { +func finalizePlan(resolved *ResolvedTargets) (*Plan, error) { if resolved == nil { return nil, ErrNilResolvedTargets } @@ -20,7 +18,7 @@ func finalizePlan(resolved *ResolvedTargets, namespaces []*policy.Namespace) (*P return nil, err } - finalizer := newPlanFinalizer(resolved, namespaces) + finalizer := newPlanFinalizer(resolved) if scopes.requiresActions() { for _, action := range resolved.Actions { @@ -59,8 +57,6 @@ func finalizePlan(resolved *ResolvedTargets, namespaces []*policy.Namespace) (*P // preserves per-target status and dependency bindings for downstream creates. type planFinalizer struct { resolved *ResolvedTargets - namespaces []*policy.Namespace - namespacePlansByID map[string]*NamespacePlan actions []*ActionPlan subjectConditionSets []*SubjectConditionSetPlan subjectMappings []*SubjectMappingPlan @@ -68,35 +64,21 @@ type planFinalizer struct { obligationTriggers []*ObligationTriggerPlan } -func newPlanFinalizer(resolved *ResolvedTargets, namespaces []*policy.Namespace) *planFinalizer { +func newPlanFinalizer(resolved *ResolvedTargets) *planFinalizer { return &planFinalizer{ - resolved: resolved, - namespaces: namespaces, - namespacePlansByID: make(map[string]*NamespacePlan), + resolved: resolved, } } func (f *planFinalizer) build() *Plan { - plan := &Plan{ + return &Plan{ Scopes: append([]Scope(nil), f.resolved.Scopes...), - Namespaces: make([]*NamespacePlan, 0, len(f.namespacePlansByID)), Actions: append([]*ActionPlan(nil), f.actions...), SubjectConditionSets: append([]*SubjectConditionSetPlan(nil), f.subjectConditionSets...), SubjectMappings: append([]*SubjectMappingPlan(nil), f.subjectMappings...), RegisteredResources: append([]*RegisteredResourcePlan(nil), f.registeredResources...), ObligationTriggers: append([]*ObligationTriggerPlan(nil), f.obligationTriggers...), } - - for _, namespace := range f.namespaces { - if namespace == nil || namespace.GetId() == "" { - continue - } - if namespacePlan, ok := f.namespacePlansByID[namespace.GetId()]; ok { - plan.Namespaces = append(plan.Namespaces, namespacePlan) - } - } - - return plan } func (f *planFinalizer) addResolvedAction(item *ResolvedAction) { @@ -109,9 +91,8 @@ func (f *planFinalizer) addResolvedAction(item *ResolvedAction) { } actionPlan := &ActionPlan{ - Source: item.Source, - References: append([]*ActionReference(nil), item.References...), - Targets: make([]*ActionTargetPlan, 0, len(item.Results)), + Source: item.Source, + Targets: make([]*ActionTargetPlan, 0, len(item.Results)), } for _, result := range item.Results { @@ -120,8 +101,6 @@ func (f *planFinalizer) addResolvedAction(item *ResolvedAction) { continue } actionPlan.Targets = append(actionPlan.Targets, target) - - f.addNamespacePlacement(target.Namespace, ScopeActions, item.Source.GetId()) } f.actions = append(f.actions, actionPlan) @@ -143,8 +122,6 @@ func (f *planFinalizer) addResolvedSubjectConditionSet(item *ResolvedSubjectCond continue } scsPlan.Targets = append(scsPlan.Targets, target) - - f.addNamespacePlacement(target.Namespace, ScopeSubjectConditionSets, item.Source.GetId()) } f.subjectConditionSets = append(f.subjectConditionSets, scsPlan) @@ -160,7 +137,6 @@ func (f *planFinalizer) addResolvedSubjectMapping(item *ResolvedSubjectMapping) target := f.newSubjectMappingTarget(item) if target != nil { mappingPlan.Target = target - f.addNamespacePlacement(target.Namespace, ScopeSubjectMappings, item.Source.GetId()) } f.subjectMappings = append(f.subjectMappings, mappingPlan) @@ -179,7 +155,6 @@ func (f *planFinalizer) addResolvedRegisteredResource(item *ResolvedRegisteredRe target := f.newRegisteredResourceTarget(item) if target != nil { resourcePlan.Target = target - f.addNamespacePlacement(target.Namespace, ScopeRegisteredResources, item.Source.GetId()) } f.registeredResources = append(f.registeredResources, resourcePlan) @@ -195,53 +170,11 @@ func (f *planFinalizer) addResolvedObligationTrigger(item *ResolvedObligationTri target := f.newObligationTriggerTarget(item) if target != nil { triggerPlan.Target = target - f.addNamespacePlacement(target.Namespace, ScopeObligationTriggers, item.Source.GetId()) } f.obligationTriggers = append(f.obligationTriggers, triggerPlan) } -func (f *planFinalizer) namespacePlan(namespace *policy.Namespace) *NamespacePlan { - if namespace == nil || namespace.GetId() == "" { - return nil - } - - namespacePlan, ok := f.namespacePlansByID[namespace.GetId()] - if ok { - return namespacePlan - } - - namespacePlan = &NamespacePlan{ - Namespace: namespace, - } - f.namespacePlansByID[namespace.GetId()] = namespacePlan - return namespacePlan -} - -func (f *planFinalizer) addNamespacePlacement(namespace *policy.Namespace, scope Scope, sourceID string) { - if namespace == nil || namespace.GetId() == "" || sourceID == "" { - return - } - - namespacePlan := f.namespacePlan(namespace) - if namespacePlan == nil { - return - } - - switch scope { - case ScopeActions: - namespacePlan.Actions = appendUniqueString(namespacePlan.Actions, sourceID) - case ScopeSubjectConditionSets: - namespacePlan.SubjectConditionSets = appendUniqueString(namespacePlan.SubjectConditionSets, sourceID) - case ScopeSubjectMappings: - namespacePlan.SubjectMappings = appendUniqueString(namespacePlan.SubjectMappings, sourceID) - case ScopeRegisteredResources: - namespacePlan.RegisteredResources = appendUniqueString(namespacePlan.RegisteredResources, sourceID) - case ScopeObligationTriggers: - namespacePlan.ObligationTriggers = appendUniqueString(namespacePlan.ObligationTriggers, sourceID) - } -} - func (f *planFinalizer) newSubjectMappingTarget(item *ResolvedSubjectMapping) *SubjectMappingTargetPlan { if item == nil || item.Namespace == nil { return nil @@ -375,12 +308,3 @@ func newSubjectConditionSetTargetPlan(result *ResolvedSubjectConditionSetResult) return target } - -func appendUniqueString(items []string, value string) []string { - for _, item := range items { - if item == value { - return items - } - } - return append(items, value) -} diff --git a/otdfctl/migrations/namespacedpolicy/finalize_plan_test.go b/otdfctl/migrations/namespacedpolicy/finalize_plan_test.go index ded358b6cc..7b0153fff4 100644 --- a/otdfctl/migrations/namespacedpolicy/finalize_plan_test.go +++ b/otdfctl/migrations/namespacedpolicy/finalize_plan_test.go @@ -84,7 +84,7 @@ func TestFinalizePlanBuildsBindingsForDependentObjects(t *testing.T) { NeedsCreate: true, }, }, - }, []*policy.Namespace{namespace}) + }) require.NoError(t, err) require.Len(t, plan.SubjectMappings, 1) @@ -102,11 +102,6 @@ func TestFinalizePlanBuildsBindingsForDependentObjects(t *testing.T) { require.Len(t, plan.ObligationTriggers, 1) require.NotNil(t, plan.ObligationTriggers[0].Target) assert.Equal(t, "action-1", plan.ObligationTriggers[0].Target.ActionSourceID) - - require.Len(t, plan.Namespaces, 1) - assert.Equal(t, []string{"mapping-1"}, plan.Namespaces[0].SubjectMappings) - assert.Equal(t, []string{"resource-1"}, plan.Namespaces[0].RegisteredResources) - assert.Equal(t, []string{"trigger-1"}, plan.Namespaces[0].ObligationTriggers) } func TestFinalizePlanOmitsCreateOnlyBindingsForAlreadyMigratedTargets(t *testing.T) { @@ -164,7 +159,7 @@ func TestFinalizePlanOmitsCreateOnlyBindingsForAlreadyMigratedTargets(t *testing AlreadyMigrated: &policy.ObligationTrigger{Id: "trigger-target"}, }, }, - }, []*policy.Namespace{namespace}) + }) require.NoError(t, err) require.Len(t, plan.SubjectMappings, 1) diff --git a/otdfctl/migrations/namespacedpolicy/interactive_commit_test.go b/otdfctl/migrations/namespacedpolicy/interactive_commit_test.go index 6df4b29370..ad999863a7 100644 --- a/otdfctl/migrations/namespacedpolicy/interactive_commit_test.go +++ b/otdfctl/migrations/namespacedpolicy/interactive_commit_test.go @@ -237,6 +237,81 @@ func TestReviewNamespacedPolicyInteractiveCommitSkipsMappingsDependentOnSkippedS assert.Nil(t, mappingTarget.Execution) } +func TestReviewNamespacedPolicyInteractiveCommitPropagatesAbort(t *testing.T) { + t.Parallel() + + // The user aborts on the first action prompt. The reviewer must (a) return + // ErrInteractiveReviewAborted so the caller stops, and (b) not prompt for + // any later action or downstream object, so no half-applied migration is + // committed. + namespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + attributeValue := testAttributeValue("https://example.com/attr/classification/value/secret", namespace) + plan := &Plan{ + Scopes: []Scope{ + ScopeActions, + ScopeSubjectConditionSets, + ScopeSubjectMappings, + }, + Actions: []*ActionPlan{ + { + Source: &policy.Action{Id: "action-1", Name: "decrypt"}, + Targets: []*ActionTargetPlan{ + {Namespace: namespace, Status: TargetStatusCreate}, + }, + }, + { + Source: &policy.Action{Id: "action-2", Name: "read"}, + Targets: []*ActionTargetPlan{ + {Namespace: namespace, Status: TargetStatusCreate}, + }, + }, + }, + SubjectConditionSets: []*SubjectConditionSetPlan{ + { + Source: &policy.SubjectConditionSet{Id: "scs-1"}, + Targets: []*SubjectConditionSetTargetPlan{ + {Namespace: namespace, Status: TargetStatusCreate}, + }, + }, + }, + SubjectMappings: []*SubjectMappingPlan{ + { + Source: &policy.SubjectMapping{ + Id: "mapping-1", + AttributeValue: attributeValue, + }, + Target: &SubjectMappingTargetPlan{ + Namespace: namespace, + Status: TargetStatusCreate, + ActionSourceIDs: []string{"action-1"}, + SubjectConditionSetSourceID: "scs-1", + }, + }, + }, + } + + prompter := &queuedSelectPrompter{ + selectValues: []string{namespacedPolicyCommitAbort}, + } + + err := ReviewNamespacedPolicyInteractiveCommit(t.Context(), plan, prompter) + require.ErrorIs(t, err, ErrInteractiveReviewAborted) + + // Only the first prompt should have fired; abort must halt the walkthrough. + require.Equal(t, 1, prompter.selectCalls) + + // Abort must not mutate target state — subsequent execution should be able + // to run or the user should be able to retry. + assert.Equal(t, TargetStatusCreate, plan.Actions[0].Targets[0].Status) + assert.Empty(t, plan.Actions[0].Targets[0].Reason) + assert.Equal(t, TargetStatusCreate, plan.Actions[1].Targets[0].Status) + assert.Equal(t, TargetStatusCreate, plan.SubjectConditionSets[0].Targets[0].Status) + assert.Equal(t, TargetStatusCreate, plan.SubjectMappings[0].Target.Status) +} + func TestApplyInteractiveDecisionHandlesChoices(t *testing.T) { t.Parallel() diff --git a/otdfctl/migrations/namespacedpolicy/interactive_review.go b/otdfctl/migrations/namespacedpolicy/interactive_review.go index 9572078d4c..9c71632a27 100644 --- a/otdfctl/migrations/namespacedpolicy/interactive_review.go +++ b/otdfctl/migrations/namespacedpolicy/interactive_review.go @@ -68,6 +68,17 @@ func (r *HuhInteractiveReviewer) Review(ctx context.Context, resolved *ResolvedT return nil } +// reviewRegisteredResource prompts the reviewer to pick a target namespace for a +// conflicted registered resource and rewrites planner state to match that choice. +// +// In-place mutations: +// - resource.Source — replaced with the namespace-filtered clone +// - resource.Namespace — set to the chosen namespace +// - resource.Unresolved — cleared +// - resource.AlreadyMigrated — cleared, then set if an existing match is found +// - resource.NeedsCreate — cleared, then set true if no existing match +// - resolved.Actions — appended to via ensureRegisteredResourceActionResolution +// - namespaceCache — populated/read by reviewNamespaceState func (r *HuhInteractiveReviewer) reviewRegisteredResource( ctx context.Context, resolved *ResolvedTargets, @@ -117,20 +128,15 @@ func (r *HuhInteractiveReviewer) reviewRegisteredResource( resource.AlreadyMigrated = nil resource.NeedsCreate = false - existing, found, err := resolveExistingRegisteredResource(filtered, namespaceState.registeredResources) - switch { - case found: + if existing, found := resolveExistingRegisteredResource(filtered, namespaceState.registeredResources); found { resource.AlreadyMigrated = existing return nil - case err != nil: - return fmt.Errorf("registered resource %q in namespace %q: %w", filtered.GetId(), chosen.GetId(), err) - default: - resource.NeedsCreate = true } + resource.NeedsCreate = true for _, value := range filtered.GetValues() { for _, aav := range value.GetActionAttributeValues() { - if err := ensureRegisteredResourceActionResolution(resolved, resource.Source.GetId(), chosen, aav.GetAction(), namespaceState.actionResolver); err != nil { + if err := ensureRegisteredResourceActionResolution(resolved, chosen, aav.GetAction(), namespaceState.actionResolver); err != nil { return fmt.Errorf("registered resource %q: %w", resource.Source.GetId(), err) } } @@ -368,10 +374,33 @@ func filterRegisteredResourceToNamespace(resource *policy.RegisteredResource, na return cloned, nil } -// ensureRegisteredResourceActionResolution may append or update entries in -// resolved.Actions so the reviewed registered resource's action bindings remain -// executable after namespace-specific filtering. -func ensureRegisteredResourceActionResolution(resolved *ResolvedTargets, resourceID string, namespace *policy.Namespace, action *policy.Action, actionResolver *resolver) error { +// ensureRegisteredResourceActionResolution is the interactive-path counterpart +// to resolveRegisteredResourceDependencies: after the reviewer has picked a +// target namespace for a conflicted registered resource and filtered its AAVs +// to that namespace, this function guarantees every referenced action has a +// corresponding ResolvedAction entry that the executor can bind to at create +// time. +// +// For each action binding on the filtered resource, it: +// 1. Validates the action reference (non-nil, non-empty id). Missing or empty +// ids are a hard error — without an id the executor cannot bind the AAV, +// so the plan must fail here rather than at create time. +// 2. Finds or creates the matching ResolvedAction in resolved.Actions, keyed +// by source id. When creating, the input action is defensively cloned so +// later mutations to the plan do not alter the retriever's cached state. +// 3. Skips if the action is already resolved for this namespace (duplicate +// AAV bindings on the same resource resolve once). Otherwise runs the +// standard resolveActionTargetFromExisting path — same semantics as the +// initial-pass resolver — and appends the result. +// +// Note: this function writes only to resolved.Actions; it does NOT populate +// actionResolver.actionResultsByKey. That map is consumed only by the +// initial-pass dependency helpers (resolveRegisteredResourceDependencies, +// resolveObligationTriggerDependencies, resolveSubjectMappingDependencies) +// which are skipped for interactively-resolved resources because their +// Unresolved state causes resolveRegisteredResource to early-return. The +// final plan is built from resolved.Actions, so the two paths converge there. +func ensureRegisteredResourceActionResolution(resolved *ResolvedTargets, namespace *policy.Namespace, action *policy.Action, actionResolver *resolver) error { if resolved == nil { return ErrNilResolvedTargets } @@ -406,12 +435,6 @@ func ensureRegisteredResourceActionResolution(resolved *ResolvedTargets, resourc item.Source = source } - addActionReferenceIfMissing(item, &ActionReference{ - Kind: ActionReferenceKindRegisteredResource, - ID: resourceID, - Namespace: namespace, - }) - if resolvedActionResultForNamespace(item, namespace) != nil { return nil } @@ -449,20 +472,6 @@ func resolvedActionResultForNamespace(action *ResolvedAction, namespace *policy. return nil } -func addActionReferenceIfMissing(action *ResolvedAction, reference *ActionReference) { - if action == nil || reference == nil { - return - } - - for _, existing := range action.References { - if actionReferenceKey(existing) == actionReferenceKey(reference) { - return - } - } - - action.References = append(action.References, reference) -} - func cloneAction(action *policy.Action) (*policy.Action, error) { if action == nil { return nil, errors.New("action is nil") diff --git a/otdfctl/migrations/namespacedpolicy/interactive_review_test.go b/otdfctl/migrations/namespacedpolicy/interactive_review_test.go index ac5e0804f0..e539c8d62b 100644 --- a/otdfctl/migrations/namespacedpolicy/interactive_review_test.go +++ b/otdfctl/migrations/namespacedpolicy/interactive_review_test.go @@ -103,10 +103,6 @@ func TestHuhInteractiveReviewerResolvesConflictingRegisteredResource(t *testing. require.Len(t, action.Results, 1) assert.True(t, sameNamespace(leftNamespace, action.Results[0].Namespace)) assert.True(t, action.Results[0].NeedsCreate) - require.Len(t, action.References, 1) - assert.Equal(t, ActionReferenceKindRegisteredResource, action.References[0].Kind) - assert.Equal(t, "resource-1", action.References[0].ID) - assert.True(t, sameNamespace(leftNamespace, action.References[0].Namespace)) } func TestHuhInteractiveReviewerSkipsActionResolutionWhenFilteredResourceAlreadyExists(t *testing.T) { @@ -196,6 +192,91 @@ func TestHuhInteractiveReviewerSkipsActionResolutionWhenFilteredResourceAlreadyE assert.Empty(t, resolved.Actions) } +func TestHuhInteractiveReviewerReusesPreviouslyResolvedActionForDuplicateBindings(t *testing.T) { + t.Parallel() + + leftNamespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + rightNamespace := &policy.Namespace{ + Id: "ns-2", + Fqn: "https://example.org", + } + + handler := &plannerTestHandler{ + actionsByNamespace: map[string]*actions.ListActionsResponse{ + leftNamespace.GetId(): { + Pagination: emptyPageResponse(), + }, + }, + registeredResourcesByNamespace: map[string]*registeredresources.ListRegisteredResourcesResponse{ + leftNamespace.GetId(): { + Pagination: emptyPageResponse(), + }, + }, + namespacesResponse: &namespaces.ListNamespacesResponse{ + Namespaces: []*policy.Namespace{leftNamespace, rightNamespace}, + Pagination: emptyPageResponse(), + }, + } + reviewer := NewHuhInteractiveReviewer(handler, &testInteractivePrompter{ + selectValue: namespaceSelectionValue(leftNamespace), + }) + resolved := &ResolvedTargets{ + Scopes: []Scope{ScopeRegisteredResources}, + RegisteredResources: []*ResolvedRegisteredResource{ + { + Source: testRegisteredResource( + "resource-1", + "documents", + testRegisteredResourceValue( + "prod", + testActionAttributeValue( + "action-1", + "decrypt", + testAttributeValue("https://example.com/attr/classification/value/secret", leftNamespace), + ), + testActionAttributeValue( + "action-1", + "decrypt", + testAttributeValue("https://example.com/attr/classification/value/internal", leftNamespace), + ), + testActionAttributeValue( + "action-2", + "encrypt", + testAttributeValue("https://example.org/attr/classification/value/restricted", rightNamespace), + ), + ), + ), + Unresolved: &Unresolved{ + Reason: UnresolvedReasonRegisteredResourceConflictingNamespaces, + }, + }, + }, + } + + err := reviewer.Review(t.Context(), resolved, []*policy.Namespace{leftNamespace, rightNamespace}) + require.NoError(t, err) + + require.Len(t, resolved.RegisteredResources, 1) + reviewedResource := resolved.RegisteredResources[0] + require.NotNil(t, reviewedResource) + require.True(t, sameNamespace(leftNamespace, reviewedResource.Namespace)) + require.Len(t, reviewedResource.Source.GetValues(), 1) + require.Len(t, reviewedResource.Source.GetValues()[0].GetActionAttributeValues(), 2) + + require.Len(t, resolved.Actions, 1) + resolvedAction := resolved.Actions[0] + require.NotNil(t, resolvedAction.Source) + assert.Equal(t, "action-1", resolvedAction.Source.GetId()) + require.Len(t, resolvedAction.Results, 1) + duplicateBindingActionResult := resolvedAction.Results[0] + assert.True(t, sameNamespace(leftNamespace, duplicateBindingActionResult.Namespace)) + assert.True(t, duplicateBindingActionResult.NeedsCreate) + assert.Nil(t, duplicateBindingActionResult.AlreadyMigrated) +} + func TestEnsureRegisteredResourceActionResolutionReusesExistingNamespaceResult(t *testing.T) { t.Parallel() @@ -222,7 +303,6 @@ func TestEnsureRegisteredResourceActionResolutionReusesExistingNamespaceResult(t err := ensureRegisteredResourceActionResolution( resolved, - "resource-1", namespace, &policy.Action{Id: "action-1", Name: "decrypt"}, &resolver{ @@ -233,10 +313,167 @@ func TestEnsureRegisteredResourceActionResolutionReusesExistingNamespaceResult(t require.Len(t, resolved.Actions, 1) require.Len(t, resolved.Actions[0].Results, 1) - require.Len(t, resolved.Actions[0].References, 1) - assert.Equal(t, ActionReferenceKindRegisteredResource, resolved.Actions[0].References[0].Kind) - assert.Equal(t, "resource-1", resolved.Actions[0].References[0].ID) - assert.True(t, sameNamespace(namespace, resolved.Actions[0].References[0].Namespace)) +} + +func TestEnsureRegisteredResourceActionResolutionCreatesNewActionResolution(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + sourceAction := &policy.Action{ + Id: "action-1", + Name: "decrypt_custom", + } + resolved := &ResolvedTargets{} + + err := ensureRegisteredResourceActionResolution( + resolved, + namespace, + sourceAction, + &resolver{ + existing: newExistingTargets(), + }, + ) + require.NoError(t, err) + + require.Len(t, resolved.Actions, 1) + resolvedAction := resolved.Actions[0] + require.NotNil(t, resolvedAction.Source) + assert.NotSame(t, sourceAction, resolvedAction.Source) + assert.Equal(t, sourceAction.GetId(), resolvedAction.Source.GetId()) + assert.Equal(t, sourceAction.GetName(), resolvedAction.Source.GetName()) + + require.Len(t, resolvedAction.Results, 1) + createdActionResult := resolvedAction.Results[0] + assert.True(t, sameNamespace(namespace, createdActionResult.Namespace)) + assert.True(t, createdActionResult.NeedsCreate) + assert.Nil(t, createdActionResult.AlreadyMigrated) + assert.Nil(t, createdActionResult.ExistingStandard) +} + +func TestEnsureRegisteredResourceActionResolutionAppendsMissingNamespaceResult(t *testing.T) { + t.Parallel() + + leftNamespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + rightNamespace := &policy.Namespace{ + Id: "ns-2", + Fqn: "https://example.org", + } + sourceAction := &policy.Action{ + Id: "action-1", + Name: "decrypt_custom", + } + existingMigratedAction := &policy.Action{ + Id: "action-existing", + Name: "decrypt_custom", + } + resolved := &ResolvedTargets{ + Actions: []*ResolvedAction{ + { + Source: sourceAction, + Results: []*ResolvedActionResult{ + { + Namespace: leftNamespace, + NeedsCreate: true, + }, + }, + }, + }, + } + actionResolver := &resolver{ + existing: newExistingTargets(), + } + actionResolver.existing.CustomActions[rightNamespace.GetId()] = []*policy.Action{existingMigratedAction} + + err := ensureRegisteredResourceActionResolution( + resolved, + rightNamespace, + sourceAction, + actionResolver, + ) + require.NoError(t, err) + + require.Len(t, resolved.Actions, 1) + resolvedAction := resolved.Actions[0] + require.Len(t, resolvedAction.Results, 2) + + existingNamespaceActionResult := resolvedAction.Results[0] + assert.True(t, sameNamespace(leftNamespace, existingNamespaceActionResult.Namespace)) + assert.True(t, existingNamespaceActionResult.NeedsCreate) + + appendedActionResult := resolvedAction.Results[1] + assert.True(t, sameNamespace(rightNamespace, appendedActionResult.Namespace)) + assert.False(t, appendedActionResult.NeedsCreate) + assert.Same(t, existingMigratedAction, appendedActionResult.AlreadyMigrated) + assert.Nil(t, appendedActionResult.ExistingStandard) +} + +func TestEnsureRegisteredResourceActionResolutionResolvesStandardActionTarget(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + sourceAction := &policy.Action{ + Id: "action-1", + Name: "read", + } + existingStandardAction := &policy.Action{ + Id: "action-standard", + Name: "read", + Namespace: namespace, + } + actionResolver := &resolver{ + existing: newExistingTargets(), + } + actionResolver.existing.StandardActions[namespace.GetId()] = []*policy.Action{existingStandardAction} + resolved := &ResolvedTargets{} + + err := ensureRegisteredResourceActionResolution( + resolved, + namespace, + sourceAction, + actionResolver, + ) + require.NoError(t, err) + + require.Len(t, resolved.Actions, 1) + require.Len(t, resolved.Actions[0].Results, 1) + standardActionResult := resolved.Actions[0].Results[0] + assert.True(t, sameNamespace(namespace, standardActionResult.Namespace)) + assert.Same(t, existingStandardAction, standardActionResult.ExistingStandard) + assert.False(t, standardActionResult.NeedsCreate) + assert.Nil(t, standardActionResult.AlreadyMigrated) +} + +func TestEnsureRegisteredResourceActionResolutionWrapsResolverError(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + sourceAction := &policy.Action{ + Id: "action-1", + Name: "read", + } + + err := ensureRegisteredResourceActionResolution( + &ResolvedTargets{}, + namespace, + sourceAction, + &resolver{ + existing: newExistingTargets(), + }, + ) + require.ErrorContains(t, err, `action "action-1" in namespace "ns-1"`) + require.ErrorContains(t, err, "matching standard action not found in target namespace") } func TestFilterRegisteredResourceToNamespaceRetainsUnboundValues(t *testing.T) { diff --git a/otdfctl/migrations/namespacedpolicy/plan.go b/otdfctl/migrations/namespacedpolicy/plan.go index 0e296ca151..f1f65e34ee 100644 --- a/otdfctl/migrations/namespacedpolicy/plan.go +++ b/otdfctl/migrations/namespacedpolicy/plan.go @@ -12,7 +12,11 @@ var ( ErrNilRetrieved = errors.New("planner retrieved state is required") ErrMissingTargetNamespace = errors.New("missing target namespace") ErrUndeterminedTargetMapping = errors.New("could not determine target namespace") - ErrDuplicateCanonicalMatch = errors.New("multiple existing target objects match canonical equality in the target namespace") + + ErrMissingActionID = errors.New("action reference missing id") + ErrMissingSubjectConditionSetID = errors.New("subject condition set reference missing id") + ErrUnresolvedActionDependency = errors.New("action dependency not resolved in target namespace") + ErrUnresolvedSubjectConditionSetDependency = errors.New("subject condition set dependency not resolved in target namespace") ) type UnresolvedReason string @@ -28,7 +32,6 @@ type Unresolved struct { type Plan struct { Scopes []Scope `json:"scopes"` - Namespaces []*NamespacePlan `json:"namespaces"` Actions []*ActionPlan `json:"actions"` SubjectConditionSets []*SubjectConditionSetPlan `json:"subject_condition_sets"` SubjectMappings []*SubjectMappingPlan `json:"subject_mappings"` @@ -36,15 +39,6 @@ type Plan struct { ObligationTriggers []*ObligationTriggerPlan `json:"obligation_triggers"` } -type NamespacePlan struct { - Namespace *policy.Namespace `json:"namespace"` - Actions []string `json:"actions,omitempty"` - SubjectConditionSets []string `json:"subject_condition_sets,omitempty"` - SubjectMappings []string `json:"subject_mappings,omitempty"` - RegisteredResources []string `json:"registered_resources,omitempty"` - ObligationTriggers []string `json:"obligation_triggers,omitempty"` -} - type TargetStatus string const ( @@ -63,26 +57,8 @@ type ExecutionResult struct { } type ActionPlan struct { - Source *policy.Action `json:"source"` - // TODO: Add analogous reference metadata for other policy object plan types - // if/when downstream consumers need the same provenance context beyond - // actions. - References []*ActionReference `json:"references,omitempty"` - Targets []*ActionTargetPlan `json:"targets,omitempty"` -} - -type ActionReferenceKind string - -const ( - ActionReferenceKindSubjectMapping ActionReferenceKind = "subject_mapping" - ActionReferenceKindRegisteredResource ActionReferenceKind = "registered_resource" - ActionReferenceKindObligationTrigger ActionReferenceKind = "obligation_trigger" -) - -type ActionReference struct { - Kind ActionReferenceKind `json:"kind"` - ID string `json:"id"` - Namespace *policy.Namespace `json:"namespace,omitempty"` + Source *policy.Action `json:"source"` + Targets []*ActionTargetPlan `json:"targets,omitempty"` } type ActionTargetPlan struct { diff --git a/otdfctl/migrations/namespacedpolicy/plan_test.go b/otdfctl/migrations/namespacedpolicy/plan_test.go index 64be8c5e9a..b11551b4de 100644 --- a/otdfctl/migrations/namespacedpolicy/plan_test.go +++ b/otdfctl/migrations/namespacedpolicy/plan_test.go @@ -17,3 +17,99 @@ func TestNamespaceFromAttributeValueFallsBackToValueFQN(t *testing.T) { require.NotNil(t, namespace) assert.Equal(t, "https://example.com", namespace.GetFqn()) } + +func TestPlanLookupActionTarget(t *testing.T) { + t.Parallel() + + nsA := &policy.Namespace{Id: "ns-a"} + nsB := &policy.Namespace{Id: "ns-b"} + targetA := &ActionTargetPlan{Namespace: nsA, Status: TargetStatusCreate} + targetB := &ActionTargetPlan{Namespace: nsB, Status: TargetStatusCreate} + + plan := &Plan{ + Actions: []*ActionPlan{ + nil, + {Source: nil, Targets: []*ActionTargetPlan{targetA}}, + { + Source: &policy.Action{Id: "action-1", Name: "decrypt"}, + Targets: []*ActionTargetPlan{ + nil, + {Namespace: nil}, + targetA, + targetB, + }, + }, + }, + } + + tests := []struct { + name string + plan *Plan + sourceID string + namespaceID string + want *ActionTargetPlan + }{ + {name: "nil plan", plan: nil, sourceID: "action-1", namespaceID: "ns-a", want: nil}, + {name: "empty sourceID", plan: plan, sourceID: "", namespaceID: "ns-a", want: nil}, + {name: "empty namespaceID", plan: plan, sourceID: "action-1", namespaceID: "", want: nil}, + {name: "match in namespace a", plan: plan, sourceID: "action-1", namespaceID: "ns-a", want: targetA}, + {name: "match in namespace b", plan: plan, sourceID: "action-1", namespaceID: "ns-b", want: targetB}, + {name: "unknown sourceID returns nil", plan: plan, sourceID: "action-missing", namespaceID: "ns-a", want: nil}, + {name: "unknown namespaceID returns nil", plan: plan, sourceID: "action-1", namespaceID: "ns-missing", want: nil}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Same(t, tc.want, tc.plan.LookupActionTarget(tc.sourceID, tc.namespaceID)) + }) + } +} + +func TestPlanLookupSubjectConditionSetTarget(t *testing.T) { + t.Parallel() + + nsA := &policy.Namespace{Id: "ns-a"} + nsB := &policy.Namespace{Id: "ns-b"} + targetA := &SubjectConditionSetTargetPlan{Namespace: nsA, Status: TargetStatusCreate} + targetB := &SubjectConditionSetTargetPlan{Namespace: nsB, Status: TargetStatusCreate} + + plan := &Plan{ + SubjectConditionSets: []*SubjectConditionSetPlan{ + nil, + {Source: nil, Targets: []*SubjectConditionSetTargetPlan{targetA}}, + { + Source: &policy.SubjectConditionSet{Id: "scs-1"}, + Targets: []*SubjectConditionSetTargetPlan{ + nil, + {Namespace: nil}, + targetA, + targetB, + }, + }, + }, + } + + tests := []struct { + name string + plan *Plan + sourceID string + namespaceID string + want *SubjectConditionSetTargetPlan + }{ + {name: "nil plan", plan: nil, sourceID: "scs-1", namespaceID: "ns-a", want: nil}, + {name: "empty sourceID", plan: plan, sourceID: "", namespaceID: "ns-a", want: nil}, + {name: "empty namespaceID", plan: plan, sourceID: "scs-1", namespaceID: "", want: nil}, + {name: "match in namespace a", plan: plan, sourceID: "scs-1", namespaceID: "ns-a", want: targetA}, + {name: "match in namespace b", plan: plan, sourceID: "scs-1", namespaceID: "ns-b", want: targetB}, + {name: "unknown sourceID returns nil", plan: plan, sourceID: "scs-missing", namespaceID: "ns-a", want: nil}, + {name: "unknown namespaceID returns nil", plan: plan, sourceID: "scs-1", namespaceID: "ns-missing", want: nil}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Same(t, tc.want, tc.plan.LookupSubjectConditionSetTarget(tc.sourceID, tc.namespaceID)) + }) + } +} diff --git a/otdfctl/migrations/namespacedpolicy/planner.go b/otdfctl/migrations/namespacedpolicy/planner.go index 67c3211018..dac6f9b0a0 100644 --- a/otdfctl/migrations/namespacedpolicy/planner.go +++ b/otdfctl/migrations/namespacedpolicy/planner.go @@ -132,7 +132,7 @@ func (p *Planner) Plan(ctx context.Context) (*Plan, error) { } } - return finalizePlan(resolved, namespaces) + return finalizePlan(resolved) } // Retrieve the candidate policy constructs for items within scope or dependent diff --git a/otdfctl/migrations/namespacedpolicy/planner_test.go b/otdfctl/migrations/namespacedpolicy/planner_test.go index 5748ca2fef..b6464f3e0d 100644 --- a/otdfctl/migrations/namespacedpolicy/planner_test.go +++ b/otdfctl/migrations/namespacedpolicy/planner_test.go @@ -503,11 +503,6 @@ func TestPlannerPlanAllScopesBuildsAllPlanSections(t *testing.T) { require.Len(t, plan.Actions, 1) require.Len(t, plan.Actions[0].Targets, 1) assert.Equal(t, TargetStatusCreate, plan.Actions[0].Targets[0].Status) - assert.ElementsMatch(t, []string{ - "subject_mapping|mapping-1", - "registered_resource|resource-1", - "obligation_trigger|trigger-1", - }, actionReferenceKindsAndIDs(plan.Actions[0].References)) require.Len(t, plan.SubjectConditionSets, 1) require.Len(t, plan.SubjectConditionSets[0].Targets, 1) @@ -529,13 +524,6 @@ func TestPlannerPlanAllScopesBuildsAllPlanSections(t *testing.T) { require.NotNil(t, plan.ObligationTriggers[0].Target) assert.Equal(t, legacyAction.GetId(), plan.ObligationTriggers[0].Target.ActionSourceID) - require.Len(t, plan.Namespaces, 1) - assert.Equal(t, []string{legacyAction.GetId()}, plan.Namespaces[0].Actions) - assert.Equal(t, []string{legacySCS.GetId()}, plan.Namespaces[0].SubjectConditionSets) - assert.Equal(t, []string{legacyMapping.GetId()}, plan.Namespaces[0].SubjectMappings) - assert.Equal(t, []string{legacyResource.GetId()}, plan.Namespaces[0].RegisteredResources) - assert.Equal(t, []string{legacyTrigger.GetId()}, plan.Namespaces[0].ObligationTriggers) - assert.Equal(t, []string{"", targetNamespace.GetId()}, handler.actionCalls) assert.Equal(t, []string{"", targetNamespace.GetId()}, handler.subjectConditionSetCalls) assert.Equal(t, []string{"", targetNamespace.GetId()}, handler.subjectMappingCalls) @@ -543,6 +531,165 @@ func TestPlannerPlanAllScopesBuildsAllPlanSections(t *testing.T) { assert.Equal(t, []string{"", targetNamespace.GetId()}, handler.obligationTriggerCalls) } +func TestPlannerPlanCarriesMultiActionMappingsAndMultiBindingRegisteredResources(t *testing.T) { + t.Parallel() + + targetNamespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + legacyCustomAction := &policy.Action{ + Id: "action-custom", + Name: "decrypt", + } + legacyReadAction := &policy.Action{ + Id: "action-read", + Name: "read", + } + targetReadAction := &policy.Action{ + Id: "action-read-target", + Name: "read", + Namespace: targetNamespace, + } + legacySCS := &policy.SubjectConditionSet{ + Id: "scs-1", + } + legacyMapping := &policy.SubjectMapping{ + Id: "mapping-1", + AttributeValue: testAttributeValue( + "https://example.com/attr/classification/value/secret", + targetNamespace, + ), + SubjectConditionSet: &policy.SubjectConditionSet{ + Id: legacySCS.GetId(), + }, + Actions: []*policy.Action{ + { + Id: legacyCustomAction.GetId(), + Name: legacyCustomAction.GetName(), + }, + { + Id: legacyReadAction.GetId(), + Name: legacyReadAction.GetName(), + }, + }, + } + legacyResource := testRegisteredResource( + "resource-1", + "documents", + testRegisteredResourceValue( + "prod", + testActionAttributeValue( + legacyCustomAction.GetId(), + legacyCustomAction.GetName(), + testAttributeValue("https://example.com/attr/classification/value/secret", targetNamespace), + ), + testActionAttributeValue( + legacyReadAction.GetId(), + legacyReadAction.GetName(), + testAttributeValue("https://example.com/attr/classification/value/restricted", targetNamespace), + ), + ), + ) + + handler := &plannerTestHandler{ + actionsByNamespace: map[string]*actions.ListActionsResponse{ + "": { + ActionsStandard: []*policy.Action{legacyReadAction}, + ActionsCustom: []*policy.Action{legacyCustomAction}, + Pagination: emptyPageResponse(), + }, + targetNamespace.GetId(): { + ActionsStandard: []*policy.Action{targetReadAction}, + Pagination: emptyPageResponse(), + }, + }, + subjectConditionSetsByNamespace: map[string]*subjectmapping.ListSubjectConditionSetsResponse{ + "": { + SubjectConditionSets: []*policy.SubjectConditionSet{legacySCS}, + Pagination: emptyPageResponse(), + }, + targetNamespace.GetId(): { + Pagination: emptyPageResponse(), + }, + }, + subjectMappingsByNamespace: map[string]*subjectmapping.ListSubjectMappingsResponse{ + "": { + SubjectMappings: []*policy.SubjectMapping{legacyMapping}, + Pagination: emptyPageResponse(), + }, + targetNamespace.GetId(): { + Pagination: emptyPageResponse(), + }, + }, + registeredResourcesByNamespace: map[string]*registeredresources.ListRegisteredResourcesResponse{ + "": { + Resources: []*policy.RegisteredResource{legacyResource}, + Pagination: emptyPageResponse(), + }, + targetNamespace.GetId(): { + Pagination: emptyPageResponse(), + }, + }, + namespacesResponse: &namespaces.ListNamespacesResponse{ + Namespaces: []*policy.Namespace{targetNamespace}, + Pagination: emptyPageResponse(), + }, + } + + planner, err := NewPlanner(handler, "subject-mappings,registered-resources") + require.NoError(t, err) + + plan, err := planner.Plan(t.Context()) + require.NoError(t, err) + + assert.Equal(t, []Scope{ + ScopeActions, + ScopeSubjectConditionSets, + ScopeSubjectMappings, + ScopeRegisteredResources, + }, plan.Scopes) + + require.Len(t, plan.Actions, 2) + assert.ElementsMatch(t, []string{legacyCustomAction.GetId(), legacyReadAction.GetId()}, actionSourceIDs(plan.Actions)) + + actionTargetsBySourceID := make(map[string]*ActionTargetPlan, len(plan.Actions)) + for _, actionPlan := range plan.Actions { + require.NotNil(t, actionPlan) + require.NotNil(t, actionPlan.Source) + require.Len(t, actionPlan.Targets, 1) + actionTargetsBySourceID[actionPlan.Source.GetId()] = actionPlan.Targets[0] + } + + require.Contains(t, actionTargetsBySourceID, legacyCustomAction.GetId()) + assert.Equal(t, TargetStatusCreate, actionTargetsBySourceID[legacyCustomAction.GetId()].Status) + assert.True(t, sameNamespace(targetNamespace, actionTargetsBySourceID[legacyCustomAction.GetId()].Namespace)) + + require.Contains(t, actionTargetsBySourceID, legacyReadAction.GetId()) + assert.Equal(t, TargetStatusExistingStandard, actionTargetsBySourceID[legacyReadAction.GetId()].Status) + assert.Equal(t, targetReadAction.GetId(), actionTargetsBySourceID[legacyReadAction.GetId()].ExistingID) + assert.True(t, sameNamespace(targetNamespace, actionTargetsBySourceID[legacyReadAction.GetId()].Namespace)) + + require.Len(t, plan.SubjectConditionSets, 1) + require.Len(t, plan.SubjectMappings, 1) + require.NotNil(t, plan.SubjectMappings[0].Target) + assert.Equal(t, TargetStatusCreate, plan.SubjectMappings[0].Target.Status) + assert.ElementsMatch(t, []string{legacyCustomAction.GetId(), legacyReadAction.GetId()}, plan.SubjectMappings[0].Target.ActionSourceIDs) + assert.Equal(t, legacySCS.GetId(), plan.SubjectMappings[0].Target.SubjectConditionSetSourceID) + + require.Len(t, plan.RegisteredResources, 1) + require.NotNil(t, plan.RegisteredResources[0].Target) + require.Len(t, plan.RegisteredResources[0].Target.Values, 1) + require.Len(t, plan.RegisteredResources[0].Target.Values[0].ActionBindings, 2) + + var resourceBindingSourceIDs []string + for _, binding := range plan.RegisteredResources[0].Target.Values[0].ActionBindings { + require.NotNil(t, binding) + resourceBindingSourceIDs = append(resourceBindingSourceIDs, binding.SourceActionID) + } + assert.ElementsMatch(t, []string{legacyCustomAction.GetId(), legacyReadAction.GetId()}, resourceBindingSourceIDs) +} + func TestPlannerPlanInvokesInteractiveReviewerWhenConfigured(t *testing.T) { t.Parallel() diff --git a/otdfctl/migrations/namespacedpolicy/registered_resources_execute_test.go b/otdfctl/migrations/namespacedpolicy/registered_resources_execute_test.go index b43ae8bd62..be5133d8f2 100644 --- a/otdfctl/migrations/namespacedpolicy/registered_resources_execute_test.go +++ b/otdfctl/migrations/namespacedpolicy/registered_resources_execute_test.go @@ -237,7 +237,7 @@ func TestExecuteRegisteredResources(t *testing.T) { Target: &RegisteredResourceTargetPlan{ Namespace: namespace1, Status: TargetStatusUnresolved, - Reason: ErrDuplicateCanonicalMatch.Error(), + Reason: "ambiguous target namespace", }, }, }, diff --git a/otdfctl/migrations/namespacedpolicy/resolved.go b/otdfctl/migrations/namespacedpolicy/resolved.go index 7cea70961f..5b09ffe1fe 100644 --- a/otdfctl/migrations/namespacedpolicy/resolved.go +++ b/otdfctl/migrations/namespacedpolicy/resolved.go @@ -18,9 +18,8 @@ type ResolvedTargets struct { } type ResolvedAction struct { - Source *policy.Action - References []*ActionReference - Results []*ResolvedActionResult + Source *policy.Action + Results []*ResolvedActionResult } type ResolvedActionResult struct { @@ -141,9 +140,8 @@ func (r *resolver) resolveAction(derived *DerivedAction) (*ResolvedAction, error } item := &ResolvedAction{ - Source: derived.Source, - References: append([]*ActionReference(nil), derived.References...), - Results: make([]*ResolvedActionResult, 0, len(derived.Targets)), + Source: derived.Source, + Results: make([]*ResolvedActionResult, 0, len(derived.Targets)), } for _, namespace := range derived.Targets { result, err := r.resolveActionTargetFromExisting(derived.Source, namespace) @@ -167,13 +165,9 @@ func (r *resolver) resolveActionTargetFromExisting(source *policy.Action, namesp return r.resolveStandardActionTarget(source, namespace) } - existing, found, err := resolveExistingAction(source, r.existing.CustomActions[namespace.GetId()]) - switch { - case found: + if existing, found := resolveExistingAction(source, r.existing.CustomActions[namespace.GetId()]); found { result.AlreadyMigrated = existing return result, nil - case err != nil: - return nil, err } result.NeedsCreate = true @@ -181,26 +175,12 @@ func (r *resolver) resolveActionTargetFromExisting(source *policy.Action, namesp } func (r *resolver) resolveStandardActionTarget(source *policy.Action, namespace *policy.Namespace) (*ResolvedActionResult, error) { - result := &ResolvedActionResult{Namespace: namespace} - - matches := make([]*policy.Action, 0, 1) for _, action := range r.existing.StandardActions[namespace.GetId()] { - if !actionCanonicalEqual(source, action) { - continue + if actionCanonicalEqual(source, action) { + return &ResolvedActionResult{Namespace: namespace, ExistingStandard: action}, nil } - matches = append(matches, action) - } - - switch len(matches) { - case 1: - result.ExistingStandard = matches[0] - case 0: - return nil, errors.New("matching standard action not found in target namespace") - default: - return nil, errors.New("multiple standard actions match in target namespace") } - - return result, nil + return nil, errors.New("matching standard action not found in target namespace") } func (r *resolver) isStandardAction(action *policy.Action) bool { @@ -258,13 +238,9 @@ func (r *resolver) resolveSubjectConditionSetTargetFromExisting(source *policy.S return nil, fmt.Errorf("%w: empty namespace reference", ErrUndeterminedTargetMapping) } result := &ResolvedSubjectConditionSetResult{Namespace: namespace} - existing, found, err := resolveExistingSubjectConditionSet(source, r.existing.SubjectConditionSets[namespace.GetId()]) - switch { - case found: + if existing, found := resolveExistingSubjectConditionSet(source, r.existing.SubjectConditionSets[namespace.GetId()]); found { result.AlreadyMigrated = existing - case err != nil: - return nil, err - default: + } else { result.NeedsCreate = true } @@ -307,13 +283,9 @@ func (r *resolver) resolveSubjectMapping(derived *DerivedSubjectMapping) (*Resol return nil, fmt.Errorf("subject mapping %q in namespace %q: %w", item.Source.GetId(), item.Namespace.GetId(), err) } - existing, found, err := resolveExistingSubjectMapping(item.Source, r.existing.SubjectMappings[item.Namespace.GetId()]) - switch { - case found: + if existing, found := resolveExistingSubjectMapping(item.Source, r.existing.SubjectMappings[item.Namespace.GetId()]); found { item.AlreadyMigrated = existing - case err != nil: - return nil, fmt.Errorf("subject mapping %q in namespace %q: %w", item.Source.GetId(), item.Namespace.GetId(), err) - default: + } else { item.NeedsCreate = true } @@ -355,13 +327,12 @@ func (r *resolver) resolveRegisteredResource(derived *DerivedRegisteredResource) if item.Namespace == nil { return nil, fmt.Errorf("%w: empty namespace reference", ErrUndeterminedTargetMapping) } - existing, found, err := resolveExistingRegisteredResource(item.Source, r.existing.RegisteredResources[item.Namespace.GetId()]) - switch { - case found: - item.AlreadyMigrated = existing - case err != nil: + if err := r.resolveRegisteredResourceDependencies(item.Source, item.Namespace); err != nil { return nil, fmt.Errorf("registered resource %q in namespace %q: %w", item.Source.GetId(), item.Namespace.GetId(), err) - default: + } + if existing, found := resolveExistingRegisteredResource(item.Source, r.existing.RegisteredResources[item.Namespace.GetId()]); found { + item.AlreadyMigrated = existing + } else { item.NeedsCreate = true } @@ -396,13 +367,12 @@ func (r *resolver) resolveObligationTrigger(derived *DerivedObligationTrigger) ( Source: derived.Source, Namespace: derived.Target, } - existing, found, err := resolveExistingObligationTrigger(item.Source, r.existing.ObligationTriggers[item.Namespace.GetId()]) - switch { - case found: - item.AlreadyMigrated = existing - case err != nil: + if err := r.resolveObligationTriggerDependencies(item.Source, item.Namespace); err != nil { return nil, fmt.Errorf("obligation trigger %q in namespace %q: %w", item.Source.GetId(), item.Namespace.GetId(), err) - default: + } + if existing, found := resolveExistingObligationTrigger(item.Source, r.existing.ObligationTriggers[item.Namespace.GetId()]); found { + item.AlreadyMigrated = existing + } else { item.NeedsCreate = true } @@ -427,116 +397,93 @@ func (r *resolver) resolveSubjectMappingDependencies(mapping *policy.SubjectMapp for _, action := range mapping.GetActions() { actionID := action.GetId() if actionID == "" { - return errors.New("subject mapping references an action without an id") + return ErrMissingActionID } - - result := r.actionResultsByKey[resolvedResultKey(actionID, namespace.GetId())] - if result == nil { - return fmt.Errorf("subject mapping dependency action %q is not resolved in namespace %q", actionID, namespace.GetId()) + if r.actionResultsByKey[resolvedResultKey(actionID, namespace.GetId())] == nil { + return fmt.Errorf("%w: action %q", ErrUnresolvedActionDependency, actionID) } } scsID := mapping.GetSubjectConditionSet().GetId() if scsID == "" { - return errors.New("subject mapping references a subject condition set without an id") + return ErrMissingSubjectConditionSetID + } + if r.scsResultsByKey[resolvedResultKey(scsID, namespace.GetId())] == nil { + return fmt.Errorf("%w: subject condition set %q", ErrUnresolvedSubjectConditionSetDependency, scsID) } - result := r.scsResultsByKey[resolvedResultKey(scsID, namespace.GetId())] - if result == nil { - return fmt.Errorf("subject mapping dependency subject condition set %q is not resolved in namespace %q", scsID, namespace.GetId()) + return nil +} + +func (r *resolver) resolveRegisteredResourceDependencies(resource *policy.RegisteredResource, namespace *policy.Namespace) error { + for _, value := range resource.GetValues() { + for _, aav := range value.GetActionAttributeValues() { + actionID := aav.GetAction().GetId() + if actionID == "" { + return ErrMissingActionID + } + if r.actionResultsByKey[resolvedResultKey(actionID, namespace.GetId())] == nil { + return fmt.Errorf("%w: action %q", ErrUnresolvedActionDependency, actionID) + } + } } + return nil +} +func (r *resolver) resolveObligationTriggerDependencies(trigger *policy.ObligationTrigger, namespace *policy.Namespace) error { + actionID := trigger.GetAction().GetId() + if actionID == "" { + return ErrMissingActionID + } + if r.actionResultsByKey[resolvedResultKey(actionID, namespace.GetId())] == nil { + return fmt.Errorf("%w: action %q", ErrUnresolvedActionDependency, actionID) + } return nil } -func resolveExistingAction(source *policy.Action, existing []*policy.Action) (*policy.Action, bool, error) { - matches := make([]*policy.Action, 0, 1) +func resolveExistingAction(source *policy.Action, existing []*policy.Action) (*policy.Action, bool) { for _, action := range existing { if actionCanonicalEqual(source, action) { - matches = append(matches, action) + return action, true } } - - switch len(matches) { - case 1: - return matches[0], true, nil - case 0: - return nil, false, nil - default: - return nil, false, ErrDuplicateCanonicalMatch - } + return nil, false } -func resolveExistingSubjectConditionSet(source *policy.SubjectConditionSet, existing []*policy.SubjectConditionSet) (*policy.SubjectConditionSet, bool, error) { - matches := make([]*policy.SubjectConditionSet, 0, 1) +func resolveExistingSubjectConditionSet(source *policy.SubjectConditionSet, existing []*policy.SubjectConditionSet) (*policy.SubjectConditionSet, bool) { for _, scs := range existing { if subjectConditionSetCanonicalEqual(source, scs) { - matches = append(matches, scs) + return scs, true } } - - switch len(matches) { - case 1: - return matches[0], true, nil - case 0: - return nil, false, nil - default: - return nil, false, ErrDuplicateCanonicalMatch - } + return nil, false } -func resolveExistingSubjectMapping(source *policy.SubjectMapping, existing []*policy.SubjectMapping) (*policy.SubjectMapping, bool, error) { - matches := make([]*policy.SubjectMapping, 0, 1) +func resolveExistingSubjectMapping(source *policy.SubjectMapping, existing []*policy.SubjectMapping) (*policy.SubjectMapping, bool) { for _, mapping := range existing { if subjectMappingCanonicalEqual(source, mapping) { - matches = append(matches, mapping) + return mapping, true } } - - switch len(matches) { - case 1: - return matches[0], true, nil - case 0: - return nil, false, nil - default: - return nil, false, ErrDuplicateCanonicalMatch - } + return nil, false } -func resolveExistingRegisteredResource(source *policy.RegisteredResource, existing []*policy.RegisteredResource) (*policy.RegisteredResource, bool, error) { - matches := make([]*policy.RegisteredResource, 0, 1) +func resolveExistingRegisteredResource(source *policy.RegisteredResource, existing []*policy.RegisteredResource) (*policy.RegisteredResource, bool) { for _, resource := range existing { if registeredResourceCanonicalEqual(source, resource) { - matches = append(matches, resource) + return resource, true } } - - switch len(matches) { - case 1: - return matches[0], true, nil - case 0: - return nil, false, nil - default: - return nil, false, ErrDuplicateCanonicalMatch - } + return nil, false } -func resolveExistingObligationTrigger(source *policy.ObligationTrigger, existing []*policy.ObligationTrigger) (*policy.ObligationTrigger, bool, error) { - matches := make([]*policy.ObligationTrigger, 0, 1) +func resolveExistingObligationTrigger(source *policy.ObligationTrigger, existing []*policy.ObligationTrigger) (*policy.ObligationTrigger, bool) { for _, trigger := range existing { if obligationTriggerCanonicalEqual(source, trigger) { - matches = append(matches, trigger) + return trigger, true } } - - switch len(matches) { - case 1: - return matches[0], true, nil - case 0: - return nil, false, nil - default: - return nil, false, ErrDuplicateCanonicalMatch - } + return nil, false } func resolvedResultKey(sourceID, namespaceID string) string { diff --git a/otdfctl/migrations/namespacedpolicy/resolved_test.go b/otdfctl/migrations/namespacedpolicy/resolved_test.go index b02c00e81c..f00ec421d9 100644 --- a/otdfctl/migrations/namespacedpolicy/resolved_test.go +++ b/otdfctl/migrations/namespacedpolicy/resolved_test.go @@ -119,11 +119,9 @@ func TestResolveExistingFailsWhenSubjectMappingActionDependencyMissing(t *testin ) require.Error(t, err) assert.Nil(t, resolved) - assert.EqualError( - t, - err, - `subject mapping "mapping-1" in namespace "ns-1": subject mapping dependency action "action-1" is not resolved in namespace "ns-1"`, - ) + require.ErrorIs(t, err, ErrUnresolvedActionDependency) + assert.Contains(t, err.Error(), `subject mapping "mapping-1" in namespace "ns-1"`) + assert.Contains(t, err.Error(), `action "action-1"`) } func TestResolveExistingKeepsRegisteredResourceConflictUnresolved(t *testing.T) { @@ -161,3 +159,732 @@ func TestResolveExistingKeepsRegisteredResourceConflictUnresolved(t *testing.T) ) assert.False(t, resolved.RegisteredResources[0].NeedsCreate) } + +func TestResolveExistingCustomActionReturnsFirstCanonicalMatch(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + existing := newExistingTargets() + // Two canonically-equal custom actions. Pinning first-match-wins here + // guards against a future refactor silently reintroducing ambiguous + // ordering (e.g. switching to map iteration). + existing.CustomActions[namespace.GetId()] = []*policy.Action{ + {Id: "first-match", Name: "decrypt-custom"}, + {Id: "second-match", Name: "DECRYPT-CUSTOM"}, + } + + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeActions}, + Actions: []*DerivedAction{ + { + Source: &policy.Action{Id: "legacy", Name: "decrypt-custom"}, + Targets: []*policy.Namespace{namespace}, + }, + }, + }, + existing, + ) + require.NoError(t, err) + require.Len(t, resolved.Actions, 1) + require.Len(t, resolved.Actions[0].Results, 1) + assert.Equal(t, "first-match", resolved.Actions[0].Results[0].AlreadyMigrated.GetId()) + assert.False(t, resolved.Actions[0].Results[0].NeedsCreate) +} + +func TestResolveExistingStandardActionReturnsFirstCanonicalMatch(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + existing := newExistingTargets() + existing.StandardActions[namespace.GetId()] = []*policy.Action{ + {Id: "first-match", Name: "read", Namespace: namespace}, + {Id: "second-match", Name: "READ", Namespace: namespace}, + } + + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeActions}, + Actions: []*DerivedAction{ + { + Source: &policy.Action{Id: "legacy", Name: "read"}, + Targets: []*policy.Namespace{namespace}, + }, + }, + }, + existing, + ) + require.NoError(t, err) + require.Len(t, resolved.Actions, 1) + require.Len(t, resolved.Actions[0].Results, 1) + assert.Equal(t, "first-match", resolved.Actions[0].Results[0].ExistingStandard.GetId()) + assert.False(t, resolved.Actions[0].Results[0].NeedsCreate) +} + +func TestResolveExistingStandardActionFailsWhenNoMatchInTargetNamespace(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + existing := newExistingTargets() + existing.StandardActions[namespace.GetId()] = []*policy.Action{ + {Id: "non-matching", Name: "write", Namespace: namespace}, + } + + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeActions}, + Actions: []*DerivedAction{ + { + Source: &policy.Action{Id: "legacy", Name: "read"}, + Targets: []*policy.Namespace{namespace}, + }, + }, + }, + existing, + ) + require.Error(t, err) + assert.Nil(t, resolved) + assert.EqualError( + t, + err, + `action "legacy" in namespace "ns-1": matching standard action not found in target namespace`, + ) +} + +func TestResolveExistingRoutesStandardActionsByName(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + sourceName string + }{ + {name: "create", sourceName: "create"}, + {name: "read", sourceName: "read"}, + {name: "update", sourceName: "update"}, + {name: "delete", sourceName: "delete"}, + {name: "uppercase routes to standard path", sourceName: "CREATE"}, + {name: "whitespace-padded routes to standard path", sourceName: " read "}, + {name: "mixed case routes to standard path", sourceName: "UpDaTe"}, + } + + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + existing := newExistingTargets() + // Populating only StandardActions forces this to fail if the + // source is misrouted through the custom-action matcher. + existing.StandardActions[namespace.GetId()] = []*policy.Action{ + {Id: "standard-target", Name: tc.sourceName, Namespace: namespace}, + } + + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeActions}, + Actions: []*DerivedAction{ + { + Source: &policy.Action{Id: "legacy", Name: tc.sourceName}, + Targets: []*policy.Namespace{namespace}, + }, + }, + }, + existing, + ) + require.NoError(t, err) + require.Len(t, resolved.Actions, 1) + require.Len(t, resolved.Actions[0].Results, 1) + assert.Equal(t, "standard-target", resolved.Actions[0].Results[0].ExistingStandard.GetId()) + assert.False(t, resolved.Actions[0].Results[0].NeedsCreate) + }) + } +} + +func TestResolveExistingRoutesStandardActionsByEnumRegardlessOfName(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + existing := newExistingTargets() + // Source name is not one of create/read/update/delete, so routing relies + // entirely on the proto Standard enum to reach the standard-action path. + existing.StandardActions[namespace.GetId()] = []*policy.Action{ + { + Id: "standard-target", + Name: "decrypt", + Value: &policy.Action_Standard{Standard: policy.Action_STANDARD_ACTION_DECRYPT}, + Namespace: namespace, + }, + } + + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeActions}, + Actions: []*DerivedAction{ + { + Source: &policy.Action{ + Id: "legacy", + Name: "decrypt", + Value: &policy.Action_Standard{Standard: policy.Action_STANDARD_ACTION_DECRYPT}, + }, + Targets: []*policy.Namespace{namespace}, + }, + }, + }, + existing, + ) + require.NoError(t, err) + require.Len(t, resolved.Actions, 1) + require.Len(t, resolved.Actions[0].Results, 1) + assert.Equal(t, "standard-target", resolved.Actions[0].Results[0].ExistingStandard.GetId()) +} + +func TestResolveExistingSubjectMappingFailsWhenDerivedTargetIsNil(t *testing.T) { + t.Parallel() + + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeSubjectMappings}, + SubjectMappings: []*DerivedSubjectMapping{ + {Source: &policy.SubjectMapping{Id: "mapping-1"}}, + }, + }, + nil, + ) + require.Error(t, err) + assert.Nil(t, resolved) + require.ErrorIs(t, err, ErrUndeterminedTargetMapping) + assert.EqualError(t, err, "could not determine target namespace: empty namespace reference") +} + +func TestResolveExistingRegisteredResourceFailsWhenSourceIsNil(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeRegisteredResources}, + RegisteredResources: []*DerivedRegisteredResource{ + {Target: namespace}, + }, + }, + nil, + ) + require.Error(t, err) + assert.Nil(t, resolved) + require.ErrorIs(t, err, ErrUndeterminedTargetMapping) + assert.EqualError(t, err, "could not determine target namespace: registered resource is empty") +} + +func TestResolveExistingRegisteredResourceFailsWhenNamespaceIsNil(t *testing.T) { + t.Parallel() + + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeRegisteredResources}, + RegisteredResources: []*DerivedRegisteredResource{ + {Source: &policy.RegisteredResource{Id: "resource-1", Name: "documents"}}, + }, + }, + nil, + ) + require.Error(t, err) + assert.Nil(t, resolved) + require.ErrorIs(t, err, ErrUndeterminedTargetMapping) + assert.EqualError(t, err, "could not determine target namespace: empty namespace reference") +} + +func TestResolveExistingObligationTriggerFailsWhenDerivedTargetIsNil(t *testing.T) { + t.Parallel() + + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeObligationTriggers}, + ObligationTriggers: []*DerivedObligationTrigger{ + {Source: &policy.ObligationTrigger{Id: "trigger-1"}}, + }, + }, + nil, + ) + require.Error(t, err) + assert.Nil(t, resolved) + require.ErrorIs(t, err, ErrUndeterminedTargetMapping) + assert.EqualError(t, err, "could not determine target namespace: empty namespace reference") +} + +func TestResolveExistingSubjectMappingFailsWhenSubjectConditionSetHasNoID(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + // Mapping has no actions (skipping the action dependency loop) and no + // SubjectConditionSet, so GetSubjectConditionSet().GetId() is "". + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeSubjectMappings}, + SubjectMappings: []*DerivedSubjectMapping{ + { + Source: &policy.SubjectMapping{Id: "mapping-1"}, + Target: namespace, + }, + }, + }, + nil, + ) + require.Error(t, err) + assert.Nil(t, resolved) + require.ErrorIs(t, err, ErrMissingSubjectConditionSetID) + assert.Contains(t, err.Error(), `subject mapping "mapping-1" in namespace "ns-1"`) +} + +func TestResolveExistingSubjectMappingFailsWhenSubjectConditionSetNotResolvedInNamespace(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + otherNamespace := &policy.Namespace{ + Id: "ns-2", + Fqn: "https://other.example.com", + } + + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeActions, ScopeSubjectConditionSets, ScopeSubjectMappings}, + Actions: []*DerivedAction{ + { + Source: &policy.Action{Id: "action-1", Name: "decrypt"}, + Targets: []*policy.Namespace{namespace}, + }, + }, + SubjectConditionSets: []*DerivedSubjectConditionSet{ + // SCS is resolved only against otherNamespace, so the mapping's + // lookup against "scs-1|ns-1" comes back nil. + { + Source: &policy.SubjectConditionSet{Id: "scs-1"}, + Targets: []*policy.Namespace{otherNamespace}, + }, + }, + SubjectMappings: []*DerivedSubjectMapping{ + { + Source: &policy.SubjectMapping{ + Id: "mapping-1", + Actions: []*policy.Action{{Id: "action-1", Name: "decrypt"}}, + SubjectConditionSet: &policy.SubjectConditionSet{Id: "scs-1"}, + }, + Target: namespace, + }, + }, + }, + nil, + ) + require.Error(t, err) + assert.Nil(t, resolved) + require.ErrorIs(t, err, ErrUnresolvedSubjectConditionSetDependency) + assert.Contains(t, err.Error(), `subject mapping "mapping-1" in namespace "ns-1"`) + assert.Contains(t, err.Error(), `subject condition set "scs-1"`) +} + +func TestResolveExistingDropsSubjectMappingsOutsideScope(t *testing.T) { + t.Parallel() + + // Scopes omit ScopeSubjectMappings. A mapping that would otherwise fail + // validation (missing SCS id) must be dropped rather than surfaced. + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeActions}, + SubjectMappings: []*DerivedSubjectMapping{ + { + Source: &policy.SubjectMapping{Id: "mapping-1"}, + Target: &policy.Namespace{Id: "ns-1", Fqn: "https://example.com"}, + }, + }, + }, + nil, + ) + require.NoError(t, err) + assert.Empty(t, resolved.SubjectMappings) +} + +func TestResolveExistingDropsRegisteredResourcesOutsideScope(t *testing.T) { + t.Parallel() + + // An empty derived resource would error on nil source if reached; clean + // NoError proves the scope gate drops it before validation. + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeActions}, + RegisteredResources: []*DerivedRegisteredResource{{}}, + }, + nil, + ) + require.NoError(t, err) + assert.Empty(t, resolved.RegisteredResources) +} + +func TestResolveExistingDropsObligationTriggersOutsideScope(t *testing.T) { + t.Parallel() + + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeActions}, + ObligationTriggers: []*DerivedObligationTrigger{ + {Source: &policy.ObligationTrigger{Id: "trigger-1"}}, + }, + }, + nil, + ) + require.NoError(t, err) + assert.Empty(t, resolved.ObligationTriggers) +} + +func TestResolveExistingRegisteredResourceFailsWhenActionDependencyMissingID(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + // AAV references an action with no ID — planner cannot wire the RR create + // to a specific action target, so the plan must fail here rather than + // surface the error at execution time. + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeRegisteredResources}, + RegisteredResources: []*DerivedRegisteredResource{ + { + Source: testRegisteredResource( + "rr-1", + "documents", + testRegisteredResourceValue( + "prod", + testActionAttributeValue("", "decrypt", testAttributeValue("https://example.com/attr/foo/value/bar", namespace)), + ), + ), + Target: namespace, + }, + }, + }, + nil, + ) + require.Error(t, err) + assert.Nil(t, resolved) + require.ErrorIs(t, err, ErrMissingActionID) + assert.Contains(t, err.Error(), `registered resource "rr-1" in namespace "ns-1"`) +} + +func TestResolveExistingRegisteredResourceFailsWhenActionDependencyNotResolvedInNamespace(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + otherNamespace := &policy.Namespace{ + Id: "ns-2", + Fqn: "https://other.example.com", + } + // Action resolves only against otherNamespace, so the RR's lookup at + // "action-1|ns-1" comes back nil — catch this at plan time rather than + // letting the executor fail on a missing action at create time. + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeActions, ScopeRegisteredResources}, + Actions: []*DerivedAction{ + { + Source: &policy.Action{Id: "action-1", Name: "decrypt"}, + Targets: []*policy.Namespace{otherNamespace}, + }, + }, + RegisteredResources: []*DerivedRegisteredResource{ + { + Source: testRegisteredResource( + "rr-1", + "documents", + testRegisteredResourceValue( + "prod", + testActionAttributeValue("action-1", "decrypt", testAttributeValue("https://example.com/attr/foo/value/bar", namespace)), + ), + ), + Target: namespace, + }, + }, + }, + nil, + ) + require.Error(t, err) + assert.Nil(t, resolved) + require.ErrorIs(t, err, ErrUnresolvedActionDependency) + assert.Contains(t, err.Error(), `registered resource "rr-1" in namespace "ns-1"`) + assert.Contains(t, err.Error(), `action "action-1"`) +} + +func TestResolveExistingObligationTriggerFailsWhenActionDependencyMissingID(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeObligationTriggers}, + ObligationTriggers: []*DerivedObligationTrigger{ + { + Source: &policy.ObligationTrigger{ + Id: "trigger-1", + Action: &policy.Action{Name: "decrypt"}, + }, + Target: namespace, + }, + }, + }, + nil, + ) + require.Error(t, err) + assert.Nil(t, resolved) + require.ErrorIs(t, err, ErrMissingActionID) + assert.Contains(t, err.Error(), `obligation trigger "trigger-1" in namespace "ns-1"`) +} + +func TestResolveExistingObligationTriggerFailsWhenActionDependencyNotResolvedInNamespace(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + otherNamespace := &policy.Namespace{ + Id: "ns-2", + Fqn: "https://other.example.com", + } + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeActions, ScopeObligationTriggers}, + Actions: []*DerivedAction{ + { + Source: &policy.Action{Id: "action-1", Name: "decrypt"}, + Targets: []*policy.Namespace{otherNamespace}, + }, + }, + ObligationTriggers: []*DerivedObligationTrigger{ + { + Source: &policy.ObligationTrigger{ + Id: "trigger-1", + Action: &policy.Action{Id: "action-1", Name: "decrypt"}, + }, + Target: namespace, + }, + }, + }, + nil, + ) + require.Error(t, err) + assert.Nil(t, resolved) + require.ErrorIs(t, err, ErrUnresolvedActionDependency) + assert.Contains(t, err.Error(), `obligation trigger "trigger-1" in namespace "ns-1"`) + assert.Contains(t, err.Error(), `action "action-1"`) +} + +func TestResolveExistingReusesAlreadyMigratedSubjectConditionSet(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + + scs := func(id string, values ...string) *policy.SubjectConditionSet { + return &policy.SubjectConditionSet{ + Id: id, + SubjectSets: []*policy.SubjectSet{ + {ConditionGroups: []*policy.ConditionGroup{ + { + Conditions: []*policy.Condition{ + { + SubjectExternalSelectorValue: ".role", + Operator: policy.SubjectMappingOperatorEnum_SUBJECT_MAPPING_OPERATOR_ENUM_IN, + SubjectExternalValues: values, + }, + }, + BooleanOperator: policy.ConditionBooleanTypeEnum_CONDITION_BOOLEAN_TYPE_ENUM_AND, + }, + }}, + }, + } + } + + existing := newExistingTargets() + // Existing SCS differs only in condition-value order — canonical equality + // must match so the resolver routes to AlreadyMigrated instead of create. + existing.SubjectConditionSets[namespace.GetId()] = []*policy.SubjectConditionSet{ + scs("existing-scs", "editor", "admin"), + } + + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeSubjectConditionSets}, + SubjectConditionSets: []*DerivedSubjectConditionSet{ + { + Source: scs("legacy-scs", "admin", "editor"), + Targets: []*policy.Namespace{namespace}, + }, + }, + }, + existing, + ) + require.NoError(t, err) + require.Len(t, resolved.SubjectConditionSets, 1) + require.Len(t, resolved.SubjectConditionSets[0].Results, 1) + result := resolved.SubjectConditionSets[0].Results[0] + require.NotNil(t, result.AlreadyMigrated) + assert.Equal(t, "existing-scs", result.AlreadyMigrated.GetId()) + assert.False(t, result.NeedsCreate) +} + +func TestResolveExistingReusesAlreadyMigratedSubjectMapping(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + sourceSCS := &policy.SubjectConditionSet{ + Id: "scs-1", + SubjectSets: []*policy.SubjectSet{ + {ConditionGroups: []*policy.ConditionGroup{ + { + Conditions: []*policy.Condition{ + { + SubjectExternalSelectorValue: ".role", + Operator: policy.SubjectMappingOperatorEnum_SUBJECT_MAPPING_OPERATOR_ENUM_IN, + SubjectExternalValues: []string{"admin"}, + }, + }, + BooleanOperator: policy.ConditionBooleanTypeEnum_CONDITION_BOOLEAN_TYPE_ENUM_AND, + }, + }}, + }, + } + attributeFQN := "https://example.com/attr/classification/value/secret" + + existing := newExistingTargets() + // Existing SM differs only in case/whitespace on the attribute value FQN and + // action names — canonical equality must still identify it as a match. + existing.SubjectMappings[namespace.GetId()] = []*policy.SubjectMapping{ + { + Id: "existing-mapping", + AttributeValue: &policy.Value{Fqn: " " + attributeFQN + " "}, + Actions: []*policy.Action{{Id: "action-1", Name: "DECRYPT"}}, + SubjectConditionSet: sourceSCS, + }, + } + + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeActions, ScopeSubjectConditionSets, ScopeSubjectMappings}, + Actions: []*DerivedAction{ + { + Source: &policy.Action{Id: "action-1", Name: "decrypt"}, + Targets: []*policy.Namespace{namespace}, + }, + }, + SubjectConditionSets: []*DerivedSubjectConditionSet{ + { + Source: sourceSCS, + Targets: []*policy.Namespace{namespace}, + }, + }, + SubjectMappings: []*DerivedSubjectMapping{ + { + Source: &policy.SubjectMapping{ + Id: "legacy-mapping", + AttributeValue: &policy.Value{Fqn: attributeFQN}, + Actions: []*policy.Action{{Id: "action-1", Name: "decrypt"}}, + SubjectConditionSet: sourceSCS, + }, + Target: namespace, + }, + }, + }, + existing, + ) + require.NoError(t, err) + require.Len(t, resolved.SubjectMappings, 1) + require.NotNil(t, resolved.SubjectMappings[0].AlreadyMigrated) + assert.Equal(t, "existing-mapping", resolved.SubjectMappings[0].AlreadyMigrated.GetId()) + assert.False(t, resolved.SubjectMappings[0].NeedsCreate) +} + +func TestResolveExistingReusesAlreadyMigratedObligationTrigger(t *testing.T) { + t.Parallel() + + namespace := &policy.Namespace{ + Id: "ns-1", + Fqn: "https://example.com", + } + attributeFQN := "https://example.com/attr/classification/value/secret" + obligationFQN := "https://example.com/obligation/notify/value/default" + + existing := newExistingTargets() + // Existing trigger differs only in case and whitespace — canonical equality + // must still identify it as a match. + existing.ObligationTriggers[namespace.GetId()] = []*policy.ObligationTrigger{ + { + Id: "existing-trigger", + Action: &policy.Action{Id: "action-1", Name: "DECRYPT"}, + AttributeValue: &policy.Value{Fqn: " " + attributeFQN + " "}, + ObligationValue: &policy.ObligationValue{Fqn: obligationFQN}, + }, + } + + resolved, err := resolveExisting( + &DerivedTargets{ + Scopes: []Scope{ScopeActions, ScopeObligationTriggers}, + Actions: []*DerivedAction{ + { + Source: &policy.Action{Id: "action-1", Name: "decrypt"}, + Targets: []*policy.Namespace{namespace}, + }, + }, + ObligationTriggers: []*DerivedObligationTrigger{ + { + Source: &policy.ObligationTrigger{ + Id: "legacy-trigger", + Action: &policy.Action{Id: "action-1", Name: "decrypt"}, + AttributeValue: &policy.Value{Fqn: attributeFQN}, + ObligationValue: &policy.ObligationValue{Fqn: obligationFQN}, + }, + Target: namespace, + }, + }, + }, + existing, + ) + require.NoError(t, err) + require.Len(t, resolved.ObligationTriggers, 1) + require.NotNil(t, resolved.ObligationTriggers[0].AlreadyMigrated) + assert.Equal(t, "existing-trigger", resolved.ObligationTriggers[0].AlreadyMigrated.GetId()) + assert.False(t, resolved.ObligationTriggers[0].NeedsCreate) +} diff --git a/otdfctl/migrations/namespacedpolicy/summary_test.go b/otdfctl/migrations/namespacedpolicy/summary_test.go index ebff3c205f..d9f9074a57 100644 --- a/otdfctl/migrations/namespacedpolicy/summary_test.go +++ b/otdfctl/migrations/namespacedpolicy/summary_test.go @@ -382,3 +382,72 @@ func stripANSI(value string) string { tidyWhitespace := regexp.MustCompile(`\x1b\[[0-9;]*m`) return tidyWhitespace.ReplaceAllString(value, "") } + +func TestRecordTargetStatus(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + status TargetStatus + want migrationStatusCounts + }{ + { + name: "existing standard", + status: TargetStatusExistingStandard, + want: migrationStatusCounts{existingStandard: 1}, + }, + { + name: "already migrated", + status: TargetStatusAlreadyMigrated, + want: migrationStatusCounts{alreadyMigrated: 1}, + }, + { + name: "skipped", + status: TargetStatusSkipped, + want: migrationStatusCounts{skipped: 1}, + }, + { + name: "unresolved", + status: TargetStatusUnresolved, + want: migrationStatusCounts{unresolved: 1}, + }, + { + name: "create is a no-op (tracked elsewhere)", + status: TargetStatusCreate, + want: migrationStatusCounts{}, + }, + { + name: "unknown status is a no-op", + status: TargetStatus("wat"), + want: migrationStatusCounts{}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + var counts migrationStatusCounts + recordTargetStatus(&counts, tc.status) + assert.Equal(t, tc.want, counts) + }) + } +} + +func TestRecordTargetStatusAccumulates(t *testing.T) { + t.Parallel() + + // recordTargetStatus is called once per target during summary assembly, so + // counters must accumulate rather than overwrite prior calls. + var counts migrationStatusCounts + recordTargetStatus(&counts, TargetStatusSkipped) + recordTargetStatus(&counts, TargetStatusSkipped) + recordTargetStatus(&counts, TargetStatusAlreadyMigrated) + recordTargetStatus(&counts, TargetStatusUnresolved) + + assert.Equal(t, migrationStatusCounts{ + skipped: 2, + alreadyMigrated: 1, + unresolved: 1, + }, counts) +}