Merge branch 'security-stable-2.138' into security-stable-2.150

core/src/main/java/hudson/model/User.java

@@ -739,14 +739,16 @@ public class User extends AbstractModelObject implements AccessControlled, Descr
      * Returns the folder that store all the user information.
      * Useful for plugins to save a user-specific file aside the config.xml.
      * Exposes implementation details that may be subject to change.
+     * 
+     * @return The folder containing the user configuration files or {@code null} if the user was not yet saved.
      *
      * @since 2.129
      */
-    public File getUserFolder() {
+    public @CheckForNull File getUserFolder() {
         return getExistingUserFolder();
     }
 
-    private File getExistingUserFolder() {
+    private @CheckForNull File getExistingUserFolder() {
         return UserIdMapper.getInstance().getDirectory(id);
     }
 

core/src/main/java/hudson/model/UserIdMapper.java

@@ -34,6 +34,7 @@ import jenkins.model.IdStrategy;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 
+import javax.annotation.CheckForNull;
 import java.io.File;
 import java.io.IOException;
 import java.nio.file.Files;
@@ -75,7 +76,7 @@ public class UserIdMapper {
         return usersDirectory;
     }
 
-    File getDirectory(String userId) {
+    @CheckForNull File getDirectory(String userId) {
         String directoryName = idToDirectoryNameMap.get(getIdStrategy().keyFor(userId));
         return directoryName == null ? null : new File(usersDirectory, directoryName);
     }

core/src/main/java/jenkins/security/ApiTokenProperty.java

@@ -128,7 +128,7 @@ public class ApiTokenProperty extends UserProperty {
             this.tokenStore = new ApiTokenStore();
         }
         if(this.tokenStats == null){
-            this.tokenStats = ApiTokenStats.load(user.getUserFolder());
+            this.tokenStats = ApiTokenStats.load(user);
         }
         if(this.apiToken != null){
             this.tokenStore.regenerateTokenFromLegacyIfRequired(this.apiToken);

core/src/main/java/jenkins/security/apitoken/ApiTokenStats.java

@@ -23,14 +23,17 @@
  */
 package jenkins.security.apitoken;
 
+import com.google.common.annotations.VisibleForTesting;
 import hudson.BulkChange;
 import hudson.Util;
 import hudson.XmlFile;
 import hudson.model.Saveable;
+import hudson.model.User;
 import hudson.model.listeners.SaveableListener;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 
+import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
 import java.io.File;
 import java.io.IOException;
@@ -54,9 +57,13 @@ public class ApiTokenStats implements Saveable {
      */
     private List<SingleTokenStats> tokenStats;
     
-    private transient File parent;
+    private transient User user;
     
-    public ApiTokenStats() {
+    @VisibleForTesting 
+    transient File parent;
+    
+    @VisibleForTesting 
+    ApiTokenStats() {
         this.init();
     }
     
@@ -94,6 +101,13 @@ public class ApiTokenStats implements Saveable {
         this.tokenStats = new ArrayList<>(temp.values());
     }
     
+    /**
+     * @deprecated use {@link #load(User)} instead of {@link #load(File)}
+     * The method will be removed in a later version as it's an internal one
+     */
+    @Deprecated
+    // to force even if someone wants to remove the one from the class
+    @Restricted(NoExternalUse.class)
     void setParent(@Nonnull File parent) {
         this.parent = parent;
     }
@@ -165,7 +179,17 @@ public class ApiTokenStats implements Saveable {
         if (BulkChange.contains(this))
             return;
         
-        XmlFile configFile = getConfigFile(parent);
+        /*
+         * Note: the userFolder should never be null at this point.
+         * The userFolder could be null during User creation with the new storage approach
+         * but when this code is called, from token used / removed, the folder exists.
+         */
+        File userFolder = getUserFolder();
+        if (userFolder == null) {
+            return;
+        }
+        
+        XmlFile configFile = getConfigFile(userFolder);
         try {
             configFile.write(this);
             SaveableListener.fireOnChange(this, configFile);
@@ -174,25 +198,41 @@ public class ApiTokenStats implements Saveable {
         }
     }
     
+    private @CheckForNull File getUserFolder(){
+        File userFolder = parent;
+        if (userFolder == null && this.user != null) {
+            userFolder = user.getUserFolder();
+            if (userFolder == null) {
+                LOGGER.log(Level.INFO, "No user folder yet for user {0}", user.getId());
+                return null;
+            }
+            this.parent = userFolder;
+        }
+        
+        return userFolder;
+    }
+    
     /**
      * Loads the data from the disk into the new object.
      * <p>
      * If the file is not present, a fresh new instance is created.
+     * 
+     * @deprecated use {@link #load(User)} instead
+     * The method will be removed in a later version as it's an internal one
      */
-    public static @Nonnull ApiTokenStats load(@Nonnull File parent) {
+    @Deprecated
+    // to force even if someone wants to remove the one from the class
+    @Restricted(NoExternalUse.class) 
+    public static @Nonnull ApiTokenStats load(@CheckForNull File parent) {
         // even if we are not using statistics, we load the existing one in case the configuration
         // is enabled afterwards to avoid erasing data
         
-        XmlFile file = getConfigFile(parent);
-        ApiTokenStats apiTokenStats;
-        if (file.exists()) {
-            try {
-                apiTokenStats = (ApiTokenStats) file.unmarshal(ApiTokenStats.class);
-            } catch (IOException e) {
-                LOGGER.log(Level.WARNING, "Failed to load " + file, e);
-                apiTokenStats = new ApiTokenStats();
-            }
-        } else {
+        if (parent == null) {
+            return new ApiTokenStats();
+        }
+    
+        ApiTokenStats apiTokenStats = internalLoad(parent);
+        if (apiTokenStats == null) {
             apiTokenStats = new ApiTokenStats();
         }
     
@@ -200,7 +240,48 @@ public class ApiTokenStats implements Saveable {
         return apiTokenStats;
     }
     
-    protected static XmlFile getConfigFile(File parent) {
+    /**
+     * Loads the data from the user folder into the new object.
+     * <p>
+     * If the folder does not exist yet, a fresh new instance is created.
+     */
+    public static @Nonnull ApiTokenStats load(@Nonnull User user) {
+        // even if we are not using statistics, we load the existing one in case the configuration
+        // is enabled afterwards to avoid erasing data
+        
+        ApiTokenStats apiTokenStats = null;
+        
+        File userFolder = user.getUserFolder();
+        if (userFolder != null) {
+            apiTokenStats = internalLoad(userFolder);
+        }
+        
+        if (apiTokenStats == null) {
+            apiTokenStats = new ApiTokenStats();
+        }
+        
+        apiTokenStats.user = user;
+        
+        return apiTokenStats;
+    }
+    
+    @VisibleForTesting
+    static @CheckForNull ApiTokenStats internalLoad(@Nonnull File userFolder) {
+        ApiTokenStats apiTokenStats = null;
+        XmlFile statsFile = getConfigFile(userFolder);
+        if (statsFile.exists()) {
+            try {
+                apiTokenStats = (ApiTokenStats) statsFile.unmarshal(ApiTokenStats.class);
+                apiTokenStats.parent = userFolder;
+            } catch (IOException e) {
+                LOGGER.log(Level.WARNING, "Failed to load " + statsFile, e);
+            }
+        }
+        
+        return apiTokenStats;
+    }
+    
+    protected static @Nonnull XmlFile getConfigFile(@Nonnull File parent) {
         return new XmlFile(new File(parent, "apiTokenStats.xml"));
     }
     

core/src/test/java/jenkins/security/apitoken/ApiTokenStatsTest.java

@@ -25,27 +25,22 @@ package jenkins.security.apitoken;
 
 import hudson.XmlFile;
 import org.apache.commons.io.FileUtils;
-import org.hamcrest.Matchers;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
 import org.junit.runner.RunWith;
 import org.mockito.Mockito;
-import org.mockito.internal.matchers.LessOrEqual;
 import org.powermock.api.mockito.PowerMockito;
 import org.powermock.core.classloader.annotations.PowerMockIgnore;
 import org.powermock.core.classloader.annotations.PrepareForTest;
 import org.powermock.modules.junit4.PowerMockRunner;
 
+import java.io.File;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
 import java.text.SimpleDateFormat;
-import java.time.LocalDate;
-import java.time.format.DateTimeFormatter;
 import java.time.temporal.ChronoUnit;
-import java.time.temporal.TemporalAccessor;
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Comparator;
@@ -84,8 +79,7 @@ public class ApiTokenStatsTest {
         final String ID_2 = "other-uuid";
         
         { // empty stats can be saved
-            ApiTokenStats tokenStats = new ApiTokenStats();
-            tokenStats.setParent(tmp.getRoot());
+            ApiTokenStats tokenStats = createFromFile(tmp.getRoot());
             
             // can remove an id that does not exist
             tokenStats.removeId(ID_1);
@@ -94,7 +88,7 @@ public class ApiTokenStatsTest {
         }
         
         { // and then loaded, empty stats is empty
-            ApiTokenStats tokenStats = ApiTokenStats.load(tmp.getRoot());
+            ApiTokenStats tokenStats = createFromFile(tmp.getRoot());
             assertNotNull(tokenStats);
             
             ApiTokenStats.SingleTokenStats stats = tokenStats.findTokenStatsById(ID_1);
@@ -105,7 +99,7 @@ public class ApiTokenStatsTest {
         
         Date lastUsage;
         { // then re-notify the same token
-            ApiTokenStats tokenStats = ApiTokenStats.load(tmp.getRoot());
+            ApiTokenStats tokenStats = createFromFile(tmp.getRoot());
             
             ApiTokenStats.SingleTokenStats stats = tokenStats.updateUsageForId(ID_1);
             assertEquals(1, stats.getUseCounter());
@@ -120,7 +114,7 @@ public class ApiTokenStatsTest {
         Thread.sleep(10);
         
         { // then re-notify the same token
-            ApiTokenStats tokenStats = ApiTokenStats.load(tmp.getRoot());
+            ApiTokenStats tokenStats = createFromFile(tmp.getRoot());
             
             ApiTokenStats.SingleTokenStats stats = tokenStats.updateUsageForId(ID_1);
             assertEquals(2, stats.getUseCounter());
@@ -131,7 +125,7 @@ public class ApiTokenStatsTest {
         }
         
         { // check all tokens have separate stats, try with another ID
-            ApiTokenStats tokenStats = ApiTokenStats.load(tmp.getRoot());
+            ApiTokenStats tokenStats = createFromFile(tmp.getRoot());
             
             {
                 ApiTokenStats.SingleTokenStats stats = tokenStats.findTokenStatsById(ID_2);
@@ -148,7 +142,7 @@ public class ApiTokenStatsTest {
         }
         
         { // reload the stats, check the counter are correct
-            ApiTokenStats tokenStats = ApiTokenStats.load(tmp.getRoot());
+            ApiTokenStats tokenStats = createFromFile(tmp.getRoot());
             
             ApiTokenStats.SingleTokenStats stats_1 = tokenStats.findTokenStatsById(ID_1);
             assertEquals(2, stats_1.getUseCounter());
@@ -159,7 +153,7 @@ public class ApiTokenStatsTest {
         }
         
         { // after a removal, the existing must keep its value
-            ApiTokenStats tokenStats = ApiTokenStats.load(tmp.getRoot());
+            ApiTokenStats tokenStats = createFromFile(tmp.getRoot());
             
             ApiTokenStats.SingleTokenStats stats_1 = tokenStats.findTokenStatsById(ID_1);
             assertEquals(0, stats_1.getUseCounter());
@@ -170,7 +164,7 @@ public class ApiTokenStatsTest {
     
     @Test
     public void testResilientIfFileDoesNotExist() throws Exception {
-        ApiTokenStats tokenStats = ApiTokenStats.load(tmp.getRoot());
+        ApiTokenStats tokenStats = createFromFile(tmp.getRoot());
         assertNotNull(tokenStats);
     }
     
@@ -181,8 +175,7 @@ public class ApiTokenStatsTest {
         final String ID_3 = UUID.randomUUID().toString();
         
         { // put counter to 4 for ID_1 and to 2 for ID_2 and 1 for ID_3
-            ApiTokenStats tokenStats = new ApiTokenStats();
-            tokenStats.setParent(tmp.getRoot());
+            ApiTokenStats tokenStats = createFromFile(tmp.getRoot());
             
             tokenStats.updateUsageForId(ID_1);
             tokenStats.updateUsageForId(ID_1);
@@ -203,7 +196,7 @@ public class ApiTokenStatsTest {
         }
         
         {
-            ApiTokenStats tokenStats = ApiTokenStats.load(tmp.getRoot());
+            ApiTokenStats tokenStats = createFromFile(tmp.getRoot());
             assertNotNull(tokenStats);
             
             ApiTokenStats.SingleTokenStats stats_1 = tokenStats.findTokenStatsById(ID_1);
@@ -230,16 +223,15 @@ public class ApiTokenStatsTest {
                     /* D */ createSingleTokenStatsByReflection(ID, "2018-05-01 09:10:59.235", 1)
             );
             
-            ApiTokenStats stats = new ApiTokenStats();
+            ApiTokenStats stats = createFromFile(tmp.getRoot());
             Field field = ApiTokenStats.class.getDeclaredField("tokenStats");
             field.setAccessible(true);
             field.set(stats, tokenStatsList);
             
-            stats.setParent(tmp.getRoot());
             stats.save();
         }
         { // reload to see the effect
-            ApiTokenStats stats = ApiTokenStats.load(tmp.getRoot());
+            ApiTokenStats stats = createFromFile(tmp.getRoot());
             ApiTokenStats.SingleTokenStats tokenStats = stats.findTokenStatsById(ID);
             // must be D (as it was the last updated one)
             assertThat(tokenStats.getUseCounter(), equalTo(1));
@@ -295,8 +287,7 @@ public class ApiTokenStatsTest {
     @Test
     public void testDayDifference() throws Exception {
         final String ID = UUID.randomUUID().toString();
-        ApiTokenStats tokenStats = new ApiTokenStats();
-        tokenStats.setParent(tmp.getRoot());
+        ApiTokenStats tokenStats = createFromFile(tmp.getRoot());
         ApiTokenStats.SingleTokenStats stats = tokenStats.updateUsageForId(ID);
         assertThat(stats.getNumDaysUse(), lessThan(1L));
         
@@ -313,4 +304,14 @@ public class ApiTokenStatsTest {
         
         assertThat(stats.getNumDaysUse(), greaterThanOrEqualTo(2L));
     }
+    
+    private ApiTokenStats createFromFile(File file){
+        ApiTokenStats result = ApiTokenStats.internalLoad(file);
+        if (result == null) {
+            result = new ApiTokenStats();
+            result.parent = file;
+        }
+        
+        return result;
+    }
 }

test/src/test/java/jenkins/security/apitoken/ApiTokenStatsRestartTest.java

+/*
+ * The MIT License
+ *
+ * Copyright (c) 2018, CloudBees, Inc.
+ *
+ * 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 jenkins.security.apitoken;
+
+import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
+import com.gargoylesoftware.htmlunit.HttpMethod;
+import com.gargoylesoftware.htmlunit.Page;
+import com.gargoylesoftware.htmlunit.WebRequest;
+import com.gargoylesoftware.htmlunit.html.HtmlPage;
+import com.gargoylesoftware.htmlunit.html.HtmlSpan;
+import com.gargoylesoftware.htmlunit.util.NameValuePair;
+import com.gargoylesoftware.htmlunit.xml.XmlPage;
+import hudson.model.User;
+import jenkins.security.ApiTokenProperty;
+import net.sf.json.JSONObject;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runners.model.Statement;
+import org.jvnet.hudson.test.For;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.jvnet.hudson.test.JenkinsRule.WebClient;
+import org.jvnet.hudson.test.RestartableJenkinsRule;
+
+import java.io.File;
+import java.net.URL;
+import java.util.Arrays;
+import java.util.concurrent.atomic.AtomicReference;
+
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.xml.HasXPath.hasXPath;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+@For(ApiTokenStats.class)
+public class ApiTokenStatsRestartTest {
+    
+    @Rule
+    public RestartableJenkinsRule rr = new RestartableJenkinsRule();
+    
+    @Test
+    @Issue("SECURITY-1072")
+    public void roundtripWithRestart() throws Exception {
+        AtomicReference<String> tokenValue = new AtomicReference<>();
+        AtomicReference<String> tokenUuid = new AtomicReference<>();
+        String TOKEN_NAME = "New Token Name";
+        int NUM_CALL_WITH_TOKEN = 5;
+        
+        rr.addStep(new Statement() {
+               @Override
+               public void evaluate() throws Throwable {
+                   JenkinsRule j = rr.j;
+                   j.jenkins.setCrumbIssuer(null);
+                   j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
+                   
+                   User u = User.getById("foo", true);
+
+                   ApiTokenProperty t = u.getProperty(ApiTokenProperty.class);
+                   assertNotNull(t.getTokenStore());
+                   assertNotNull(t.getTokenStats());
+
+                   // test the authentication via Token
+                   WebClient wc = j.createWebClient().withBasicCredentials(u.getId());
+                   wc.getOptions().setThrowExceptionOnFailingStatusCode(false);
+
+                   WebRequest request = new WebRequest(new URL(j.getURL() + "user/" + u.getId() + "/descriptorByName/" + ApiTokenProperty.class.getName() + "/generateNewToken"), HttpMethod.POST);
+                   request.setRequestParameters(Arrays.asList(new NameValuePair("newTokenName", TOKEN_NAME)));
+
+                   Page page = wc.getPage(request);
+                   assertEquals(200, page.getWebResponse().getStatusCode());
+                   String responseContent = page.getWebResponse().getContentAsString();
+                   JSONObject jsonObject = JSONObject.fromObject(responseContent);
+                   JSONObject jsonData = jsonObject.getJSONObject("data");
+                   String tokenName = jsonData.getString("tokenName");
+                   tokenValue.set(jsonData.getString("tokenValue"));
+                   tokenUuid.set(jsonData.getString("tokenUuid"));
+
+                   assertEquals(TOKEN_NAME, tokenName);
+
+                   WebClient restWc = j.createWebClient().withBasicCredentials(u.getId(), tokenValue.get());
+                   checkUserIsConnected(restWc, u.getId());
+
+                   HtmlPage config = wc.goTo(u.getUrl() + "/configure");
+                   assertEquals(200, config.getWebResponse().getStatusCode());
+                   assertThat(config.getWebResponse().getContentAsString(), containsString(tokenUuid.get()));
+                   assertThat(config.getWebResponse().getContentAsString(), containsString(tokenName));
+
+                   // one is already done with checkUserIsConnected
+                   for (int i = 1; i < NUM_CALL_WITH_TOKEN; i++) {
+                       restWc.goToXml("whoAmI/api/xml");
+                   }
+
+                   HtmlPage configWithStats = wc.goTo(u.getUrl() + "/configure");
+                   assertEquals(200, configWithStats.getWebResponse().getStatusCode());
+                   HtmlSpan useCounterSpan = configWithStats.getDocumentElement().getOneHtmlElementByAttribute("span", "class", "token-use-counter");
+                   assertThat(useCounterSpan.getTextContent(), containsString("" + NUM_CALL_WITH_TOKEN));
+
+                   File apiTokenStatsFile = new File(u.getUserFolder(), "apiTokenStats.xml");
+                   assertTrue("apiTokenStats.xml file should exist", apiTokenStatsFile.exists());
+               }
+           });
+    
+        rr.addStep(new Statement() {
+            @Override
+            public void evaluate() throws Throwable {
+                JenkinsRule j = rr.j;
+                j.jenkins.setCrumbIssuer(null);
+                
+                User u = User.getById("foo", false);
+                assertNotNull(u);
+    
+                WebClient wc = j.createWebClient().login(u.getId());
+                checkUserIsConnected(wc, u.getId());
+    
+                HtmlPage config = wc.goTo(u.getUrl() + "/configure");
+                assertEquals(200, config.getWebResponse().getStatusCode());
+                assertThat(config.getWebResponse().getContentAsString(), containsString(tokenUuid.get()));
+                assertThat(config.getWebResponse().getContentAsString(), containsString(TOKEN_NAME));
+                HtmlSpan useCounterSpan = config.getDocumentElement().getOneHtmlElementByAttribute("span", "class", "token-use-counter");
+                assertThat(useCounterSpan.getTextContent(), containsString("" + NUM_CALL_WITH_TOKEN));
+                
+                revokeToken(wc, u.getId(), tokenUuid.get());
+                
+                // token is no more valid
+                WebClient restWc = j.createWebClient().withBasicCredentials(u.getId(), tokenValue.get());
+                checkUserIsNotConnected(restWc);
+                
+                HtmlPage configWithoutToken = wc.goTo(u.getUrl() + "/configure");
+                assertEquals(200, configWithoutToken.getWebResponse().getStatusCode());
+                assertThat(configWithoutToken.getWebResponse().getContentAsString(), not(containsString(tokenUuid.get())));
+                assertThat(configWithoutToken.getWebResponse().getContentAsString(), not(containsString(TOKEN_NAME)));
+            }
+        });
+    }
+    
+    private void checkUserIsConnected(WebClient wc, String username) throws Exception {
+        XmlPage xmlPage = wc.goToXml("whoAmI/api/xml");
+        assertThat(xmlPage, hasXPath("//name", is(username)));
+        assertThat(xmlPage, hasXPath("//anonymous", is("false")));
+        assertThat(xmlPage, hasXPath("//authenticated", is("true")));
+        assertThat(xmlPage, hasXPath("//authority", is("authenticated")));
+    }
+    
+    private void checkUserIsNotConnected(WebClient wc) throws Exception {
+        try {
+            wc.goToXml("whoAmI/api/xml");
+            fail();
+        } catch (FailingHttpStatusCodeException e) {
+            assertEquals(401, e.getStatusCode());
+        }
+    }
+    
+    private void revokeToken(WebClient wc, String login, String tokenUuid) throws Exception {
+        WebRequest request = new WebRequest(
+                new URL(rr.j.getURL(), "user/" + login + "/descriptorByName/" + ApiTokenProperty.class.getName() + "/revoke/?tokenUuid=" + tokenUuid),
+                HttpMethod.POST
+        );
+        Page p = wc.getPage(request);
+        assertEquals(200, p.getWebResponse().getStatusCode());
+    }
+}