JEP-224 - Add a Overall/SystemRead permission enabler controlled by the jenkins.security.SystemReadPermission system property for Jenkins 2.222+ + Add a Beta hudson.plugins.extendedread.SystemReadPermission#SYSTEM_READ permission entity for plugin developers who want to use the permission without updating the Jenkins core dependency
#7
Conversation
|
@timja @MarkEWaite Changelog in jenkins-infra/jenkins.io#2899 makes users to assume that this change is released while it is not. Let's spend some time today to get it over the line |
|
I will retrigger CI manually |
|
bet you to it |
| @Initializer(after = InitMilestone.PLUGINS_STARTED) | ||
| public static void enableSystemReadPermission() { | ||
| if (System.getProperty("jenkins.security.SystemReadPermission") == null) { | ||
| SystemReadPermission.SYSTEM_READ.setEnabled(true); |
There was a problem hiding this comment.
NIT: In the current implementation it will be enabling ADMINISTER on older Jenkins core versions. I am not that concerned about that, but maybe it is an overkill
src/main/java/hudson/plugins/extendedread/SystemReadPermission.java
Outdated
Show resolved
Hide resolved
oleg-nenashev
changed the title
Overall/SystemRead permission enabler controlled by the jenkins.security.SystemReadPermission system property for Jenkins 2.222+ + Add a Beta hudson.plugins.extendedread.SystemReadPermission#SYSTEM_READ permission entity for plugin developers who want to use the permission without updating the Jenkins core dependency
|
I'd guess my Release Drafter approach was a bad idea :) |
not without bumping the core version, but it can be tested interactively |
….java Co-Authored-By: Oleg Nenashev <[email protected]>
There was a problem hiding this comment.
I agree with @res0nance's question about testing. Since the feature is already in the weekly, I suggest to do a YOLO merge and to work on test automation as a follow-up.
|
FTR I have tested it manually. It works as expected though it does not do much without follow-up PRs like jenkinsci/jenkins#4520 |
|
Tests I have done:
Will cut the release |
|
|
||
| @Initializer(after = InitMilestone.PLUGINS_STARTED) | ||
| public static void enableSystemReadPermission() { | ||
| if (System.getProperty("jenkins.security.SystemReadPermission") == null) { |
There was a problem hiding this comment.
🤔 Seems like the current condition enables the feature if the property is not set. I think meant to enable the feature if the property was set.
There was a problem hiding this comment.
no this is the expected behaviour, if unset then this plugin will enable it, if set then the administrator has taken control and wants to disable it even though the plugin is installed
There was a problem hiding this comment.
Great sorry for the confusion 🌮
| static { | ||
| Permission systemRead; | ||
| try { // System Read is available starting from Jenkins 2.222 (https://jenkins.io/changelog/#v2.222). See JEP-224 for more info | ||
| systemRead = (Permission) ReflectionUtils.getPublicProperty(Jenkins.get(), "SYSTEM_READ"); |
There was a problem hiding this comment.
Is this ever not just a field? Why go through ReflectionUtils?
There was a problem hiding this comment.
always a field, not sure, this can probably be removed in a follow up PR now and bump to 2.235.x LTS
Downstream of: jenkinsci/jenkins#4149
@batmat what do you think of this living here?
cc @oleg-nenashev
(haven't added a test here as it wouldn't show anything without bumping the jenkins core version)