[#1103] Add upgrade step syncing missing ScriptingService sub-configurations

[vharseko]

Summary

Fixes #1103.

PR #1034 (16.1.1) introduced the OAUTH2_ACCESS_TOKEN_MODIFICATION script context and its default global script in the <Configuration> section of scripting.xml. The SMS only registers that section when the whole service is new, and the upgrade framework (UpgradeServiceSchemaStep → ServiceSchemaModifications) diffs <Schema> only. As a result, instances upgraded from a version that already contained the Scripting Service (e.g. 16.0.6 → 16.1.1) are left without:

• the scriptContext=OAUTH2_ACCESS_TOKEN_MODIFICATION sub-configuration (incl. its engineConfiguration whitelist),
• the globalScripts/d22f9a0c-426a-4466-b95e-d0f125b0d5fa default global script,
• the new context choice values in the ScriptingService schema (attribute modifications are only applied when an UpgradeHelper is registered for the service, and ScriptingService had none).

Meanwhile the new OAuth2 Provider attribute forgerock-oauth2-provider-access-token-modification-script is added on upgrade (via OAuth2ProviderUpgradeHelper) with its default pointing at the missing script. Reading /json/global-config/services?_action=nextdescendents then fails single_choice validation (ScriptChoiceValues finds no script of that context) and the whole Global Services page returns 500.

Changes

• UpgradeScriptingSubConfigsStep (new, depends on UpgradeServiceSchemaStep): reads the tag-swapped bundled scripting.xml via UpgradeServiceUtils.getServiceDefinitions, recursively compares its <GlobalConfiguration> sub-configurations with the Scripting Service global config in the data store, and creates any missing ones. Existing sub-configurations are left untouched (user-tuned engine whitelists are not overwritten). The generic diff also self-heals installations already upgraded to 16.1.1/16.1.2 on their next upgrade, and automatically covers any script contexts added in future versions. Follows the UpgradeEntitlementSubConfigsStep pattern.
• ScriptingServiceHelper (new, registered in serverupgrade.properties): syncs the script context choice values of defaultScriptContext and scriptConfiguration.context with the service definition.
• UpgradeServiceUtils.getServiceDefinitions widened to public so the step in the steps.scripting sub-package can reuse it.
• New report keys in amUpgrade.properties.

Creation order is safe: the defaultScript attribute of scriptContext validates against the ScriptConstants.GlobalScript enum (GlobalOnly=true), not against the data store.

Testing

mvn -o -pl openam-upgrade test
# Tests run: 126, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS

Includes the new UpgradeScriptingSubConfigsStepTest (+ test-scripting.xml resource) verifying that a missing script context (with engine configuration) and a missing default global script are created with the expected attributes, and that the step is not applicable when everything is already configured. The generated upgradesteps.properties orders the new step after UpgradeServiceSchemaStep and ScriptingSchemaStep.

[maximthomas]

Verified the fix end to end against a 16.0.6 → 16.1.x store, and it is correct: the step records exactly two entries in document order (context, then script) and writes them through the same code a fresh install uses (CreateServiceConfig.getAttributeValuePairs → createSubConfigEntry), so nothing is dropped relative to a fresh install. Creating defaultScript before the script exists is safe because AttributeValidator short-circuits under installTime=true, and tag swapping resolves @GlobalAccessTokenModificationScriptId@ on the upgrade path. Step ordering, the single-instance addSubConfig guard, and the cost of re-calling getServiceDefinitions were all checked and are non-issues.

Two things to address before merge: the existence check misses two node types, and the new helper silently reverts an administrator setting.

Existence check is unsound for engineConfiguration and globalScripts (major)

openam-upgrade/src/main/java/org/forgerock/openam/upgrade/steps/scripting/UpgradeScriptingSubConfigsStep.java:113

ServiceConfig.getSubConfig(name) does not return null for every missing sub-config. For an absent entry ServiceConfigImpl.getInstance finds no ATTR_SERVICE_ID and derives the sub-schema id from the sub-config name; it returns null only when parentSS.getSubSchema(name) also fails. Where the name equals its sub-schema name — globalScripts (scripting.xml:426) and engineConfiguration (scripting.xml:252,263,326,380) — it returns a non-null phantom wrapping an absent SMSEntry, so the step records nothing.

Concrete case: an operator who applied the manual ssoadm create-sub-cfg workaround for OAUTH2_ACCESS_TOKEN_MODIFICATION but not its engineConfiguration child upgrades — the script whitelist is never created and the report says nothing is missing. The happy path passes only because context names and script UUIDs differ from their sub-schema names, and the test's mock returns null outright.

SMSUtils is already imported:

            ServiceConfig existingConfig = parentConfig.getSubConfig(name);
            if (!SMSUtils.serviceExists(existingConfig)) {

Helper reverts the administrator's global Default Script Context (minor)

openam-upgrade/src/main/java/org/forgerock/openam/upgrade/helpers/ScriptingServiceHelper.java:49-51

Returning attributeFromNewSchema wholesale makes ServiceSchema.replaceChildNode swap the entire <AttributeSchema> node, including <DefaultValues><Value>POLICY_CONDITION</Value></DefaultValues>. For Global attributes the administrator's configured value is the schema default — SmsGlobalSingletonProvider round-trips it through setAttributeDefaults/getAttributeDefaults. Anyone who changed Global Services → Scripting → Default Script Context has it silently reset, with nothing in the upgrade report. This is the only place the PR regresses existing behaviour.

LoggingUpgradeHelper:52-55 does the same job — adding a choice value to a Global attribute that carries a meaningful default — and preserves it. The non-empty guard matters: updateDefaultValues with an empty set removes the element entirely.

    @Override
    public AttributeSchemaImpl upgradeAttribute(AttributeSchemaImpl attributeToUpgrade,
            AttributeSchemaImpl attributeFromNewSchema) throws UpgradeException {

        if (asSet(attributeToUpgrade.getChoiceValues()).equals(asSet(attributeFromNewSchema.getChoiceValues()))) {
            return null;
        }
        // For Global attributes the administrator's configured value is persisted as <DefaultValues>,
        // so carry it over instead of reverting to the value bundled in the WAR.
        Set<String> existingDefaults = attributeToUpgrade.getDefaultValues();
        if (existingDefaults.isEmpty()) {
            return attributeFromNewSchema;
        }
        return updateDefaultValues(attributeFromNewSchema, existingDefaults);
    }

perform() replays stale state and leaks unchecked exceptions (minor)

openam-upgrade/src/main/java/org/forgerock/openam/upgrade/steps/scripting/UpgradeScriptingSubConfigsStep.java:133-146

UpgradeServices runs every step's initialize() before any perform(), so this list is ~29 steps stale when replayed. If an entry appeared meanwhile, addSubConfig throws ServiceAlreadyExistsException (an SMSException) and the upgrade aborts partway with no rollback. Not reachable today, but the re-check is free. Separately, getSubConfig returning null on line 137 NPEs past the SMSException | SSOException handler, bypassing reportEnd("upgrade.failed").

            for (MissingSubConfig missing : missingSubConfigs) {
                UpgradeProgress.reportStart(AUDIT_NEW_SUB_CONFIG_START, missing.name);
                ServiceConfig parentConfig = globalConfig;
                for (String parentName : missing.parentPath) {
                    parentConfig = parentConfig.getSubConfig(parentName);
                    if (parentConfig == null) {
                        throw new UpgradeException("Missing parent configuration for " + missing.getDisplayName());
                    }
                }
                // The list was captured in initialize(); another step may have created the entry since.
                if (SMSUtils.serviceExists(parentConfig.getSubConfig(missing.name))) {
                    DEBUG.message("Scripting Service configuration {} already exists, skipping", missing.name);
                } else {
                    addSubConfig(parentConfig, missing.node);
                }
                UpgradeProgress.reportEnd("upgrade.success");
            }
        } catch (UpgradeException e) {
            UpgradeProgress.reportEnd("upgrade.failed");
            throw e;
        } catch (Exception e) {
            UpgradeProgress.reportEnd("upgrade.failed");
            DEBUG.error("An error occurred while adding missing Scripting Service configurations", e);
            throw new UpgradeException("Unable to add missing Scripting Service configurations", e);
        }

Nits

• Dead context registration: openam-upgrade/src/main/java/org/forgerock/openam/upgrade/helpers/ScriptingServiceHelper.java:35,42 — both context declarations (scripting.xml:180, :223) are type="single", and AttributeSchemaImpl:620-622 only parses <ChoiceValues> for SINGLE_CHOICE/MULTIPLE_CHOICE/LIST, so getChoiceValues() is null on both sides and the helper can never fire for it. Drop SCRIPT_CONTEXT and trim the class javadoc — only defaultScriptContext does real work.
• Document-wide <GlobalConfiguration> lookup: .../UpgradeScriptingSubConfigsStep.java:105 — getElementsByTagName(...).item(0) takes the first match anywhere in the file instead of navigating Service[@name='ScriptingService']/Configuration/GlobalConfiguration. Harmless today, silently wrong if scripting.xml ever holds a second <Service>.
• Report undercounts: .../UpgradeScriptingSubConfigsStep.java:174,186 — only top-level missing nodes are counted, so engineConfiguration children created by the recursion are never listed; UpgradeScriptingSubConfigsStepTest.java:93 asserts 2 while verifying 3 addSubConfig calls. Needs a separate reporting-only list — adding descendants to missingSubConfigs would double-create, and the report is also rendered pre-upgrade so a perform-time counter will not work.
• ScriptingServiceHelper is untested: there is no openam-upgrade/src/test/java/org/forgerock/openam/upgrade/helpers/ directory at all.
• Fixture misses the fallback branches: openam-upgrade/src/test/resources/test-scripting.xml gives every <SubConfiguration> an id and no priority, so the id = name, priority == null and attributes == null paths are never exercised — the real scripting.xml:252 has an empty <SubConfiguration name="engineConfiguration" id="engineConfiguration"/>, which makes getAttributeValuePairs return null.
• Scope caveat worth a javadoc line: the step only creates missing sub-configurations and never reconciles attributes on existing ones, so a later release adding e.g. a whiteList entry to an existing context's engineConfiguration will not reach upgraded instances.
• Dead lines in a file already being edited: openam-server-only/src/main/webapp/WEB-INF/template/sms/serverupgrade.properties:49-50 — line 47 ends the upgrade.helper continuation, so RestSecurity=...UserSelfServiceHelper parses as a standalone key nothing reads, and services.to.delete is then redefined identically. Delete both lines; do not repair the continuation — UserSelfServiceHelper does not exist in the tree, so re-joining it would raise ClassNotFoundException in populateUpgradeHelpers and assertInitialized() would abort every upgrade.