JENKINS-61812 Fix read only password

[timja]

See JENKINS-61812.

Previously N/A was always being set because value wasn't initialised when the j:choose was being used.

Proposed changelog entries

• Distinguish between defined (*****) and undefined (N/A) password on read-only configuration forms for users with Overall/SystemRead or Item/ExtendedRead permissions.

Proposed upgrade guidelines

N/A

Submitter checklist

• JIRA issue is well described
• Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
  • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
• Appropriate autotests or explanation to why this change has no tests
• For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

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

• There are at least 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 does not block the change
• Changelog entries in the PR title and/or Proposed changelog entries are correct
• Proper changelog labels are set so that the changelog can be generated automatically
• If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
• If it would make sense to backport the change to LTS, a JIRA issue should exist and be labeled as lts-candidate

[daniel-beck]

Would be great to have a small test for this. I plan to work in this space in the near future and would rather not break this again.

[oleg-nenashev]

@timja I made some amendments in the changelog entry, PTAL

[timja]

Thanks, looks good

[oleg-nenashev]

I plan to merge it tomorrow if no negative feedback

[daniel-beck]

Would be great to have a small test for this. I plan to work in this space in the near future and would rather not break this again.

@timja I was done with my work earlier than expected, see #4630

Specifically, I don't think a call to #getPasswordValue should be done in readOnlyMode. Could you amend your PR to not do that? The prepared value alone should be enough to distinguish between N/A and **** IMO.

[timja]

Would be great to have a small test for this. I plan to work in this space in the near future and would rather not break this again.

@timja I was done with my work earlier than expected, see #4630

Specifically, I don't think a call to #getPasswordValue should be done in readOnlyMode. Could you amend your PR to not do that? The prepared value alone should be enough to distinguish between N/A and **** IMO.

I was having weird behaviour with this before without the above change I wasn't getting a value at all that I could use to check, I can check after your change? or you could push a commit to this or your change since you're already in the area?

Otherwise I can pick this up later

[daniel-beck]

@timja

I was having weird behaviour with this before without the above change I wasn't getting a value at all that I could use to check, I can check after your change?

Can't you obtain the real value early, but defer the call to h.getPasswordValue until after you've determined whether it's read-only or not? Or did that not work either?

[timja]

@timja

I was having weird behaviour with this before without the above change I wasn't getting a value at all that I could use to check, I can check after your change?

Can't you obtain the real value early, but defer the call to h.getPasswordValue until after you've determined whether it's read-only or not? Or did that not work either?

it's possible to do this, but I think it just adds complexity for no value?

this has the actual value of the secret in plain text:

attrs.value ?: instance[attrs.field]

calling the function just returns the encrypted value.

[timja]

If you still think it's worth it I have a patch that can be applied which works:

Patch

commit 7737299f290302ec5d45bc4d8ca27d75c814fba6 (HEAD -> JENKINS-61812-read-only-password)
Author: Tim Jacomb <timjacomb1+github@gmail.com>
Date:   Sun Apr 19 10:47:28 2020 +0100

    Don't call getPasswordValue until later

diff --git a/core/src/main/resources/lib/form/password.jelly b/core/src/main/resources/lib/form/password.jelly
index 4ec78ef0e4..253130e638 100644
--- a/core/src/main/resources/lib/form/password.jelly
+++ b/core/src/main/resources/lib/form/password.jelly
@@ -61,12 +61,12 @@ THE SOFTWARE.
     </st:attribute>
   </st:documentation>
   <f:prepareDatabinding/>
-  <j:set var="value" value="${h.getPasswordValue(attrs.value ?: instance[attrs.field])}"/>
+  <j:set var="resolvedValue" value="${attrs.value ?: instance[attrs.field]}" />

   <j:choose>
     <j:when test="${readOnlyMode}">
       <j:choose>
-        <j:when test="${!empty(value)}"><span class="jenkins-readonly">****</span></j:when>
+        <j:when test="${!empty(resolvedValue)}"><span class="jenkins-readonly">****</span></j:when>
         <j:otherwise>
           <span class="jenkins-not-applicable">N/A</span>
         </j:otherwise>
@@ -75,6 +75,7 @@ THE SOFTWARE.
     <j:otherwise>
       <j:choose>
         <j:when test="${h.useHidingPasswordFields()}">
+        <j:set var="value" value="${h.getPasswordValue(resolvedValue)}"/>
           <j:choose>
             <j:when test="${ value != null }">
               <st:adjunct includes="lib.form.password.password"/>
@@ -135,7 +136,7 @@ THE SOFTWARE.
           <m:input xmlns:m="jelly:hudson.util.jelly.MorphTagLibrary"
                    class="setting-input ${attrs.checkUrl!=null?'validated ':''}${attrs.clazz}"
                    name="${attrs.name ?: '_.'+attrs.field}"
-                   value="${h.getPasswordValue(attrs.value ?: instance[attrs.field])}"
+                   value="${h.getPasswordValue(resolvedValue)}"
                    type="password"
                    checkMethod="post"
                    ATTRIBUTES="${attrs}" EXCEPT="field clazz value"/>

[daniel-beck]

@timja That looks great and is exactly what I had in mind.

[daniel-beck]

Looks plausible. Untested.

Thanks for amending!

[timja]

Will merge in 24 hours if no negative feedback