Skip to content
Permalink
Browse files
[JENKINS-47768] - Avoid having "authenticated" twice in the group mem…
…bership of a user (LastGrantedAuthorities) (#3123)

* Avoid having "authenticated" twice in the group membership of a user
- this occur when the SecurityRealm potentially already grants that role (like in github-oauth-plugin)

* - changed as requested by Oleg
- the list has a maximum of roles.length and in reality it's either roles.length or (roles.length-1), so the maximum is ok

* - fix problem of missing the "authenticated" authority

* - convert the Groovy script to a Java version
- the Groovy test was not run by default (IIUC Groovy scripts are not compiled if placed in java src folder)
  • Loading branch information
Wadeck authored and oleg-nenashev committed Nov 12, 2017
1 parent a2fcfa0 commit 9735043b6192df3ba37a5a30d146fb807c3fc9ef
@@ -48,14 +48,22 @@ public UserProperty reconfigure(StaplerRequest req, JSONObject form) throws Form
public GrantedAuthority[] getAuthorities() {
String[] roles = this.roles; // capture to a variable for immutability

GrantedAuthority[] r = new GrantedAuthority[roles==null ? 1 : roles.length+1];
r[0] = SecurityRealm.AUTHENTICATED_AUTHORITY;
if (roles != null) {
for (int i = 1; i < r.length; i++) {
r[i] = new GrantedAuthorityImpl(roles[i - 1]);
if(roles == null){
return new GrantedAuthority[]{SecurityRealm.AUTHENTICATED_AUTHORITY};
}

String authenticatedRole = SecurityRealm.AUTHENTICATED_AUTHORITY.getAuthority();
List<GrantedAuthority> grantedAuthorities = new ArrayList<>(roles.length + 1);
grantedAuthorities.add(new GrantedAuthorityImpl(authenticatedRole));

for (int i = 0; i < roles.length; i++){
// to avoid having twice that role
if(!authenticatedRole.equals(roles[i])){
grantedAuthorities.add(new GrantedAuthorityImpl(roles[i]));
}
}
return r;

return grantedAuthorities.toArray(new GrantedAuthority[grantedAuthorities.size()]);
}

/**

This file was deleted.

@@ -0,0 +1,104 @@
package jenkins.security;

import com.gargoylesoftware.htmlunit.html.HtmlPage;
import hudson.security.AbstractPasswordBasedSecurityRealm;
import hudson.security.GroupDetails;
import hudson.security.UserMayOrMayNotExistException;
import org.acegisecurity.*;
import org.acegisecurity.userdetails.User;
import org.acegisecurity.userdetails.UserDetails;
import org.acegisecurity.userdetails.UsernameNotFoundException;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.JenkinsRule;
import org.springframework.dao.DataAccessException;

import java.util.Arrays;
import java.util.List;
import java.util.logging.Logger;
import java.util.stream.Collectors;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;

/**
* @author Kohsuke Kawaguchi
*/
public class LastGrantedAuthoritiesPropertyTest {
@Rule
public JenkinsRule j = new JenkinsRule();

@Test
public void basicFlow() throws Exception {
j.jenkins.setSecurityRealm(new TestSecurityRealm());

// login, and make sure it leaves the LastGrantedAuthoritiesProperty object
JenkinsRule.WebClient wc = j.createWebClient();
wc.login("alice", "alice:development:us");

hudson.model.User u = hudson.model.User.get("alice");
LastGrantedAuthoritiesProperty p = u.getProperty(LastGrantedAuthoritiesProperty.class);
assertAuthorities(p, "authenticated:alice:development:us");
assertAuthorities(u.impersonate(), "authenticated:alice:development:us");

// visiting the configuration page shouldn't change authorities
HtmlPage pg = wc.goTo("user/alice/configure");
j.submit(pg.getFormByName("config"));

p = u.getProperty(LastGrantedAuthoritiesProperty.class);
assertAuthorities(p, "authenticated:alice:development:us");
assertAuthorities(u.impersonate(), "authenticated:alice:development:us");

// change should be reflected right away
wc.login("alice", "alice:development:uk");
p = u.getProperty(LastGrantedAuthoritiesProperty.class);
assertAuthorities(p, "authenticated:alice:development:uk");
assertAuthorities(u.impersonate(), "authenticated:alice:development:uk");

// if already receiving the authenticated group, we should avoid duplicate
wc.login("alice", "alice:authenticated:development:uk");
p = u.getProperty(LastGrantedAuthoritiesProperty.class);

assertAuthorities(p, "authenticated:alice:development:uk");
assertAuthorities(u.impersonate(), "authenticated:alice:development:uk");
}

private void assertAuthorities(LastGrantedAuthoritiesProperty p, String expected) {
_assertAuthorities(p.getAuthorities(), expected);
}

private void assertAuthorities(Authentication auth, String expected) {
_assertAuthorities(auth.getAuthorities(), expected);
}

private void _assertAuthorities(GrantedAuthority[] grantedAuthorities, String expected){
List<String> authorities = Arrays.stream(grantedAuthorities).map(GrantedAuthority::getAuthority).collect(Collectors.toList());

assertEquals(String.join(":", authorities), expected);
}

/**
* SecurityRealm that cannot load information about other users, such Active Directory.
*/
private static class TestSecurityRealm extends AbstractPasswordBasedSecurityRealm {
@Override
protected UserDetails authenticate(String username, String password) throws AuthenticationException {
if (password.equals("error"))
throw new BadCredentialsException(username);
String[] desiredAuthorities = password.split(":");
List<GrantedAuthority> authorities = Arrays.stream(desiredAuthorities).map(GrantedAuthorityImpl::new).collect(Collectors.toList());

return new User(username, "", true, authorities.toArray(new GrantedAuthority[0]));
}

@Override
public GroupDetails loadGroupByGroupname(String groupname) throws UsernameNotFoundException, DataAccessException {
throw new UnsupportedOperationException();
}

@Override
public UserDetails loadUserByUsername(String username) throws UsernameNotFoundException, DataAccessException {
throw new UserMayOrMayNotExistException("fallback");
}
}
}

0 comments on commit 9735043

Please sign in to comment.