[JENKINS-73404] SecretTextArea missing FormValidation support #9450

jtnord · 2024-07-10T13:43:53Z

FormValidation was not working with SecretTextArea, this change fixes that and at the same time simplifies how the SecretTextArea is handled.

There is a change in behaviour to note: null Secrets will end up being an empty Secret (that is one with the empty String "").
As there is no functionality to clear a SecretTextArea other than to set the value in the Textarea to the empty String code should already handle this case, as such it should not break any production code.
This behaviour is also the same as currently happens for f:password and f:textarea

Screenshots

empty (null) value with validation

populated value with validation

empty (null) value after clicking Add

populated value after clicking Replace

Testing done

Added new Unit test in FormFieldValidatorTest
Ran the unit test in the debugger and stopped after the job was created in a breakpoint
- checked that the SecretTextArea works as expected (similar to today) by clicking add to set a credential and update to replace an existing credential.
- Saved a form with updated and missing values and inspected the config.xml and decrypted the secrets to check they had been set/updated correctly
- configured and saved again without updating any data, validated the secrets where still using the values saved in the previous save

Proposed changelog entries

Form validation now works for `SecretTextArea fields.

Proposed upgrade guidelines

N/A

Submitter checklist

Give feedback

The Jira issue, if it exists, is well-described.
The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples). Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
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 TODO Javadocs, as appropriate.
New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
For dependency updates, there are links to external changelogs and, if possible, full differentials.
For new APIs and extension points, there is a link to at least one consumer.
Options

Desired reviewers

N/A

Before the changes are marked as ready-for-merge:

Maintainer checklist

Give feedback

There are at least two (2) approvals for the pull request and no outstanding requests for change.
Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
Proper changelog labels are set so that the changelog can be generated automatically.
If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).
Options

reworked the jelly and simplified the javascript. the textarea is now always present in the DOM, so no need to remove inputs to add a textarea. Rather we just remove /hide the "hidden" attribute from the elements we want to show/hide and give the textArea the expected class and attributes for validation to occur

jtnord · 2024-07-10T17:09:30Z

core/src/main/resources/lib/form/secretTextarea.jelly

@@ -56,17 +56,32 @@ Example usage:
        <st:attribute name="placeholder">
            Placeholder text for input field when displayed.
        </st:attribute>
+        <st:attribute name="checkUrl">


there is no use=optional here and this was copied from other jelly files. placeholder also does not have it, so I can only conclude that it is unused?
if this is used then I would suggest that it is fixed in all Jelly files at once rather than piecemeal which is not the realm of this PR.

core/src/main/resources/lib/form/secretTextarea.jelly

jtnord · 2024-07-10T17:14:34Z

core/src/main/resources/lib/form/secretTextarea/secret.css

+.secret * [hidden] {
+  display: none !important;
+}


despite the elements having the hidden attributes they were continuing to show up.
I did not need !important (they where hidden without it) not did I try and track down exactly what CSS rule was matching so that they did show up.

whilst !important is probably bad form - anyone setting a display on something with a hidden attribute is probably a bad actor that would need to be fixed. In any case hidden here means hidden.

jtnord · 2024-07-10T17:15:43Z

core/src/main/resources/lib/form/secretTextarea/secret.js

all the elements are now always in the dom and the textarea is always populated for form binding.

All we do now is manipulate the hidden nature of the various things to show/hide.

jtnord · 2024-07-10T17:16:28Z

test/src/test/java/hudson/util/FormFieldValidatorTest.java

+            return emptyPassword;
+        }
+
+        @DataBoundSetter


setters were not needed for the test, but helped with developer testing so I left them in for the next person that has to debug something in the area

values for SecretTextArea are now always sent. Whilst this is a change in behavior the only way once set to unset is to set the value to the mpty string "" which would still produce a Secret Additionally this matches the behaviour of f:password and f:textarea Given both of the above this change in behaviour should not break any production use cases.

jtnord · 2024-07-11T10:53:24Z

test/src/test/java/lib/form/SecretTextareaTest.java

@@ -144,7 +144,7 @@ private static TestBuilder fromStringWithDescription(String secret, String descr

        @DataBoundConstructor
        public TestBuilder(Secret secret) {
-            this.secret = secret;
+            this.secret = fixEmptySecret(secret);


The test was checking using j.assertEqualDataBoundBeans on the TestBuilders and the new Secret was set to the empty String.
This adapts the TestBuilder to convert an empty secret to null, as it was less intrusive and also the test would still check the description.

scherler

LGTM

MarkEWaite

Thanks very much. I found no issues during interactive testing and I see no issues in the code changes. I plan to run a plugin bill of materials check of the build to confirm that none of the plugins from the BOM have an issue with the change.

jenkinsci/jenkins#9450 with the most recent changes to the master branch merged into it.

MarkEWaite · 2024-07-20T23:08:37Z

The check with the plugin bill of materials completed successfully.

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback.

/label ready-for-merge

…sci#9450) * [JENKINS-73404] unit test * roundtrip as Secrets * [JENKINS-73404] add validation for SecretTextArea reworked the jelly and simplified the javascript. the textarea is now always present in the DOM, so no need to remove inputs to add a textarea. Rather we just remove /hide the "hidden" attribute from the elements we want to show/hide and give the textArea the expected class and attributes for validation to occur * remove unused data-prompt attribute * Adapt getHiddenSecretValue to backend changes * Adapt to Secrets being sent as "" rather than null values for SecretTextArea are now always sent. Whilst this is a change in behavior the only way once set to unset is to set the value to the mpty string "" which would still produce a Secret Additionally this matches the behaviour of f:password and f:textarea Given both of the above this change in behaviour should not break any production use cases. * Remove trailing spaces --------- Co-authored-by: Mark Waite <[email protected]> (cherry picked from commit 81b90e4)

jtnord force-pushed the JENKINS-73404 branch from dfed790 to e166d4a Compare July 10, 2024 13:52

jtnord added web-ui The PR includes WebUI changes which may need special expertise javascript Pull requests that update Javascript code, used by dependency tooling labels Jul 10, 2024

[JENKINS-73404] unit test

96da588

jtnord force-pushed the JENKINS-73404 branch from e166d4a to 96da588 Compare July 10, 2024 14:08

jtnord added 2 commits July 10, 2024 15:55

roundtrip as Secrets

9ca1a96

jtnord marked this pull request as ready for review July 10, 2024 17:04

jtnord requested review from scherler, daniel-beck and a team July 10, 2024 17:04

jtnord changed the title ~~[JENKINS-73404] unit test~~ [JENKINS-73404] SecretTextArea missing FormValidation support Jul 10, 2024

jtnord commented Jul 10, 2024

View reviewed changes

core/src/main/resources/lib/form/secretTextarea.jelly Outdated Show resolved Hide resolved

jtnord commented Jul 10, 2024

View reviewed changes

remove unused data-prompt attribute

a0fb3a3

jtnord marked this pull request as draft July 11, 2024 09:22

jtnord added 2 commits July 11, 2024 11:47

Adapt getHiddenSecretValue to backend changes

3330d9f

jtnord commented Jul 11, 2024

View reviewed changes

scherler approved these changes Jul 11, 2024

View reviewed changes

jtnord marked this pull request as ready for review July 11, 2024 13:49

MarkEWaite added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Jul 19, 2024

Merge branch 'master' into JENKINS-73404

cbed629

MarkEWaite approved these changes Jul 19, 2024

View reviewed changes

MarkEWaite added a commit to MarkEWaite/bom that referenced this pull request Jul 20, 2024

Test add FormValidation support to SecretTextArea

22027e2

jenkinsci/jenkins#9450 with the most recent changes to the master branch merged into it.

MarkEWaite mentioned this pull request Jul 20, 2024

Test add FormValidation support to SecretTextArea jenkinsci/bom#3391

Closed

6 tasks

Merge branch 'master' into JENKINS-73404

4c99855

Remove trailing spaces

8008122

comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jul 20, 2024

MarkEWaite merged commit 81b90e4 into jenkinsci:master Jul 21, 2024
16 checks passed

olamy mentioned this pull request Sep 5, 2024

Upgrade to winstone 6.22 and so Jetty 10.0.24 (branch stable 2.462) #9697

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-73404] SecretTextArea missing FormValidation support #9450

[JENKINS-73404] SecretTextArea missing FormValidation support #9450

jtnord commented Jul 10, 2024 •

edited by MarkEWaite

Loading

Submitter checklist

Maintainer checklist

jtnord Jul 10, 2024 •

edited

Loading

jtnord Jul 10, 2024 •

edited by MarkEWaite

Loading

jtnord Jul 10, 2024

jtnord Jul 10, 2024 •

edited by MarkEWaite

Loading

jtnord Jul 11, 2024 •

edited

Loading

scherler left a comment

MarkEWaite left a comment

MarkEWaite commented Jul 20, 2024

[JENKINS-73404] SecretTextArea missing FormValidation support #9450

[JENKINS-73404] SecretTextArea missing FormValidation support #9450

Conversation

jtnord commented Jul 10, 2024 • edited by MarkEWaite Loading

Testing done

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

jtnord Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

jtnord Jul 10, 2024 • edited by MarkEWaite Loading

Choose a reason for hiding this comment

jtnord Jul 10, 2024

Choose a reason for hiding this comment

jtnord Jul 10, 2024 • edited by MarkEWaite Loading

Choose a reason for hiding this comment

jtnord Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

scherler left a comment

Choose a reason for hiding this comment

MarkEWaite left a comment

Choose a reason for hiding this comment

MarkEWaite commented Jul 20, 2024

jtnord commented Jul 10, 2024 •

edited by MarkEWaite

Loading

jtnord Jul 10, 2024 •

edited

Loading

jtnord Jul 10, 2024 •

edited by MarkEWaite

Loading

jtnord Jul 10, 2024 •

edited by MarkEWaite

Loading

jtnord Jul 11, 2024 •

edited

Loading