Move proxy configuration form out of pluginManager screens as it is not related

[olamy]

Signed-off-by: Olivier Lamy [email protected]

See JENKINS-72326.

Testing done

Proposed changelog entries

• Move the proxy configuration form to its own screen.

Proposed upgrade guidelines

N/A

Submitter checklist

Beta Give feedback

1. The Jira issue, if it exists, is well-described.
2. 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.
3. There is automated testing or an explanation as to why this change has no tests.
4. New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
5. New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
6. 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).
7. For dependency updates, there are links to external changelogs and, if possible, full differentials.
8. For new APIs and extension points, there is a link to at least one consumer.

Desired reviewers

@mention

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

Maintainer checklist

Beta Give feedback

1. There are at least two (2) approvals for the pull request and no outstanding requests for change.
2. Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
3. Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
4. Proper changelog labels are set so that the changelog can be generated automatically.
5. 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).
6. 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).

[daniel-beck]

While it's a good idea to move the proxy configuration out of the plugin manager, what motivates it getting its own top-level ManagementLink? I would not expect that to be relevant regularly enough to justify the space it'll take up. Why not make it just another option in Configure System?

[jtnord]

I think this should be Jenkins.MANAGE as changing a proxy should not allow escallation of permissions to Administer?

[olamy]

this is a move of the current code. I didn't touch that. (

jenkins/core/src/main/java/hudson/PluginManager.java

Line 1803 in 51c5fa2

jenkins.checkPermission(Jenkins.ADMINISTER);

)
Should it be change?

[jtnord]

Should it be change?

let's not do it here - can be done in a follow up as it may require some thought as to if it can be abused (I do not see a vector to escalate here - but at least it would require a security review)

[jtnord]

does this not need to set readOnly mode so the fields show as disabled when the user misses the permission to change them?

Suggested change

	<j:set var="readOnlyMode" value="${!app.hasPermission(app.MANAGE)}"/>

[olamy]

this mean modifying all config.groovy for ProxyConfiguration. This PR is only about moving the form not really changing how it works

[jtnord]

Is there not something in the setup wizard that lets you setup a proxy if it detects you have no connection, does that not need some changes to call the changed API?

jenkins/core/src/main/resources/jenkins/install/SetupWizard/proxy-configuration.jelly

Lines 14 to 23 in 92aa498

	<div id="proxy-configuration">
	<h1>${%HTTP Proxy Configuration}</h1>
	<f:form method="post" action="/pluginManager/proxyConfigure" name="proxyConfigure">
	<j:scope>
	<j:set var="instance" value="${app.proxy}"/>
	<j:set var="descriptor" value="${app.pluginManager.proxyDescriptor}"/>
	<st:include from="${descriptor}" page="${descriptor.configPage}" />
	</j:scope>
	</f:form>
	</div>

[olamy]

While it's a good idea to move the proxy configuration out of the plugin manager, what motivates it getting its own top-level ManagementLink? I would not expect that to be relevant regularly enough to justify the space it'll take up. Why not make it just another option in Configure System?

Because I find the new icon really cute!

Sounds a good idea to add another option in Configure System. I will change to this.

[olamy]

While it's a good idea to move the proxy configuration out of the plugin manager, what motivates it getting its own top-level ManagementLink? I would not expect that to be relevant regularly enough to justify the space it'll take up. Why not make it just another option in Configure System?

@daniel-beck
done with 37918b9

[olamy]

I wonder if this method should be move this to the class ProxyConfiguration.
I'm tempted but so no strong opinion really

[olamy]

Is there not something in the setup wizard that lets you setup a proxy if it detects you have no connection, does that not need some changes to call the changed API?

jenkins/core/src/main/resources/jenkins/install/SetupWizard/proxy-configuration.jelly

Lines 14 to 23 in 92aa498

	<div id="proxy-configuration">
	<h1>${%HTTP Proxy Configuration}</h1>
	<f:form method="post" action="/pluginManager/proxyConfigure" name="proxyConfigure">
	<j:scope>
	<j:set var="instance" value="${app.proxy}"/>
	<j:set var="descriptor" value="${app.pluginManager.proxyDescriptor}"/>
	<st:include from="${descriptor}" page="${descriptor.configPage}" />
	</j:scope>
	</f:form>
	</div>

