[JENKINS-19409] - Added option, which makes plugin to convert all SIDs to the lower-case (fixed merge conflicts) #225
Conversation
Resolves: https://issues.jenkins-ci.org/browse/JENKINS-19409 Signed-off-by: Oleg Nenashev <nenashev@synopsys.com>
There was a problem hiding this comment.
This change completely ignores the fact that the security Realm now defines the IdStrategy (Which did not exist at the time of the first commit). This decides if comparison needs to be done case sensitive or case insensitive. Also the strategy can be different for users and groups so I think this needs to wait until JENKINS-68755 is implemented.
Ideally on a new Jenkins the plugin will use what the IdStrategy is using and only for old instances it will use the current case sensitive behavior, with the option to use the IdStrategy from the security realm.
Missing is also an integration with Configuration as Code probably. This needs to be covered by unit tests.
| @@ -895,6 +948,13 @@ public AuthorizationStrategy newInstance(StaplerRequest req, JSONObject formData | |||
| strategy.assignRole(RoleType.Global, adminRole, getCurrentUser()); | |||
| } | |||
|
|
|||
| // global properties | |||
| if (formData.containsKey("globalProperties")) { | |||
There was a problem hiding this comment.
use GLOBAL_PROPERTIES_NODE once moved
| @@ -623,6 +659,9 @@ public void doGetMatchingAgents(@QueryParameter(required = true) String pattern, | |||
| * </p> | |||
| */ | |||
| public static class ConverterImpl implements Converter { | |||
|
|
|||
| private static final String GLOBAL_PROPERTIES_NODE = "globalProperties"; | |||
There was a problem hiding this comment.
Move that to the outer class so it can be used also in the descriptor
| public RoleBasedAuthorizationStrategy(Map<String, RoleMap> grantedRoles, RoleStrategyProperties prop) { | ||
| RoleMap map = grantedRoles.get(SLAVE); | ||
| agentRoles = map == null ? new RoleMap() : map; | ||
|
|
||
| map = grantedRoles.get(GLOBAL); | ||
| globalRoles = map == null ? new RoleMap() : map; | ||
|
|
||
| map = grantedRoles.get(PROJECT); | ||
| itemRoles = map == null ? new RoleMap() : map; |
There was a problem hiding this comment.
duplicated code with other constructor. The former should be deprecated and just forward to this one
|
Thanks for the quick code review! Is there any specific timeline for integrating JENKINS-68755? I'll check on the review comments and on the mentioned IdStrategy class. Same applies for the JCasC implementation and unit tests. |
|
closed in favor of #332 |
Hello,
this is based on PR #5 from 2013/15 and should resolve the merge conflicts. Just checked that everything is working as expected.
Right now I still get two checkstyle warnings:
// Edit: Fixed that for now by Suppressing the warnings for this single
getACL()function. In addition foreffectiveSIDsimply changed that for now toeffectiveSid. Most things should be fixed now, any feedback on how to properly implement unit / integration tests for this feature?Feedback highly welcome since I'm not completely in Jenkins plugin development.
Cheers,
Chris