Skip to content
Permalink
Browse files

[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
  • Loading branch information...
oleg-nenashev committed Nov 26, 2017
1 parent 56f5abd commit 211b57e8b002b4da92743dfac3a1b902835e7652
@@ -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 @@
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;
}

@@ -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 @@
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)
@@ -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();
}

}

0 comments on commit 211b57e

Please sign in to comment.
You can’t perform that action at this time.