Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-45387] Fix validation error displaying in setup wizard's "create first admin" form #3116

Merged
merged 4 commits into from Mar 7, 2018

Conversation

@xxyy
Copy link
Contributor

commented Oct 29, 2017

See JENKINS-45387.

Synopsis

This PR resolves validation errors not being properly displayed in the setup wizard's "create first admin user" form. For that, it creates a new special account creation method in the security realm. In the frontend, it adapts the error handling to work.

Technical issue description

  1. The SetupWizard was sending two responses if the form had validation errors. The first was sent in the security realm's createAccount method, and the second on the null check for the created user. This needed an API change in the security realm because the SetupWizard is forced to send a request (dictated by return type).
  2. The frontend was expecting errors and successes in the same callback. However, the success callback accepts a data (JSON object received) parameter, while the error callback accepts a response (HTTP response object) parameter. This wasn't compatible.
  3. The frontend tried to write to the iframes document directly, which was (apparently) prevented by browsers (Chrome) for XSS protection. (This seems obvious, so I didn't research it further - correct me if I'm wrong!) Therefore, the whole iframe needed to be replaced instead of just the content.
  4. The submit buttons stayed disabled even after receiving the response.

Technical summary

For (1), I extracted the validation logic in the realm into a new private method and the creation logic into another createAccount() that accepts a SignupInfo parameter. This leaves the initial createAccount() , which is used by the 'normal' account creation forms trivial. Therefore, I was able to create a new account creation method for the setup wizard, which throws an exception if invalid data is entered. This allowed me to catch that exception in the wizard and redirect to the HTML page there.

For the frontend issues, I split the response handling into two methods. The success method does the same thing as before, just without the unnecessary parsing and with a slightly more meaningful error message.

The failure method now doesn't check the response for the error class any more and just displays it. The distinction didn't seem to be working, and seemed unnecessary to me. It also replaces the iframe element completely now, since I wasn't able to change its underlying document otherwise. My suspicion is that Chrome (and probably other major browsers) prevents this as XSS protection, which seems completely reasonable to me. Further, that method now resets the disabled attribute on the submit buttons.

