Repository navigation
[JENKINS-74964] Display error message when adding invalid certificate credentials - #580
Conversation
…icate credentials
f7447bf to
632611f
Compare
| .catch((e) => { | ||
| // notificationBar.show(...) with logging ID could be handy here? | ||
| console.error("Could not add credentials:", e); | ||
| window.notificationBar.show("Credentials creation failed.", window.notificationBar["ERROR"]); |
There was a problem hiding this comment.
"Add Credentials" popup does not handle HTML responses ("angry Jenkins"), causing the catch block to execute. Added notificationBar to inform the user about the failure(unexpected cases) in credential creation.
…n into JENKINS-74964
| * Descriptor#newInstanceImpl throws a LinkageError if the constructor throws any exception other | ||
| * than HTTP response exceptions such as Descriptor.FormException. |
There was a problem hiding this comment.
AFAIK FormException is not a RuntimeException, and only `RuntimeException are not causing a LinkageError: https://github.com/jenkinsci/jenkins/blob/171097618b5b2b70ec3142e3efc80b4f2aff9601/core/src/main/java/hudson/model/Descriptor.java#L623 so
| * Descriptor#newInstanceImpl throws a LinkageError if the constructor throws any exception other | |
| * than HTTP response exceptions such as Descriptor.FormException. | |
| * Descriptor#newInstanceImpl throws a LinkageError if the DataBoundConstructor or any DataBoundSetter throw any exception other | |
| * than RuntimeException implementing HttpResponse. |
There was a problem hiding this comment.
While FormException itself is not a RuntimeException, it is wrapped into HttpResponses, which is a RuntimeException,
https://github.com/jenkinsci/stapler/blob/master/core/src/main/java/org/kohsuke/stapler/RequestImpl.java#L691
There was a problem hiding this comment.
no, that's incorrect. As for https://github.com/jenkinsci/jenkins/blob/171097618b5b2b70ec3142e3efc80b4f2aff9601/core/src/main/java/hudson/model/Descriptor.java#L622-L627 the Linkage error only can happen if the caught exception is RuntimeException AND HttpResponse. FormException is not in the catch clause, so it's not caught at all but directly thrown as stated in the method signature. The only way of getting a LinkageError is that the error is a NoSuchMethodException, an InstantiationException, an IllegalAccessException, an InvocationTargetException, or a RuntimeException that is not implementing HttpResponse
There was a problem hiding this comment.
You are correct that FormException is not explicitly in the catch clause of Descriptor#newInstanceImpl. However, my point was that FormException, when thrown, is wrapped into HttpResponses and HttpResponses is a RuntimeException.
Because of this wrapping, it ultimately satisfies the condition of being a RuntimeException and implementing HttpResponse
There was a problem hiding this comment.
Updated the comment
jmdesprez
left a comment
jmdesprez
left a comment
There was a problem hiding this comment.
I agree with previous reviews/comments
When adding or updating CertificateCredentialsImpl with invalid PEM data, Stapler's Descriptor.bindJSON wraps the constructor exception in a LinkageError, causing an unhandled stack trace instead of a user-facing error message. Added LinkageError catch blocks to DomainWrapper.doCreateCredentials() and CredentialsWrapper.doUpdateSubmit(), extracting the root cause (IOException | IllegalArgumentException | GeneralSecurityException) and returning a JSON error response for json requests, matching the pattern established in PR jenkinsci#580 for the popup add-credentials path. When addCredentials returns false, check whether the domain still exists before assuming the credential ID is a duplicate. If the domain was removed between page load and form submit, the error now says 'Domain X does not exist' instead of the misleading 'Credentials with specified ID already exist'. Test validates that creating credentials with invalid PEM returns a JSON error (notificationType=ERROR, not a 500). Closes jenkinsci#971 Signed-off-by: Lohit Kolluri <lohitkolluri@gmail.com>
When adding or updating CertificateCredentialsImpl with invalid PEM data, Stapler's Descriptor.bindJSON wraps the constructor exception in a LinkageError, causing an unhandled stack trace instead of a user-facing error message. Added LinkageError catch blocks to DomainWrapper.doCreateCredentials() and CredentialsWrapper.doUpdateSubmit(), extracting the root cause (IOException | IllegalArgumentException | GeneralSecurityException) and returning a JSON error response for json requests, matching the pattern established in PR jenkinsci#580 for the popup add-credentials path. When addCredentials returns false, check whether the domain still exists before assuming the credential ID is a duplicate. If the domain was removed between page load and form submit, the error now says 'Domain X does not exist' instead of the misleading 'Credentials with specified ID already exist'. Test validates that creating credentials with invalid PEM returns a JSON error (notificationType=ERROR, not a 500). Closes jenkinsci#971 Signed-off-by: Lohit Kolluri <lohitkolluri@gmail.com>
JENKINS-74964
Testing done
Automated Unit Testing
Implemented unit tests to verify that the error messages are returned in the response on form submit
Manual Testing
Popup screen:
Submitter checklist