Skip to content

Stop 2FA override leaking from definition spec#23952

Open
myabc wants to merge 1 commit into
release/17.6from
bug/settings-definition-spec-plugin-override-leak
Open

Stop 2FA override leaking from definition spec#23952
myabc wants to merge 1 commit into
release/17.6from
bug/settings-definition-spec-plugin-override-leak

Conversation

@myabc

@myabc myabc commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

What are you trying to accomplish?

A test-isolation leak in spec/constants/settings/definition_spec.rb intermittently fails unrelated request specs (e.g. modules/backlogs/spec/requests/backlogs/sprints_spec.rb) with 401/302, depending on suite ordering. It surfaced on the CI for #23938 but is independent of that change.

The two "with definitions from plugins" examples call override_value directly on the shared plugin_openproject_two_factor_authentication definition instance. override_value mutates the object in place (enforced: true), and the "with settings reset" context only snapshots/restores Settings::Definition.all shallowly — so the mutation survives into later examples. Any spec that performs a real post signin_path login afterwards is then forced into 2FA, never completes login, and fails.

What approach did you choose and why?

  • Operate on a dup of the definition before calling override_value, so the shared instance retained by the snapshot is never mutated. coerce/value= already reassign (deep_merge returns a new hash), so the dup is fully independent.

Merge checklist

  • Added/updated tests
  • Tested major browsers (Chrome, Firefox, Edge, ...) — N/A (test-only change)

The two "with definitions from plugins" examples called
`override_value` directly on the shared
`plugin_openproject_two_factor_authentication` definition instance.
The "with settings reset" context snapshots `@all` shallowly, so the
in-place mutation (2FA `enforced: true`) survived the restore and
leaked globally. Any spec performing a real `post signin_path` login
later in the same process (e.g. backlogs sprints request specs) was
then forced into 2FA, never completing login, and failed with 401.

Operates on a `dup` of the definition so `override_value` no longer
mutates the instance retained by the snapshot.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes an intermittent test-isolation leak in spec/constants/settings/definition_spec.rb by ensuring the spec no longer mutates the shared plugin_openproject_two_factor_authentication settings definition instance when exercising ENV overrides. This prevents later specs from inheriting an unintended “2FA enforced” configuration due to shallow snapshot/restore behavior.

Changes:

  • Duplicate (dup) the plugin settings definition before calling the private override_value helper, so the shared definition in Settings::Definition.all is not mutated.
  • Update expectations to assert against the duplicated definition’s overridden value.
  • Add clarifying comments documenting why the dup is required (shallow restore of the settings snapshot).

@myabc myabc marked this pull request as ready for review June 25, 2026 22:34
@github-actions

Copy link
Copy Markdown

Warning

This pull request does not link an OpenProject work package.

Please add a link to the work package in the description, or reference it in the
title in square brackets, e.g. [SLUG-123] My title here.

@myabc myabc added bugfix ruby Pull requests that update Ruby code needs review flaky-spec Addresses a Flaky Spec ci labels Jun 25, 2026
@github-actions

Copy link
Copy Markdown

Warning

Flaky specs

  • rspec ./modules/wikis/spec/features/admin/internal_provider_spec.rb[1:1]
🤖 Ask Copilot to investigate

Copy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer.

@copilot The following spec(s) are flaky in CI (first seen on PR #23952, linked for reference only):

- `rspec ./modules/wikis/spec/features/admin/internal_provider_spec.rb[1:1]`

Treat this as a standalone task, unrelated to PR #23952. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #23952 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix ci flaky-spec Addresses a Flaky Spec needs review ruby Pull requests that update Ruby code

Development

Successfully merging this pull request may close these issues.

2 participants