Known caveats

  1. Receiving responses from the backend as HTML and putting them into an iframe really doesn't smell good. I don't think that's an XSS issue though, because an adversary could just inject their JS into any other Jenkins page if they can control the templates. I tried to change the response to JSON, but as mentioned, iframe documents (afaik) can't be modified by their parent documents to prevent cross-site scripting. (Maybe there's a header to change that) Changing the response to JSON however would probably require the form to be moved to the frontend, so I didn't bother with that.
  2. I noticed that the 400 Bad Request response code that I'm returning with my HTML response doesn't seem to be consistent with the other responses I've seen in the code. This is however necessary for the distinction between error/success callback.
  3. I haven't seen any neighbouring methods using exceptions to signal errors, so this might be a source of inconsistency. I don't see another way of passing the response/error message to the caller with a User return type though.
  4. I'm a first-time contributor and don't have a full overview of the Jenkins system yet, so it's possible that I'm missing some context. I did conduct a usage search in IntelliJ for existing code that I changed though.

Tests

I have not included automated tests for this change because I frankly don't think it's worth the effort for this type of change. To test the UI would require full-blown UI tests and checking the backend response for the error code would just be repeating the production code in a test. I have manually tested the changes in Chrome and Firefox on Arch Linux though.

To always start the setup wizard, I (locally and temporarily) added the jenkins.install.state=INITIAL_SECURITY_SETUP and jenkins.install.runWizard=true system properties in the jenkins-war pom-xml at build plugins plugin[artifactId='maven-jenkins-dev-plugin'] configuration systemProperties. Also, I short-circuited SetupWizard#isUsingSecurityDefaults() to true using if(true) return true; to save me from having to re-create war/work every startup.

I have not run the full test suite on this and am waiting for the CI to do it for me. When I first set up the project, I accidentally ran it with mvn install and it took about one to two forevers (I aborted it).

Proposed changelog entries

  • bug: [JENKINS-45387]: Resolve setup wizard not displaying form validation errors in "create first admin user" form

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
  • Appropriate autotests or explanation to why this change has no tests

@jenkinsci/hacktoberfest

I'm looking forward to hear what you think!

@@ -323,6 +343,24 @@ private void tryToMakeAdmin(User u) {
* a valid {@link User} object if the user creation was successful.
*/
private User createAccount(StaplerRequest req, StaplerResponse rsp, boolean selfRegistration, String formView) throws ServletException, IOException {
SignupInfo si = validateAccountCreationForm(req, selfRegistration);

if(si.errorMessage!=null) {

This comment has been minimized.

Copy link
@recena

recena Oct 30, 2017

Contributor

I suggest you to follow the same code style: if (si.errorMessage != null) {

This comment has been minimized.

Copy link
@xxyy

xxyy Oct 31, 2017

Author Contributor

I was leaving the existing code alone because it said in one of the many "how to contribute" documents not to include unrelated formatting changes and I guess I was overdoing it :) Will change.

This comment has been minimized.

Copy link
@recena

recena Oct 31, 2017

Contributor

The idea is to modify only what you need. But, if you modify a line where you can apply a code style (according to the rest of the file), my suggestion is to do it.


if(si.errorMessage!=null) {
// failed. ask the user to try again.
req.getView(this, formView).forward(req,rsp);

This comment has been minimized.

Copy link
@recena

recena Oct 30, 2017

Contributor

Ditto req.getView(this, formView).forward(req, rsp);

* @throws IllegalArgumentException if an invalid signup info is passed
*/
private User createAccount(SignupInfo si) throws IOException {
if(si.errorMessage != null) {

This comment has been minimized.

Copy link
@recena

recena Oct 30, 2017

Contributor

Space after if

}
return HttpResponses.okJSON(data);
} catch (AccountCreationFailedException e) {
rsp.setStatus(400);

This comment has been minimized.

Copy link
@recena

recena Oct 30, 2017

Contributor

Why 400? According to what I see AccountCreationFailedException is for validating the input data, right?

This comment has been minimized.

Copy link
@xxyy

xxyy Oct 31, 2017

Author Contributor

Yeah. What other status code would you suggest? According to this Stackoverflow question's answers (and this), 400 may be a viable choice, but that technically indicates a syntax error. However, it seems that most APIs use it for any kind of input error, lacking a better option. 422 also goes in the direction of what we're trying to say, but that's not in the HTTP standard, WebDAV only.

Generally, I think 400 is definitely better than 200 here because the request was not "ok". Ideally, I'd be returning the error message as JSON, but sadly I can't do anything with that on the client since I can't modify the contents of the iframe.

409 also seems to be a possible choice, but that doesn't describe our error better than 400.

This comment has been minimized.

Copy link
@recena

recena Oct 31, 2017

Contributor

@xxyy I agree. What do you think about 422? ref.

This comment has been minimized.

Copy link
@xxyy

xxyy Oct 31, 2017

Author Contributor

@recena We can settle on this, it being not part of the initial HTTP/1.1 probably won't be a problem in practice.

This comment has been minimized.

Copy link
@recena

recena Oct 31, 2017

Contributor

@xxyy To be honest, I'm not 100% sure about what the most used. What do you think about this description? https://httpstatuses.com/422

This comment has been minimized.

Copy link
@xxyy

xxyy Oct 31, 2017

Author Contributor

@recena the description seems like it's more appropriate than the general 400. In my experience, I've usually only seen 400, not 422. But honestly, it doesn't really matter - we know it's a client error so 4xx, and the frontend only cares about the fact that it's > 200. I've already changed it to 422 in case that wasn't clear.

@@ -3,7 +3,7 @@
</div>
<div class="modal-body">
<div class="jumbotron welcome-panel security-panel">
<iframe src="{{baseUrl}}/setupWizard/setupWizardFirstUser"></iframe>
<iframe src="{{baseUrl}}/setupWizard/setupWizardFirstUser" class="setup-first-user"></iframe>

This comment has been minimized.

Copy link
@recena

recena Oct 30, 2017

Contributor

Where is define the CSS class setup-first-user?

This comment has been minimized.

Copy link
@recena

recena Oct 30, 2017

Contributor

If you are using a CSS class as selector, please, use id attribute.

This comment has been minimized.

Copy link
@kzantow

kzantow Oct 30, 2017

Contributor

👎 always use CSS selectors.

This comment has been minimized.

Copy link
@recena

recena Oct 31, 2017

Contributor

@kzantow I'm sorry but I disagree, specially, in that case where you are 100% about the id for this iframe. CSS Classes without semantic only for selecting... 🐼

This comment has been minimized.

Copy link
@xxyy

xxyy Oct 31, 2017

Author Contributor

@recena It does have some meaning, it indicates that this is the iframe where the first user is configured. It's just not used for styling. I wasn't using an id because I'm conditioned to avoid it, plus as I'm dynamically adding it in JS, I don't consider it to be really unique any more. Granted, there normally won't be multiple of this at the same time, but it still doesn't really identify a single element imo.

This comment has been minimized.

Copy link
@xxyy

xxyy Oct 31, 2017

Author Contributor

Although in this case, it is used to identify a single element, so I'll change it now, I can change it back if the discussion results in something else

This comment has been minimized.

Copy link
@recena

recena Oct 31, 2017

Contributor

@xxyy My personal rule is if the element is 100% unique and I'm totally sure, use id. In that case, it seems the iframe will be unique.

} else {
return HttpResponses.okJSON();
User u = securityRealm.createAccountFromSetupWizard(req);
if(admin != null) {

This comment has been minimized.

Copy link
@recena

recena Oct 31, 2017

Contributor

if (admin != null) {

doc.open();
doc.write(res.responseText);
doc.close();
$('button').prop({disabled:false});

This comment has been minimized.

Copy link
@recena

recena Oct 31, 2017

Contributor

Wrong indent.

This comment has been minimized.

Copy link
@xxyy

xxyy Oct 31, 2017

Author Contributor

Sorry, these look all the same for me, and IntelliJ is supposed to detect the current file's indentation and override the project preference, but looks like it didn't here :(

@xxyy xxyy force-pushed the xxyy:pn/jenkins-45387 branch from 4a30b52 to 70a9d15 Oct 31, 2017

@lhoss

This comment has been minimized.

Copy link

commented Nov 27, 2017

Let's get this reviewed, merged.. released 👍

I just ran into the same use

  • installing the latest jenkins LTS 2.73.3 using the docker image
  • selecting all the proposed plugins (except I removed the 'ant plugin')
  • entering ALL the fields, including a correct email (pattern: .@.ch)
Nov 27, 2017 11:20:41 AM hudson.init.impl.InstallUncaughtExceptionHandler$1 reportException
WARNING: null
java.io.IOException: finished
	at com.jcraft.jzlib.DeflaterOutputStream.write(DeflaterOutputStream.java:90)
	at java.io.FilterOutputStream.write(FilterOutputStream.java:97)
	at org.kohsuke.stapler.compression.FilterServletOutputStream.write(FilterServletOutputStream.java:26)
...

Note: No issue on the 2. try, when I retried filling out the empty form, with same values , except this time I also choose just a single word in the full name field (Admin), before I had entered a 3 words name.

@daniel-beck daniel-beck self-requested a review Dec 5, 2017

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Dec 5, 2017

Merge conflict after #3164

* @return
* null if failed. The browser is already redirected to retry by the time this method returns.
* a valid {@link User} object if the user creation was successful.
* @return null if failed. The browser is already redirected to retry by the time this method returns. a valid

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Dec 5, 2017

Member

Line rewrapping broke the list-style "formatting" of the original Javadoc.


/**
* @param req the request to process
* @param selfRegistration whether this account creation was issued by the user themselves

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Dec 5, 2017

Member

The call passes false, probably because there's no captcha in the setup wizard, but still weird.

This comment has been minimized.

Copy link
@xxyy

xxyy Dec 14, 2017

Author Contributor

You're right. I just copied this parameter from the original method.. Turns out it value is either statically false or the class field enableCaptcha. I've renamed it to validateCaptcha in both signatures, and also completed the JavaDoc for the original method while I was at it.

}
return HttpResponses.okJSON(data);
} catch (AccountCreationFailedException e) {
rsp.setStatus(422); // Unprocessable Entity (WebDAV)

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Dec 5, 2017

Member

This status probably needs the explanation currently in the PR comment.

@daniel-beck
Copy link
Member

left a comment

Overall this looks reasonable from a code review.

Would prefer if someone on the team could manually test this before we merge to prevent further messes in the setup wizard area.

@xxyy

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2017

Thanks for the comments @daniel-beck, haven't got to resolve these yet. (tomorrow?) From a quick look at the PR that created the merge conflict, the issue it solves seems quite similar to my one. I'll need to verify whether this PR is still necessary or the other one already resolved the issue.

xxyy added some commits Oct 29, 2017

[JENKINS-45387] Improve setup wizard account creation in security realm
This commit adds an additional account creation method to the
security realm, that allows to create a new user account (as the system)
and is intended to be used by the setup wizard.

The main difference to the existing method is that the new method does
not force the Stapler to send a response, but instead throws an
exception if invalid data is submitted. This allows to call this from
the setup wizard, and send the response there. (This is necessary
because the setup wizard method has a response return type and
there is no way to access the response already send in the realm)

Further, it splits the private createAccount() method for
clarity as well as to allow code reuse from the new method.
[JENKINS-45387] Fix SetupWizard sending responses twice on create admin
This commit fixes an issue where the SetupWizard class would send
two responses (indirectly) when invalid form data was provided for
creating the first admin account.
[Fix JENKINS-45387] Setup wizard not displaying first account errors
This commit fixes the setup wizard not displaying HTML error
responses upon first account creation.

Previously, it just froze (buttons were not re-enabled) and didn't
display responses. (Probably caused by XSS policies on the iframe)

@xxyy xxyy force-pushed the xxyy:pn/jenkins-45387 branch from 70a9d15 to a9a4dde Dec 14, 2017

@xxyy

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2017

Sorry for the delay! After some manual testing, I can confirm that #3164 fixes the frontend issue. However, the stack trace still appeared. I have rebased my commits on master, resolved the remaining code comments and tested the result to make sure the issue is still resolved.

I merged my frontend changes with the fix from #3164 and kept them in because I find it a lot cleaner to have separate callbacks for success and error.

I also squashed the code style commits because the rebase needed a force-push anyways.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Dec 22, 2017

I have some comments, didn't finish the review yet. Will try to do it next week

@oleg-nenashev oleg-nenashev self-assigned this Dec 22, 2017

@oleg-nenashev
Copy link
Member

left a comment

Oops, apparently I have not submitted the review. In December I have tested this PR, reproduced the issue and confirmed the fix.

The code is still reasonable, so I want to merge it. @xxyy my apologies for the delay

@@ -240,49 +241,53 @@ public boolean isUsingSecurityToken() {
* Called during the initial setup to create an admin user
*/
@RequirePOST
public HttpResponse doCreateAdminUser(StaplerRequest req, StaplerResponse rsp) throws IOException, ServletException {
public HttpResponse doCreateAdminUser(StaplerRequest req, StaplerResponse rsp) throws IOException {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 3, 2018

Member

Should be ideally restricted

This comment has been minimized.

Copy link
@xxyy

xxyy Mar 4, 2018

Author Contributor

Sorry, I don't understand what you mean. From what I see, permission is checked in the second line with checkPermission.
If you're referring to the IOException, that very class is actually thrown from the method body.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 4, 2018

Member

I meant adding the @Restricted(NoExternalUse.class) annotation so that we are sure that nobody tries to use this method externally.

Generally your change is binary compatible, so it is fine anyway

This comment has been minimized.

Copy link
@xxyy

xxyy Mar 4, 2018

Author Contributor

Oh, yeah, that sounds reasonable. It's half past 1 am here, so I'll add that tomorrow – or today, depending on how you see it.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Mar 4, 2018

Member

"Today" for me as well :)
It is not a blocker for the merge tho

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Mar 3, 2018

@daniel-beck @fcojfernandez WDYT about merging?

@oleg-nenashev oleg-nenashev requested a review from daniel-beck Mar 3, 2018

xxyy added a commit to xxyy/jenkins that referenced this pull request Mar 4, 2018

*
* @author Philipp Nowak
*/
public class AccountCreationFailedException extends Exception {

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Mar 4, 2018

Member

@Restricted

@daniel-beck
Copy link
Member

left a comment

Code (Java part at least) looks reasonably, but would prefer we got confirmation that the PR build works as advertised.

We don't have the test coverage for the setup wizard we need, as recent security issues showed.

@xxyy xxyy force-pushed the xxyy:pn/jenkins-45387 branch from 038d2b5 to 24ff668 Mar 4, 2018

@fcojfernandez
Copy link
Contributor

left a comment

LGTM. I don't see any issue that prevents the merge

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Mar 5, 2018

How is this related to JENKINS-48080 / #3164 in Jenkins 2.96?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Mar 7, 2018

@daniel-beck the fix you reference has beed created by @fcojfernandez, and that's why I CCed him. IIUC #3164 is just a diagnostics, this is a wider fix for a more generic type of error AFAIK.

@xxyy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2018

@daniel-beck from what I saw when resolving the merge conflicts, it seems to fix the frontend issue (iirc the HTTP response and response DTO were confused when trying to display the error, and there was an issue with iframes). However, the stack trace still appeared (responses were still attempted to be sent twice on error), so this PR is still necessary. I had also done some splitting to the frontend code, which I kept as well. (also see my comment above #3116 (comment))

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Mar 7, 2018

Oops, sorry about that @xxyy @oleg-nenashev I missed the earlier references to that issue.

@oleg-nenashev oleg-nenashev merged commit 12031d7 into jenkinsci:master Mar 7, 2018

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details
@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Mar 7, 2018

Thanks a lot for your contribution @xxyy! It is great to finally have it merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.