Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[JENKINS-35515] - Fix the performance issue (regression in 2.3.0) (#18)
* [JENKINS-35515] - Disable user authorities handling by deefault, allow tweaking cache settings.

This change should solve the reported performance issues.
https://issues.jenkins-ci.org/browse/JENKINS-35515

* [JENKINS-35515] - Fix typo

* Fixed the copy-paste error in the test suite
  • Loading branch information
oleg-nenashev committed Jun 13, 2016
1 parent a686d4a commit fe8a518
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 3 deletions.
Expand Up @@ -54,6 +54,7 @@
import org.acegisecurity.GrantedAuthority;
import org.acegisecurity.acls.sid.Sid;
import org.acegisecurity.userdetails.UserDetails;
import org.jenkinsci.plugins.rolestrategy.Settings;
import org.springframework.dao.DataAccessException;

/**
Expand All @@ -70,8 +71,8 @@ public class RoleMap {

private final Cache<String, UserDetails> cache = CacheBuilder.newBuilder()
.softValues()
.maximumSize(100)
.expireAfterWrite(60, TimeUnit.SECONDS)
.maximumSize(Settings.USER_DETAILS_CACHE_MAX_SIZE)
.expireAfterWrite(Settings.USER_DETAILS_CACHE_EXPIRATION_TIME_SEC, TimeUnit.SECONDS)
.build();

RoleMap() {
Expand Down Expand Up @@ -103,7 +104,7 @@ private boolean hasPermission(String sid, Permission p, RoleType roleType, Acces
else {
return true;
}
} else {
} else if (Settings.TREAT_USER_AUTHORITIES_AS_ROLES) {
try {
UserDetails userDetails = cache.getIfPresent(sid);
if (userDetails == null) {
Expand Down
74 changes: 74 additions & 0 deletions src/main/java/org/jenkinsci/plugins/rolestrategy/Settings.java
@@ -0,0 +1,74 @@
/*
* The MIT License
*
* Copyright (c) 2016 Oleg Nenashev.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* 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.
*/
package org.jenkinsci.plugins.rolestrategy;

import com.michelin.cio.hudson.plugins.rolestrategy.RoleMap;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

/**
* Class for managing the strategy.
* This class will be converted to additional UI at some point.
* Now it stores System properties only.
* @author Oleg Nenashev
* @since 2.3.1
*/
@Restricted(NoExternalUse.class)
public class Settings {

/**
* Defines maximum size of the User details cache.
* This cache is being used when {@link #TREAT_USER_AUTHORITIES_AS_ROLES} is enabled.
* Changing of this option requires Jenkins restart.
* @since 2.3.1
*/
public static final int USER_DETAILS_CACHE_MAX_SIZE =
Integer.getInteger(Settings.class.getName() + ".userDetailsCacheMaxSize", 100);

/**
* Defines lifetime of entries in the User details cache.
* This cache is being used when {@link #TREAT_USER_AUTHORITIES_AS_ROLES} is enabled.
* Changing of this option requires Jenkins restart.
* @since 2.3.1
*/
public static final int USER_DETAILS_CACHE_EXPIRATION_TIME_SEC =
Integer.getInteger(Settings.class.getName() + ".userDetailsCacheExpircationTimeSec", 60);

This comment has been minimized.

Copy link
@proxity

proxity Mar 17, 2020

There's a typo! You've put an extra c in Expiration!!


/**
* Enabling processing of User Authorities.
* Alters the behavior of
* {@link RoleMap#hasPermission(java.lang.String, hudson.security.Permission, com.synopsys.arc.jenkins.plugins.rolestrategy.RoleType, hudson.security.AccessControlled)}.
* Since 2.3.0 this value was {@code true}, but it has been switched due to the performance reasons.
* The behavior can be reverted (even dynamically via System Groovy Script).
* @since 2.3.1
*/
@SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "We want to be it modifyable on the flight")
public static boolean TREAT_USER_AUTHORITIES_AS_ROLES =
Boolean.getBoolean(Settings.class.getName() + ".threatUserAuthoritiesAsRoles");

private Settings() {}


}
Expand Up @@ -14,6 +14,8 @@
import org.acegisecurity.context.SecurityContextHolder;
import org.acegisecurity.userdetails.UserDetails;
import org.acegisecurity.userdetails.UsernameNotFoundException;
import org.junit.After;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.JenkinsRule;
Expand All @@ -24,6 +26,16 @@ public class UserAuthoritiesAsRolesTest {

@Rule public JenkinsRule j = new JenkinsRule();

@Before
public void enableUserAuthorities() {
Settings.TREAT_USER_AUTHORITIES_AS_ROLES = true;
}

@After
public void disableUserAuthorities() {
Settings.TREAT_USER_AUTHORITIES_AS_ROLES = false;
}

@LocalData
@Test public void testRoleAuthority() throws Exception {
j.jenkins.setSecurityRealm(new AbstractPasswordBasedSecurityRealm() {
Expand Down

0 comments on commit fe8a518

Please sign in to comment.