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-31661] Check the root url (Admin monitor + Wizard) #3082

Merged
merged 28 commits into from Apr 26, 2018

Conversation

@Wadeck
Copy link
Contributor

Wadeck commented Oct 13, 2017

The problem is that the application has some little issues when there is no rootURL configured. Example: JENKINS-47426.
Normally with the Setup Wizard, this should not be the case. But there are some exceptions like development instance, administrator that disabled the wizard or if the administrator simply clear the root URL in the config.

There was already a PR (#1921) for the issue but as the PR went too far in the correction it was not merged (as I understand). This PR focuses only on the monitor part.

See JENKINS-31661.

Proposed changelog entries

  • Add administrative monitor to check the configuration of the root URL
  • Add a new page in the Wizard to let the administrator configure the root URL
  • Add an additional option in HttpResponses for sending an error.

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
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests

Desired reviewers

@reviewbybees @oleg-nenashev @amuniz

Screenshot

root_url_not_set_monitor_message

@Wadeck Wadeck self-assigned this Oct 13, 2017

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Oct 13, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@oleg-nenashev
Copy link
Member

oleg-nenashev left a comment

🐛 The error messages are inversed in the tests. It should be fixed. The rest is fine IMHO.

* Normally this root URL is set during SetupWizard phase, this monitor is there to ensure that behavior.
* Potential exceptions are the dev environment, if someone disable the wizard or
* the administrator put an empty string on the configuration page.
*/

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Oct 15, 2017

Member

@since may be useful here though not strictly required

This comment has been minimized.

@Wadeck

Wadeck Oct 15, 2017

Author Contributor

added

@@ -1,3 +1,4 @@
CompletedInitializationMonitor.DisplayName=Jenkins Initialization Monitor
SecurityIsOffMonitor.DisplayName=Disabled Security
URICheckEncodingMonitor.DisplayName=Check URI Encoding
RootUrlNotSetMonitor.DisplayName=Root URL configured Monitor

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Oct 15, 2017

Member

"not" is missing somewhere, I'd guess

This comment has been minimized.

@Wadeck

Wadeck Oct 15, 2017

Author Contributor

added

assertTrue(StringUtils.isNotBlank(JenkinsLocationConfiguration.get().getUrl()));

RootUrlNotSetMonitor monitor = j.jenkins.getExtensionList(AdministrativeMonitor.class).get(RootUrlNotSetMonitor.class);
assertFalse("Monitor must be activated", monitor.isActivated());

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Oct 15, 2017

Member

"not be activated"

This comment has been minimized.

@Wadeck

Wadeck Oct 15, 2017

Author Contributor

corrected

JenkinsLocationConfiguration.get().setUrl(null);

RootUrlNotSetMonitor monitor = j.jenkins.getExtensionList(AdministrativeMonitor.class).get(RootUrlNotSetMonitor.class);
assertTrue("Monitor must not be activated", monitor.isActivated());

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Oct 15, 2017

Member

"must be activated"

This comment has been minimized.

@Wadeck

Wadeck Oct 15, 2017

Author Contributor

corrected

@Test
@Issue("JENKINS-31661")
public void testWithRootUrl_configured() {
assertTrue(StringUtils.isNotBlank(JenkinsLocationConfiguration.get().getUrl()));

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Oct 15, 2017

Member

The test relies on the default JTH behavior. Maybe it makes sense to mention it

This comment has been minimized.

@Wadeck

Wadeck Oct 15, 2017

Author Contributor

good point :)

@oleg-nenashev
Copy link
Member

oleg-nenashev left a comment

🐝

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Oct 15, 2017

@reviewbybees not do-ne

edit: to show it in PR list to review

@daniel-beck daniel-beck self-requested a review Oct 15, 2017

@daniel-beck daniel-beck self-assigned this Oct 19, 2017

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented Oct 20, 2017

I just tried with the PR build: Going through the setup wizard leaves Jenkins without a defined root URL.

While I think this admin monitor is a good idea, Jenkins should not be showing warnings (other than "newer Jenkins is available") to someone who just went through the setup wizard.

For that reason, I strongly disagree with merging this change as is. The setup wizard should be extended to allow defining a Jenkins URL (or could perhaps infer one from the URL used to access Jenkins in the setup wizard, similar to how the form field determines its default value?).

Only then would this change make sense.

@daniel-beck
Copy link
Member

daniel-beck left a comment

See previous comment; this does not make sense if the setup wizard leaves the root URL undefined.

@Wadeck

This comment has been minimized.

Copy link
Contributor Author

Wadeck commented Oct 25, 2017

Werk in progress for the modification in the SetupWizard flow. Under the panel which contain the admin user creation form, I added the new field for RootURL.

Screenshot of the initial proposed version

with_rootfield

@Wadeck

This comment has been minimized.

Copy link
Contributor Author

Wadeck commented Oct 26, 2017

@kzantow do you have time to review those modifications ?
@daniel-beck as requested, the field is proposed during setup, so if you finish the Setup, the root URL will be set.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Oct 26, 2017

@Wadeck After "werk in progress" I rather expected to see a German translation :D

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Oct 26, 2017

I am a bit aware about the "Proceed as current admin" hyperlink. If I click this button, will the local URL change be applied?

P.S: I would be happy to remove this button at all. It causes too many issues with unintended clicks

@Wadeck

This comment has been minimized.

Copy link
Contributor Author

Wadeck commented Oct 26, 2017

@oleg-nenashev Yes, I put my "save action" for both the skip and save first user. And as it's a button and not a link, no risk to see anybody doing some ctrl+click or right-click open in a new tab, etc. So normally it should be called all the time.

@jglick jglick self-requested a review Oct 26, 2017

@jglick jglick dismissed stale reviews from oleg-nenashev and daniel-beck Oct 27, 2017

Predates major changes.

@jglick

jglick approved these changes Oct 27, 2017

@Override
public boolean isActivated() {
JenkinsLocationConfiguration loc = JenkinsLocationConfiguration.get();
return loc == null || loc.getUrl() == null;

This comment has been minimized.

@jglick

jglick Oct 27, 2017

Member

#2935 forces the null check (so #3021 does not save us here), but if it is null, we should just do nothing because Jenkins has not even started up yet anyway. Thus

return loc != null && loc.getUrl() == null;

This comment has been minimized.

@Wadeck

Wadeck Oct 30, 2017

Author Contributor

thank you for the information

Jenkins j = Jenkins.getInstance();
j.checkPermission(Jenkins.ADMINISTER);

String rootUrl = req.getParameter("rootUrl");

This comment has been minimized.

@jglick

jglick Oct 27, 2017

Member

Just replace the current method parameters with

@QueryParameter String rootUrl

This comment has been minimized.

@Wadeck

Wadeck Oct 30, 2017

Author Contributor

nice feature 👍

@@ -44,6 +51,29 @@
<script>
$('username').focus();
</script>
</form>
<form action="${rootURL}/setupWizard/configureRootUrl" class="root-url no-json" method="post">

This comment has been minimized.

@jglick

jglick Oct 27, 2017

Member

saveRootUrl specifies the URL to post to, so I suspect action and method could just be deleted.

This comment has been minimized.

@Wadeck

Wadeck Oct 30, 2017

Author Contributor

removed

Create\ First\ Admin\ User=Create First Admin User
Other\ Configuration=Other Configuration
Jenkins\ URL=Jenkins URL

This comment has been minimized.

@jglick

jglick Oct 27, 2017

Member

trailing newlines please

This comment has been minimized.

@Wadeck

Wadeck Oct 30, 2017

Author Contributor

corrected (also for FR translation)

@Test
@Issue("JENKINS-31661")
public void testWithRootUrl_notConfigured() {
JenkinsLocationConfiguration.get().setUrl(null);

This comment has been minimized.

@jglick

jglick Oct 27, 2017

Member

Could be merged into the preceding test, save a Jenkins startup.

This comment has been minimized.

@Wadeck

Wadeck Oct 30, 2017

Author Contributor

done

@oleg-nenashev
Copy link
Member

oleg-nenashev left a comment

I do not see any obvious issues in the code

@dwnusbaum
Copy link
Member

dwnusbaum left a comment

Small comments about some of the wording but looks fine to me in general.

I am somewhat against adding a new dependency but if new URL(input).toURI() is totally inadequate then I am ok with it.

import java.net.URL;

public class UrlHelper {
public static boolean isValid(String url){

This comment has been minimized.

@dwnusbaum

dwnusbaum Apr 2, 2018

Member

If something basic like the following catches most potential errors then I would be inclined to use it instead of adding a new dependency. Either way I think this class should be @Restricted(NoExternalUse.class).

public static String isValidUrl(String url) {
    try {  
        URL url = new URL(url);
        url.toURI();
    } catch (MalformedURLException | URISyntaxException e) {  
        return e.getMessage();  
    }
    return null;
}

Since this is really just intended to help the admin so I would err on the side of allowing invalid URLs rather than disallowing valid URLs.

This comment has been minimized.

@Wadeck

Wadeck Apr 3, 2018

Author Contributor

Annotation added.

For the second point, actually only the Wizard and the admin monitors are asking for valid URL. But in case you have a very weird URL that is not considered valid for this helper, you can still use it in the configuration page, no check are done there. You will "just" have to disable the admin monitor.

*
* @since TODO
*/
public static HttpResponse errorJSON(@Nonnull String message, @Nonnull Map<?,?> data) {

This comment has been minimized.

@dwnusbaum

dwnusbaum Apr 2, 2018

Member

nit: Since you are making this public I think it would make sense to go ahead and add methods for the other JSONObjectResponse constructors (JSONObject and JSONArray) for consistency with okJSON. Probably better to handle in another PR though.

This comment has been minimized.

@Wadeck

Wadeck Apr 3, 2018

Author Contributor

See #3379

This comment has been minimized.

@daniel-beck

daniel-beck Apr 4, 2018

Member

Needs corresponding proposed changelog entry.

# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
# THE SOFTWARE.
SetupWizard_ConfigureInstance_ValidationErrors=Some data are not valid, please look at the error messages close to the concerned fields

This comment has been minimized.

@dwnusbaum

dwnusbaum Apr 2, 2018

Member

I might reword this from "Some data are not valid, please look at the error messages close the concerned fields" to "Some settings are invalid. See the error messages for details". Then I would get rid of the next message and just reuse this one.

SetupWizard_ConfigureInstance_ValidationErrors=Some data are not valid, please look at the error messages close to the concerned fields
SetupWizard_ConfigureInstance_ProcessingErrors=Some data were not processed correctly, please look at the error messages close to the concerned fields
SetupWizard_ConfigureInstance_RootUrl_Empty=The URL cannot be empty
SetupWizard_ConfigureInstance_RootUrl_Invalid=The URL is invalid, please ensure you are using http:// or http:// with a valid domain.

This comment has been minimized.

@dwnusbaum

dwnusbaum Apr 2, 2018

Member

I think you are missing an s in one of the http:// references.

<dependency>
<groupId>commons-validator</groupId>
<artifactId>commons-validator</artifactId>
<version>1.6</version>

This comment has been minimized.

@daniel-beck

daniel-beck Apr 4, 2018

Member

Could we get this without adding a dependency that will be available to dependent plugins? E.g. integrate the relevant class into Jenkins with @Restricted?

This comment has been minimized.

@daniel-beck

daniel-beck Apr 5, 2018

Member

To clarify, this really should be addressed.

This comment has been minimized.

@Wadeck

Wadeck Apr 6, 2018

Author Contributor

👍 addressed, 4 classes included with @Restricted

*
* @since TODO
*/
public static HttpResponse errorJSON(@Nonnull String message, @Nonnull Map<?,?> data) {

This comment has been minimized.

@daniel-beck

daniel-beck Apr 4, 2018

Member

Needs corresponding proposed changelog entry.

* Please DO NOT ADD ITEM TO THIS LIST. <p>
* If you add an item here, the deserialization process will break
* because it is used for serialized state like "jenkins.install.InstallState$4"
* before the change from anonymous class to named class. If you need to add a new InstallState, you can just add a new inner named class but nothing to change in this list.

This comment has been minimized.

@daniel-beck

daniel-beck Apr 4, 2018

Member

CC @jglick for confirmation, looks good but to be sure.

<!--
The MIT License
Copyright (c) 2018, CloudBees, Inc.

This comment has been minimized.

@daniel-beck

daniel-beck Apr 4, 2018

Member

Not actually true?

This comment has been minimized.

@Wadeck

Wadeck Apr 4, 2018

Author Contributor

I think I added the header to the wrong file -.-

@@ -0,0 +1,101 @@

This comment has been minimized.

@daniel-beck

daniel-beck Apr 4, 2018

Member

Missing license header.

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented Apr 4, 2018

Manually tested behavior looks good.

I'm on-holding this for another week though due to the upcoming core security update. I don't think the risk here is huge, but it's still a substantial change.

@daniel-beck daniel-beck added the on-hold label Apr 4, 2018

obsolete

@kuisathaverat
Copy link
Contributor

kuisathaverat left a comment

LGTM

@dwnusbaum
Copy link
Member

dwnusbaum left a comment

Looks good to me. Maybe the imported classes could be moved into a different package since jenkins.org.blah is a little weird but I don't feel strongly about it.

@Wadeck

This comment has been minimized.

Copy link
Contributor Author

Wadeck commented Apr 9, 2018

Maybe the imported classes could be moved into a different package...

@dwnusbaum it was based on the same concept as used in hudson/org/apache/tools/tar/TarInputStream.java

@daniel-beck daniel-beck removed the on-hold label Apr 10, 2018

@daniel-beck daniel-beck self-requested a review Apr 10, 2018

@Wadeck

This comment has been minimized.

Copy link
Contributor Author

Wadeck commented Apr 12, 2018

@daniel-beck are we good with this one ?

@daniel-beck

This comment has been minimized.

Copy link
Member

daniel-beck commented Apr 17, 2018

To be merged later this week.

@dwnusbaum

This comment has been minimized.

Copy link
Member

dwnusbaum commented Apr 19, 2018

@daniel-beck daniel-beck merged commit 17b94a0 into jenkinsci:master Apr 26, 2018

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@dwnusbaum dwnusbaum referenced this pull request May 4, 2018

Merged

[JENKINS-51064] Lenient URL validator for Root URL #3415

3 of 3 tasks complete
$('root-url').focus();
(function setInitialRootUrlFieldValue(){
var iframeUrl = window.location.href;
var iframeRelativeUrl = '/setupWizard/setupWizardConfigureInstance';

This comment has been minimized.

@jglick

jglick Jun 1, 2018

Member

Wrong! This strips the trailing slash, which is required. JENKINS-51660

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Jun 1, 2018

@jglick a follow-up PR would be much appreciated

@Wadeck

This comment has been minimized.

Copy link
Contributor Author

Wadeck commented Jun 1, 2018

Due to Jenkins:2247 for read and JenkinsLocationConfiguration:123 for write it does not seem to have a real impact.

Related change: #3474

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.