The same API will be still here.

[prakash13594]

Changes Updated Proceed

[mawinter69]

Do we know if this affects CasC? I know there is special handling for proxy here in CasC

[mawinter69]

How about naming this ProxyGlobalConfiguration?

[olamy]

There is a class called ProxyConfiguration which already some contains some logic about this.
So the idea was to have a name similar.
Anywat naming it's like color or codestyle or wine. Everybody may have his own taste preferences.
why not another name but someone may not like it or prefer something different,
This will eventually turn into an endless loop and definitely a waste of time.

[Kevin-CB]

Just to let you know, I will try to do a security review of this PR soonish

[timja]

Thanks for this PR.

Two questions (non-blocking):

• The form is quite large, which was okay on it's own page but now on an instance with no plugins it takes up nearly a third of the system configuration page, is it worth hiding it behind a button like the administrative monitors?
• (inline)

[olamy]

• The form is quite large, which was okay on it's own page but now on an instance with no plugins it takes up nearly a third of the system configuration page, is it worth hiding it behind a button like the administrative monitors?

@timja

doesn't bother me too much compared to what you get when you have plenty of plugins installed :)

I'm not quite sure to follow " like the administrative monitors" where is the code for this?

[timja]

• The form is quite large, which was okay on it's own page but now on an instance with no plugins it takes up nearly a third of the system configuration page, is it worth hiding it behind a button like the administrative monitors?

@timja

doesn't bother me too much compared to what you get when you have plenty of plugins installed :)

I'm not quite sure to follow " like the administrative monitors" where is the code for this?

On the system configuration page:

[Kevin-CB]

Apologies for the delay. I've reviewed it and from a security standpoint, it looks fine.

[jtnord]

While it's a good idea to move the proxy configuration out of the plugin manager, what motivates it getting its own top-level ManagementLink? I would not expect that to be relevant regularly enough to justify the space it'll take up. Why not make it just another option in Configure System?

the form is quite large, which was okay on it's own page but now on an instance with no plugins it takes up nearly a third of the system configuration page, is it worth hiding it behind a button like the administrative monitors?

the proxy is unusual as it does not configure Jenkins like the rest of the things in the System page, but really the JVM (it doesn't but its more a JVM thing than a Jenkins thing) which is why in my initial discussion with Olamy I suggested it was there.

You will likely need to configure the proxy to be able to other parts of the Jenkins config (like test connections etc). Whilst you can "apply" or do a 2 step "save" this could also be needed for some security realms hence I still believe this is better as a separate item.

And then we have the discoverability. if it has its own icon it is obvious - it was burried in the plugin section for a while, if we move it and hide it behind a button for users with a few of plugins installed will they easily be able to discover the new location?

I'm not going to die on this hill - but it seems like putting it into the the System page creates its own issues.

[timja]

And then we have the discoverability. if it has its own icon it is obvious - it was burried in the plugin section for a while, if we move it and hide it behind a button for users with a few of plugins installed will they easily be able to discover the new location?

I would expect most users would ctrl+F proxy, whether or not it's behind a button I don't think matters much.

---

I don't think proxy configuration is important enough or distinct enough to get it's own page.
It's in the setup wizard anyway so should be setup in first time setup first.

[olamy]

@timja use some advanced done 8315dd5

[timja]

Thanks, re-tested looks good!

[daniel-beck]

discoverability

I don't think this is any different from any other option in Configure System? The same argument can be made for every option there.

it was burried in the plugin section for a while, if we move it and hide it behind a button for users with a few of plugins installed will they easily be able to discover the new location?

Yes, if we do it like we did for the cloud config (leaving behind a placeholder, #4339 third screenshot).

You will likely need to configure the proxy to be able to other parts of the Jenkins config (like test connections etc).

This argument makes more sense to me. OTOH at least the form allows for it, while otherwise system config needs to be abandoned to set up the proxy first.

Note that

could also be needed for some security realms

is less relevant, security realms are on Configure Global Security, so separate from either. Unless you're saying admins changing security options cannot be expected to visit Configure System beforehand if their environment demands it?

[timja]

/label ready-for-merge

---

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!