Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-55803] - Attempt to solve issue:removing the intermediate collections created each time getRolesHavingPermission() is invoked #54

Merged
merged 26 commits into from
Feb 11, 2019
Merged
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -109,35 +109,49 @@ private boolean hasPermission(String sid, Permission p, RoleType roleType, Acces
/* if this is a dangerous permission, fall back to Administer unless we're in compat mode */
p = Jenkins.ADMINISTER;
}

for(Role role : getRolesHavingPermission(p)) {

if(this.grantedRoles.get(role).contains(sid)) {
final Set<Permission> permissions = new HashSet<>();
final Permission per = p;
final boolean[] temp = {false};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional would be recommended, but ok

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sir i didn't understand.... Can you clarify a bit....?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Get the implying permissions
for (; p!=null; p=p.impliedBy) {
permissions.add(p);
}
// Walk through the roles, and only add the roles having the given permission,
// or a permission implying the given permission
new RoleWalker() {
public void perform(Role current) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to add an overide annotation here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

if (current.hasAnyPermission(permissions)) {
if (grantedRoles.get(current).contains(sid)) {
// Handle roles macro
if (Macro.isMacro(role)) {
Macro macro = RoleMacroExtension.getMacro(role.getName());
if (macro != null) {
RoleMacroExtension macroExtension = RoleMacroExtension.getMacroExtension(macro.getName());
if (macroExtension.IsApplicable(roleType) && macroExtension.hasPermission(sid, p, roleType, controlledItem, macro)) {
return true;
}
if (Macro.isMacro(current)) {
Macro macro = RoleMacroExtension.getMacro(current.getName());
if (macro != null) {
RoleMacroExtension macroExtension = RoleMacroExtension.getMacroExtension(macro.getName());
if (macroExtension.IsApplicable(roleType) && macroExtension.hasPermission(sid, per, roleType, controlledItem, macro)) {
temp[0] =true;
abort();
return ;
}
} // Default handling
else {
return true;
}
} else {
temp[0] =true;
abort();
return ;
}
} else if (Settings.TREAT_USER_AUTHORITIES_AS_ROLES) {
} else if (Settings.TREAT_USER_AUTHORITIES_AS_ROLES) {
try {
UserDetails userDetails = cache.getIfPresent(sid);
if (userDetails == null) {
userDetails = Jenkins.getActiveInstance().getSecurityRealm().loadUserByUsername(sid);
cache.put(sid, userDetails);
}
for (GrantedAuthority grantedAuthority : userDetails.getAuthorities()) {
if (grantedAuthority.getAuthority().equals(role.getName())) {
return true;
}
UserDetails userDetails = cache.getIfPresent(sid);
if (userDetails == null) {
userDetails = Jenkins.getActiveInstance().getSecurityRealm().loadUserByUsername(sid);
cache.put(sid, userDetails);
}
for (GrantedAuthority grantedAuthority : userDetails.getAuthorities()) {
if (grantedAuthority.getAuthority().equals(current.getName())) {
temp[0] =true;
abort();
return ;
}
}
} catch (BadCredentialsException e) {
LOGGER.log(Level.FINE, "Bad credentials", e);
} catch (DataAccessException e) {
Expand All @@ -147,11 +161,11 @@ private boolean hasPermission(String sid, Permission p, RoleType roleType, Acces
// So we want to ensure this method does not fail horribly in such case
LOGGER.log(Level.WARNING, "Unhandled exception during user authorities processing", ex);
}
}
}

// TODO: Handle users macro
}
return false;
}
};
return temp[0];
}

/**
Expand Down Expand Up @@ -408,7 +422,7 @@ public static List<String> getMatchingJobNames(Pattern pattern, int maxJobs) {

return matchingJobNames;
}

/**
* The Acl class that will delegate the permission check to the {@link RoleMap} object.
*/
Expand All @@ -421,7 +435,7 @@ public AclImpl(RoleType roleType, AccessControlled item) {
this.item = item;
this.roleType = roleType;
}

/**
* Checks if the sid has the given permission.
* <p>Actually only delegate the check to the {@link RoleMap} instance.</p>
Expand Down