[JENKINS-34545] - Support case insensitive user IDs in roles #43

seanmceligot · 2018-08-13T18:51:46Z

This should fix JENKINS-34545 and related issues. It allows case insensitive matching to user/group values for Global and Item roles in Assign Roles.

oleg-nenashev

And it would be a breaking change... I tried to do something similar at some point, bu it appeared that there are Jenkins users who really rely on the current behavior.

I would be fine if the plugin becomes case-insensitive by default, but there should be an option to restore the original behavior

seanmceligot · 2018-08-27T11:33:34Z

OK, I'll look into that.

seanmceligot · 2018-09-10T00:49:16Z

Here's a screenshot of what I have now. I was thinking Assign Roles screen is the most relevant place to put this but let me know what you think. Current it doesn't reload map without restarting Jenkins. I'm looking into that.

KostyaSha · 2018-09-10T17:31:07Z

src/main/java/com/michelin/cio/hudson/plugins/rolestrategy/Role.java

+        bldr.append(name);
+        bldr.append(':');
+        bldr.append(pattern);
+        return bldr.toString();


with String.format() you can do it in one line

oleg-nenashev · 2018-09-10T22:50:39Z

Maybe it makes sense to move the setting to the common Authorization Strategy configuration screen (config.jelly in https://github.com/jenkinsci/role-strategy-plugin/tree/master/src/main/resources/com/michelin/cio/hudson/plugins/rolestrategy/RoleBasedAuthorizationStrategy) and making the setter a @DataBoundSetter. Otherwise this option will immediately become unmanageable from Configuration as Code.

No strong opinion tho, it is a wider problem than this PR

oleg-nenashev

After some consideration, I believe that using standard data binding is required here. JCasC 1.0 has been released recently, and I do not want to introduce compatibility issues

oleg-nenashev · 2018-09-16T11:39:40Z

src/main/java/com/michelin/cio/hudson/plugins/rolestrategy/RoleMap.java

@@ -78,8 +78,10 @@
          .expireAfterWrite(Settings.USER_DETAILS_CACHE_EXPIRATION_TIME_SEC, TimeUnit.SECONDS)
          .build();

+  private final transient boolean caseInsensitiveUser;


I am not sure what is the intention here. With the current logic the flag will be always false after the restart, and adding new roles from API won't behave as expected

The real configuration variable is in RoleBasedAuthorizationStrategy. That one is currently marshed/unmarshed explicitly in that RoleBasedAuthorizationStrategy.ConverterImpl. I just followed what was already there. Does the ConvererImpl entirely need to be converted or do you want the new variable managed elsewhere?

This RoleMap.caseInsensitiveUser is a copy needed to create the TreeSet when RoleMap.addRole is called. I made it transient because I didn't want it stored in config.xml and final to show that it was not the config.xml variable. It loads correctly at startup from config.xml, but I need to recreate all the TreeSets when it's changed realtime.

config.xml

class="com.michelin.cio.hudson.plugins.rolestrategy.RoleBasedAuthorizationStrategy"> <caseInsensitiveUser>true</caseInsensitiveUser>

new variable placement on Configure Global Security page.

daniel-beck · 2018-09-16T11:49:34Z

Why not integrate with case sensitivity of the chosen security realm? Or what am I missing?

reload grantedRoles when caseinsensitive changes

[maven-release-plugin] copy for tag role-strategy-2.9.0

ingwarsw · 2018-11-10T10:55:32Z

@oleg-nenashev Any plans to integrate this..
Its really hard to explain users to login using always the same case..

oleg-nenashev · 2018-11-10T15:58:20Z

No short-term plans. If somebody wants to contribute, happy to share my vision

…

On Sat, Nov 10, 2018, 11:55 Karol Lassak ***@***.*** wrote: @oleg-nenashev <https://github.com/oleg-nenashev> Any plans to integrate this.. Its really hard to explain users to login using always the same case.. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#43 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC3IoDHhx1Rp376vU08IyFaD5VKwVYOfks5utrCogaJpZM4V7CNT> .

seanmceligot · 2018-11-15T23:49:54Z

Just an update. My original fix was two lines lines of code.

master...seanmceligot:case-insensitive-2.9.0

I started to add an configuration option turn it on and off as requested for legacy users, but my change wasn't compatible with configuration as code. I'd need to delve into the CASC framework if I wanted to continue.

…e-strategy-plugin into caseinsensitive

…2.11

…lugin into case-insensitive-2.11

kiran1abc · 2019-06-18T20:08:04Z

@seanmceligot can we expect any progress on this issue in future.

seanmceligot · 2019-06-19T14:15:32Z

@seanmceligot can we expect any progress on this issue in future.

Now should be a good time since configuration as code was merged. Let me look at adding the configuration parameter again and make sure it's CASC compatible.

kiran1abc · 2019-06-19T19:46:57Z

@seanmceligot thanks that helps us a lot.

seanmceligot · 2019-07-13T21:37:24Z

oleg-nenashev · 2019-07-16T00:40:00Z

Thanks @seanmceligot ! It would be really great to get it over the line

res0nance

Not much to add other than there are several code formatting issues and what seems to be excessive logging. hasPermission is called quite often and to prevent polluting logs it might make sense to log it at a finer level.

AbhyudayaSharma

There seems to be quite a big performance penalty of this change apart from formatting issues.

AbhyudayaSharma · 2019-07-16T03:19:03Z

src/main/java/com/michelin/cio/hudson/plugins/rolestrategy/RoleMap.java

@@ -141,7 +158,7 @@ private boolean hasPermission(String sid, Permission permission, RoleType roleTy
    new RoleWalker() {
      public void perform(Role current) {
        if (current.hasAnyPermission(permissions)) {
-          if (grantedRoles.get(current).contains(sid)) {
+          if (hasSid( grantedRoles.get(current), userIdStrategy, sid))  {


contains() in a set is a relatively fast operation. Now, you have to traverse the entire set to see if the role actually contains the sid.

The benchmarks suggest the same:

The results were compared to the last successful build of master (4efef4f).

Yes, I can't use Set because it's optimized at creation time time but IDStrategy as a configuration can be changed anytime. I suppose I could use a case insensitive Set and then verify it using the correct IDStrategy at lookup time. Or, rebuild the Set every time the configuration changes, but that's a but trickier to get right.

Rebuilding set would be a good approach IMO.
I would be fine if the entire RoleStrategy object gets rebuilt on a configuration change

AbhyudayaSharma · 2019-07-16T03:20:51Z

src/main/java/com/michelin/cio/hudson/plugins/rolestrategy/RoleMap.java

@@ -127,12 +141,15 @@ public RoleMap(@Nonnull SortedMap<Role,Set<String>> grantedRoles) {
  public static void invalidateDangerousPermissions() {
    PermissionHelper.DANGEROUS_PERMISSIONS.forEach(implyingPermissionCache::remove);
  }
-
+  private boolean hasSid(Set<String> roleAssignements, IdStrategy userIdStrategy, String sid) {


Typo here: roleAssignements → roleAssignments

fixed. Thank you. seanmceligot@3be3e3b

AbhyudayaSharma · 2019-07-16T03:30:26Z

src/test/resources/org/jenkinsci/plugins/rolestrategy/Configuration-as-Code.yml

@@ -1,6 +1,7 @@
 jenkins:
  authorizationStrategy:
    roleBased:
+      userIdStrategyChoice: DEFAULT


Shouldn't we have a test case where the userIdStrategyChoice is not present?

We should. Also, it is odd that all tests require "DEFAULT" and none really tests values set

oleg-nenashev

Thanks @seanmceligot ! It is getting closer. but ultimately we need a performance degradation issue solved. Also, would be nice to get new autotests, Javadoc and formatting fixes

oleg-nenashev · 2019-07-21T21:23:23Z

src/main/java/com/michelin/cio/hudson/plugins/rolestrategy/RoleBasedAuthorizationStrategy.java

+      agentRoles.invalidateCache();
+      itemRoles.invalidateCache();
+  }
+  public void setUserIdStrategyChoiceStr(String userIdStrategyChoice) {


I suggest not making such public method.

Suggested change

public void setUserIdStrategyChoiceStr(String userIdStrategyChoice) {

@Restricted(NoExternalUse.class)

public void setUserIdStrategyChoiceStr(String userIdStrategyChoice) {

oleg-nenashev · 2019-07-21T21:23:54Z

src/main/java/com/michelin/cio/hudson/plugins/rolestrategy/RoleBasedAuthorizationStrategy.java

+  }
+  public void setUserIdStrategyChoiceStr(String userIdStrategyChoice) {
+      LOGGER.info("setUserIdStrategyChoice"+ userIdStrategyChoice);
+      setUserIdStrategyChoice( UserIdStrategyChoice.valueOf(userIdStrategyChoice) );


What if a non-existent value is passed. It will fail here, right?

oleg-nenashev · 2019-07-21T21:24:55Z

src/main/java/com/michelin/cio/hudson/plugins/rolestrategy/RoleBasedAuthorizationStrategy.java

@@ -680,6 +757,7 @@ public void doRolesSubmit(StaplerRequest req, StaplerResponse rsp) throws Unsupp

        req.setCharacterEncoding("UTF-8");
      JSONObject json = req.getSubmittedForm();
+      LOGGER.info("doRolesSubmit: "+json.toString());


Unrelated. If you need it, please use FINEST level or lower

oleg-nenashev · 2019-07-21T21:25:02Z

src/main/java/com/michelin/cio/hudson/plugins/rolestrategy/RoleBasedAuthorizationStrategy.java

@@ -696,6 +774,7 @@ public void doAssignSubmit(StaplerRequest req, StaplerResponse rsp) throws Unsup

        req.setCharacterEncoding("UTF-8");
      JSONObject json = req.getSubmittedForm();
+      LOGGER.info("doAssignSubmit: "+json.toString());


oleg-nenashev · 2019-07-21T21:27:18Z

src/main/java/com/michelin/cio/hudson/plugins/rolestrategy/RoleMap.java

@@ -141,7 +158,7 @@ private boolean hasPermission(String sid, Permission permission, RoleType roleTy
    new RoleWalker() {
      public void perform(Role current) {
        if (current.hasAnyPermission(permissions)) {
-          if (grantedRoles.get(current).contains(sid)) {
+          if (hasSid( grantedRoles.get(current), userIdStrategy, sid))  {


Rebuilding set would be a good approach IMO.
I would be fine if the entire RoleStrategy object gets rebuilt on a configuration change

oleg-nenashev · 2019-07-21T21:27:43Z

...java/org/jenkinsci/plugins/rolestrategy/casc/RoleBasedAuthorizationStrategyConfigurator.java

@@ -33,6 +36,8 @@
 @Restricted({NoExternalUse.class})
 public class RoleBasedAuthorizationStrategyConfigurator extends BaseConfigurator<RoleBasedAuthorizationStrategy> {

+	  private static final Logger LOGGER = Logger.getLogger(RoleBasedAuthorizationStrategyConfigurator.class.getName());  


tabs vs. spaces

Suggested change

private static final Logger LOGGER = Logger.getLogger(RoleBasedAuthorizationStrategyConfigurator.class.getName());

private static final Logger LOGGER = Logger.getLogger(RoleBasedAuthorizationStrategyConfigurator.class.getName());

oleg-nenashev · 2019-07-21T21:28:32Z

src/test/java/org/jenkinsci/plugins/rolestrategy/RoleStrategyTest.java

@@ -7,8 +7,7 @@
 import hudson.model.FreeStyleProject;
 import hudson.model.Item;
 import hudson.model.User;
-import hudson.security.AuthorizationStrategy;
-import io.jenkins.plugins.casc.ConfigurationContext;
+import hudson.security.AuthorizationStrategy; import io.jenkins.plugins.casc.ConfigurationContext;


Suggested change

import hudson.security.AuthorizationStrategy; import io.jenkins.plugins.casc.ConfigurationContext;

import hudson.security.AuthorizationStrategy;

import io.jenkins.plugins.casc.ConfigurationContext;

oleg-nenashev · 2019-07-21T21:28:50Z

...resources/com/michelin/cio/hudson/plugins/rolestrategy/RoleStrategyConfig/assign-roles.jelly

@@ -56,7 +56,7 @@
              <td class="left-most">${title}</td>
              <j:forEach var="r" items="${it.strategy.getGrantedRoles(attrs.type)}">
                <td width="*">
-                  <f:checkbox name="[${r.key.name}]" checked="${r.value.contains(attrs.sid)}"/>
+                    <f:checkbox name="[${r.key.name}]" checked="${r.value.contains(attrs.sid)}"/>


Revert it

Suggested change

<f:checkbox name="[${r.key.name}]" checked="${r.value.contains(attrs.sid)}"/>

<f:checkbox name="[${r.key.name}]" checked="${r.value.contains(attrs.sid)}"/>

oleg-nenashev · 2019-07-21T21:29:46Z

src/test/resources/org/jenkinsci/plugins/rolestrategy/Configuration-as-Code.yml

@@ -1,6 +1,7 @@
 jenkins:
  authorizationStrategy:
    roleBased:
+      userIdStrategyChoice: DEFAULT


We should. Also, it is odd that all tests require "DEFAULT" and none really tests values set

AbhyudayaSharma · 2019-07-26T09:58:05Z

src/main/java/com/michelin/cio/hudson/plugins/rolestrategy/RoleMap.java

@@ -127,12 +141,15 @@ public RoleMap(@Nonnull SortedMap<Role,Set<String>> grantedRoles) {
  public static void invalidateDangerousPermissions() {
    PermissionHelper.DANGEROUS_PERMISSIONS.forEach(implyingPermissionCache::remove);
  }
-
+  private boolean hasSid(Set<String> roleAssignments, IdStrategy userIdStrategy, String sid) {
+    return roleAssignments.stream().anyMatch( rolesid -> userIdStrategy.compare(rolesid, sid) == 0 );


I was going through how Matrix Authorization plugin handles it. Looks to me that this check is incomplete. Since these sids can be group names too and there exists SecurityRealm#getGroupIdStrategy(), you will need to take care of that.

https://github.com/jenkinsci/matrix-auth-plugin/blob/8c7b7bcec67a03ab73498486fc2029b2b886541d/src/main/java/org/jenkinsci/plugins/matrixauth/AuthorizationContainer.java#L152

I will look that at. Good catch.

zhaojinggang · 2020-05-06T20:27:09Z

This should fix JENKINS-34545 and related issues. It allows case insensitive matching to user/group values for Global and Item roles in Assign Roles.

Just check if there's any plan to release this feature?

case insensitive set for sids

c20a73e

oleg-nenashev requested changes Aug 26, 2018

View reviewed changes

seanmceligot added 2 commits September 9, 2018 20:30

add configuration to toggle case insensitive user roles

f9bdf92

fix npe. add toString for debugging

81c1ce3

KostyaSha reviewed Sep 10, 2018

View reviewed changes

use String.format()

611da0f

oleg-nenashev self-requested a review September 10, 2018 22:47

oleg-nenashev changed the title ~~case insensitive user ID's in roles~~ [JENKINS-34545] - case insensitive user ID's in roles Sep 16, 2018

oleg-nenashev requested changes Sep 16, 2018

View reviewed changes

oleg-nenashev reviewed Sep 16, 2018

View reviewed changes

seanmceligot added 3 commits September 17, 2018 21:21

move config to Configure Global Security screen

aa71c57

reload grantedRoles when caseinsensitive changes

jelly escape-by-default

d9f0e0f

Merge tag 'role-strategy-2.9.0' into case-insensitive-2.9.0

05498ee

[maven-release-plugin] copy for tag role-strategy-2.9.0

seanmceligot added 9 commits November 20, 2018 19:43

Merge branch 'master' into caseinsensitive

59de1f4

use IdStrategy on sid

c2d122e

remove log message

5f52cda

Merge branch 'case-insensitive-2.10'

77aec7d

use IdStrategy

9098b83

Merge branch 'caseinsensitive' of https://github.com/seanmceligot/rol…

e8c96c1

…e-strategy-plugin into caseinsensitive

remove old changes

cabe4dd

Merge remote-tracking branch 'upstream/master' into case-insensitive-…

630fbeb

…2.11

Merge branch 'master' of https://github.com/jenkinsci/role-strategy-p…

816a67c

…lugin into case-insensitive-2.11

seanmceligot added 8 commits July 7, 2019 14:15

disableUserIdStrategy

0680e31

Merge remote-tracking branch 'upstream/master' into caseinsensitive

3a1c44d

merge fixes

7064584

default, CaseInsensitive, CaseSensitive

26efbc9

Merge remote-tracking branch 'upstream/master' into chooseIdStrategy

c356da2

configuration as code test

f60ec6b

use IdStrategy.compare

153137a

userIdStrategyChoice: DEFAULT

2520423

seanmceligot added 2 commits July 15, 2019 20:36

clean up diff

76f25ac

not needed

4e8006e

oleg-nenashev self-requested a review July 16, 2019 00:39

oleg-nenashev changed the title ~~[JENKINS-34545] - case insensitive user ID's in roles~~ [JENKINS-34545] - Support case insensitive user IDs in roles Jul 16, 2019

oleg-nenashev added the enhancement label Jul 16, 2019

clean up diffs

fbd5815

res0nance reviewed Jul 16, 2019

View reviewed changes

AbhyudayaSharma reviewed Jul 16, 2019

View reviewed changes

fix type-o in roleAssignments

3be3e3b

oleg-nenashev requested changes Jul 21, 2019

View reviewed changes

AbhyudayaSharma reviewed Jul 26, 2019

View reviewed changes

seanmceligot closed this Jun 25, 2020

seanmceligot deleted the caseinsensitive branch June 25, 2020 23:50

	public void setUserIdStrategyChoiceStr(String userIdStrategyChoice) {
	@Restricted(NoExternalUse.class)
	public void setUserIdStrategyChoiceStr(String userIdStrategyChoice) {

	private static final Logger LOGGER = Logger.getLogger(RoleBasedAuthorizationStrategyConfigurator.class.getName());
	private static final Logger LOGGER = Logger.getLogger(RoleBasedAuthorizationStrategyConfigurator.class.getName());

	import hudson.security.AuthorizationStrategy; import io.jenkins.plugins.casc.ConfigurationContext;
	import hudson.security.AuthorizationStrategy;
	import io.jenkins.plugins.casc.ConfigurationContext;

	<f:checkbox name="[${r.key.name}]" checked="${r.value.contains(attrs.sid)}"/>
	<f:checkbox name="[${r.key.name}]" checked="${r.value.contains(attrs.sid)}"/>

[JENKINS-34545] - Support case insensitive user IDs in roles #43

[JENKINS-34545] - Support case insensitive user IDs in roles #43

Conversation

seanmceligot commented Aug 13, 2018

oleg-nenashev left a comment

Choose a reason for hiding this comment

seanmceligot commented Aug 27, 2018

seanmceligot commented Sep 10, 2018

Choose a reason for hiding this comment

oleg-nenashev commented Sep 10, 2018

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-beck commented Sep 16, 2018

ingwarsw commented Nov 10, 2018

oleg-nenashev commented Nov 10, 2018 via email

seanmceligot commented Nov 15, 2018

kiran1abc commented Jun 18, 2019

seanmceligot commented Jun 19, 2019

kiran1abc commented Jun 19, 2019

seanmceligot commented Jul 13, 2019

oleg-nenashev commented Jul 16, 2019

res0nance left a comment

Choose a reason for hiding this comment

AbhyudayaSharma left a comment

Choose a reason for hiding this comment

AbhyudayaSharma Jul 16, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaojinggang commented May 6, 2020

AbhyudayaSharma Jul 16, 2019 •

edited