Hide CSRF configuration UI when only DefaultCrumbIssuer is available

[AmoghParmar]

Fixes #26012

Testing done

This change was manually verified by running Jenkins locally from source.

Steps:

1. Built Jenkins from the fix-26012-hide-csrf-ui branch.
2. Opened Manage Jenkins → Configure Global Security.
3. Verified that when only DefaultCrumbIssuer is available, the CSRF Protection section is not rendered.
4. Verified that the section is still shown when CSRF is disabled or when a non-default CrumbIssuer is available.

No automated test was added because this change affects a Groovy UI configuration file, and existing test coverage for this specific UI condition is not present.

Screenshots (UI changes only)

Before

CSRF Protection section was always visible even when only DefaultCrumbIssuer was available.

After

CSRF Protection section is hidden when only DefaultCrumbIssuer is available.

Proposed changelog entries

• Hide CSRF configuration section when only the default crumb issuer is available

Proposed changelog category

/label rfe,web-ui

Proposed upgrade guidelines

N/A

Submitter checklist

• The issue, if it exists, is well-described.
• The changelog entries and upgrade guidelines are appropriate for the audience affected by the change and are in the imperative mood.
• There is automated testing or an explanation as to why this change has no tests.
• New public classes, fields, and methods are annotated with @Restricted or have @since Javadocs, as appropriate.
• New deprecations are annotated appropriately, if applicable.
• UI changes do not introduce regressions with Content Security Policy.
• For dependency updates, links to external changelogs are provided, if applicable.
• For new APIs and extension points, there is a link to at least one consumer, if applicable.

Desired reviewers

@jenkinsci/core-pr-reviewers

[welcome]

Yay, your first pull request towards Jenkins core was created successfully! Thank you so much!

A contributor will provide feedback soon. Meanwhile, you can join the chats and community forums to connect with other Jenkins users, developers, and maintainers.

[daniel-beck]

Kinda rude to just take an already assigned issue.

No automated test was added because this change affects a Groovy UI configuration file, and existing test coverage for this specific UI condition is not present.

Time to add a new test then.

Also, please add screenshots of steps 3 and 4. We've recently had an influx of PRs whose submitters made claims that were untrue, so evidence that you actually did these steps would be nice.

[AmoghParmar]

Kinda rude to just take an already assigned issue.

No automated test was added because this change affects a Groovy UI configuration file, and existing test coverage for this specific UI condition is not present.

Time to add a new test then.

Also, please add screenshots of steps 3 and 4. We've recently had an influx of PRs whose submitters made claims that were untrue, so evidence that you actually did these steps would be nice.

@daniel-beck

Apologies for picking up an already assigned issue - I should have checked first.

I’ve now manually verified and attached screenshots for the requested scenarios:

Step 3:
Jenkins started with
-Dhudson.security.csrf.GlobalCrumbIssuerConfiguration.DISABLE_CSRF_PROTECTION=true.
The CSRF section is rendered and shows the expected warning indicating the
configuration is unavailable due to the system property.

Step 4:
With a non-default CrumbIssuer available (Strict Crumb Issuer plugin installed),
the CSRF section is rendered and the dropdown shows multiple options.

Screenshots for steps 3 and 4 are attached above as evidence of verification.
I understand automated coverage is desirable here and will follow up on adding
appropriate test coverage.

[AmoghParmar]

@daniel-beck @MarkEWaite
All CI checks are passing now and the branch is up to date.
Could you please review and advise on next steps?

[daniel-beck]

Could you please review and advise on next steps?

This change looks reasonable as implemented. Otherwise I'm still waiting for

I understand automated coverage is desirable here and will follow up on adding
appropriate test coverage.

[AmoghParmar]

Could you please review and advise on next steps?

This change looks reasonable as implemented. Otherwise I'm still waiting for

I understand automated coverage is desirable here and will follow up on adding
appropriate test coverage.

Thanks for the review and confirmation that the change itself looks good.

I did attempt to add automated coverage, but I’m running into non-trivial Jenkins core test harness issues for this UI/DOM-behavior change (locale-dependent, generated IDs).

Before continuing further, could you please advise what kind of automated coverage would be preferred here? For example, would a focused unit test around ID generation be sufficient, or is a higher-level UI test expected?

Happy to follow the recommended approach.

[daniel-beck]

JenkinsRule tests are always the same locale, so whatever you're doing is probably wrong.

[AmoghParmar]

Hello @daniel-beck
So... I tried writing the test. Then I tried again. Then again. Each time something new broke , wrong module, wrong JUnit version, methods that don't exist anymore. You know, the usual "I have no idea what I'm doing" experience lol.
Since I'm pretty new to writing tests in this codebase, I was honestly a bit lost. But then I found the existing tests like CrumbExclusionTest and DefaultCrumbIssuerTest sitting right there, and it all started to make sense. Used those as a guide and finally got something working!
Just wanted to be upfront about the process. It wasn't pretty but we got there :)

[daniel-beck]

No worries. Thanks for the update. I think you forgot to add the @TestExtension class though?

Also, please don't rebase/force push. That makes it difficult to identify what was already reviewed.

cbcfe18 looks new, but is just 816a3a1.

[AmoghParmar]

No worries. Thanks for the update. I think you forgot to add the @TestExtension class though?

Also, please don't rebase/force push. That makes it difficult to identify what was already reviewed.

cbcfe18 looks new, but is just 816a3a1.

Thanks a lot, @daniel-beck. I had forgotten to include the @TestExtension class in the previous revision.

I’ve now added the DummyCrumbIssuer test extension so that an additional CrumbIssuer descriptor is available during the test execution, and confirmed that the test passes locally.

Apologies as well for the rebase/force push. I understand that it makes the review history harder to follow. I won’t rewrite history on this PR further and will keep future updates incremental.

Please let me know if anything else should be adjusted.

[daniel-beck]

Limit to #csrfSectionShownWhenNonDefaultIssuerConfigured otherwise #csrfSectionShownWhenCsrfProtectionDisabled is unconvincing. I expect that would pass even if you never set the flag.

[daniel-beck]

Should specifically assert the placeholder message, see other comment.

[daniel-beck]

Add trailing newline.

[daniel-beck]

Please also add a test for the default case to ensure that behavior doesn't regress.

[daniel-beck]

Huh?

[AmoghParmar]

Sorry @daniel-beck for the confusion.. The comment was misleading ..
I'm fixing the test now to correctly assert that the CSRF section
is hidden when only DefaultCrumbIssuer is available.