[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)
jenkinsci · Nov 12, 2017 · 9735043 · 9735043
1 parent a2fcfa0
commit 9735043
Show file tree

Hide file tree

Showing 3 changed files with 118 additions and 88 deletions.
diff --git a/core/src/main/java/jenkins/security/LastGrantedAuthoritiesProperty.java b/core/src/main/java/jenkins/security/LastGrantedAuthoritiesProperty.java
@@ -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()]);
     }
 
     /**

diff --git a/test/src/test/java/jenkins/security/LastGrantedAuthoritiesPropertyTest.groovy b/test/src/test/java/jenkins/security/LastGrantedAuthoritiesPropertyTest.groovy
diff --git a/test/src/test/java/jenkins/security/LastGrantedAuthoritiesPropertyTest.java b/test/src/test/java/jenkins/security/LastGrantedAuthoritiesPropertyTest.java
@@ -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");
+        }
+    }
+}