[JENKINS-48157] - Risk of NPE when migrating MyViewProperty without P…

…rimaryView (#3156) * [JENKINS-48157] - Reproduce the issue in test * [JENKINS-48157] - Annotate and document nullness conditions in MyViewsProperty and ViewGroupMixIn * [FIXED JENKINS-48157] - Prevent NPEs when using public API and when using null primaryViewName * [JENKINS-48157] - Fix typo in Javadoc (cherry picked from commit 211b57e)
jenkinsci · Dec 11, 2017 · 6be05b8 · 6be05b8
1 parent 065b874
commit 6be05b8
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 9 deletions.
diff --git a/core/src/main/java/hudson/model/MyViewsProperty.java b/core/src/main/java/hudson/model/MyViewsProperty.java
@@ -39,6 +39,7 @@
 import java.util.List;
 import java.util.concurrent.CopyOnWriteArrayList;
 
+import javax.annotation.CheckForNull;
 import javax.servlet.ServletException;
 
 import jenkins.model.Jenkins;
@@ -61,6 +62,12 @@
  * @author Tom Huybrechts
  */
 public class MyViewsProperty extends UserProperty implements ModifiableViewGroup, Action, StaplerFallback {
+
+    /**
+     * Name of the primary view defined by the user.
+     * {@code null} means that the View is not defined.
+     */
+    @CheckForNull
     private String primaryViewName;
 
     /**
@@ -71,12 +78,13 @@ public class MyViewsProperty extends UserProperty implements ModifiableViewGroup
     private transient ViewGroupMixIn viewGroupMixIn;
 
     @DataBoundConstructor
-    public MyViewsProperty(String primaryViewName) {
+    public MyViewsProperty(@CheckForNull String primaryViewName) {
         this.primaryViewName = primaryViewName;
+        readResolve(); // initialize fields
     }
 
     private MyViewsProperty() {
-        readResolve();
+        this(null);
     }
 
     public Object readResolve() {
@@ -88,7 +96,10 @@ public Object readResolve() {
             // preserve the non-empty invariant
             views.add(new AllView(AllView.DEFAULT_VIEW_NAME, this));
         }
-        primaryViewName = AllView.migrateLegacyPrimaryAllViewLocalizedName(views, primaryViewName);
+        if (primaryViewName != null) {
+            // It may happen when the default constructor is invoked
+            primaryViewName = AllView.migrateLegacyPrimaryAllViewLocalizedName(views, primaryViewName);
+        }
 
         viewGroupMixIn = new ViewGroupMixIn(this) {
             protected List<View> views() { return views; }
@@ -99,11 +110,17 @@ public Object readResolve() {
         return this;
     }
 
+    @CheckForNull
     public String getPrimaryViewName() {
         return primaryViewName;
     }
 
-    public void setPrimaryViewName(String primaryViewName) {
+    /**
+     * Sets the primary view.
+     * @param primaryViewName Name of the primary view to be set.
+     *                        {@code null} to make the primary view undefined.
+     */
+    public void setPrimaryViewName(@CheckForNull String primaryViewName) {
         this.primaryViewName = primaryViewName;
     }
 

diff --git a/core/src/main/java/hudson/model/ViewGroupMixIn.java b/core/src/main/java/hudson/model/ViewGroupMixIn.java
@@ -36,6 +36,7 @@
 import java.util.Collections;
 import java.util.List;
 import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
 
 /**
  * Implements {@link ViewGroup} to be used as a "mix-in".
@@ -66,27 +67,40 @@ public abstract class ViewGroupMixIn {
     private final ViewGroup owner;
 
     /**
-     * Returns all the views. This list must be concurrently iterable.
+     * Returns all views in the group. This list must be modifiable and concurrently iterable.
      */
+    @Nonnull
     protected abstract List<View> views();
+
+    /**
+     * Gets primary view of the mix-in.
+     * @return Name of the primary view, {@code null} if there is no primary one defined.
+     */
+    @CheckForNull
     protected abstract String primaryView();
+
+    /**
+     * Sets the primary view.
+     * @param newName Name of the primary view to be set.
+     *                {@code null} to make the primary view undefined.
+     */
     protected abstract void primaryView(String newName);
 
     protected ViewGroupMixIn(ViewGroup owner) {
         this.owner = owner;
     }
 
-    public void addView(View v) throws IOException {
+    public void addView(@Nonnull View v) throws IOException {
         v.owner = owner;
         views().add(v);
         owner.save();
     }
 
-    public boolean canDelete(View view) {
+    public boolean canDelete(@Nonnull View view) {
         return !view.isDefault();  // Cannot delete primary view
     }
 
-    public synchronized void deleteView(View view) throws IOException {
+    public synchronized void deleteView(@Nonnull View view) throws IOException {
         if (views().size() <= 1)
             throw new IllegalStateException("Cannot delete last view");
         views().remove(view);
@@ -101,11 +115,14 @@ public synchronized void deleteView(View view) throws IOException {
      */
     @CheckForNull
     public View getView(@CheckForNull String name) {
+        if (name == null) {
+            return null;
+        }
         for (View v : views()) {
             if(v.getViewName().equals(name))
                 return v;
         }
-        if (name != null && !name.equals(primaryView())) {
+        if (!name.equals(primaryView())) {
             // Fallback to subview of primary view if it is a ViewGroup
             View pv = getPrimaryView();
             if (pv instanceof ViewGroup)

diff --git a/test/src/test/java/hudson/model/MyViewsPropertyTest.java b/test/src/test/java/hudson/model/MyViewsPropertyTest.java
@@ -33,6 +33,7 @@
 import jenkins.model.Jenkins;
 import org.acegisecurity.AccessDeniedException;
 import org.acegisecurity.context.SecurityContextHolder;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import static org.junit.Assert.*;
 /**
@@ -288,5 +289,20 @@ public void testHasPermission() throws IOException {
         auth.add(Jenkins.ADMINISTER, "User2");
         assertTrue("User User2 should configure permission for user User",property.hasPermission(Permission.CONFIGURE));
     }
+
+    @Test
+    @Issue("JENKINS-48157")
+    public void shouldNotFailWhenMigratingLegacyViewsWithoutPrimaryOne() throws IOException {
+        rule.jenkins.setSecurityRealm(rule.createDummySecurityRealm());
+        User user = User.get("User");
+
+        // Emulates creation of a new object with Reflection in User#load() does.
+        MyViewsProperty property = new MyViewsProperty(null);
+        user.addProperty(property);
+
+        // At AllView with non-default to invoke NPE path in AllView.migrateLegacyPrimaryAllViewLocalizedName()
+        property.addView(new AllView("foobar"));
+        property.readResolve();
+    }
 
 }