Handle metadata requet with wrong view

When asking for gadget iframe url with a view that does not exists in the gadget, and there is no default view, you get an NPE.

The fix here is basically check for view before trying to get rendered url by the gadget handler, and return processing error if not available.

The problem is that the iframe url depend on the view - in case of type url view, put user prefs in fragment or query params, and put token in fragment or query params.
I guess an alternate solution would be to default to worst case - put in query params.

[plindner1, 2011-03-11 18:25:35]

lgtm

[ssievers_us.ibm.com, 2011-03-11 18:26:31]

Hi Ziv,

I'm seeing an extra method in your patch in GadgetsHandlerService.java 
that is causing compile issues.  The getCajoled() method that you've added 
is annotated with @Override, but GadgetContext does not have a 
getCajoled() method so the compiler is throwing an error.  The code 
doesn't seem related to the rest of the fix, so I'm wondering if this is 
just a remnant from some other work you've been doing.

Once I removed the @Override annotation for this method in my local 
environment, everything built just fine and seems to work as expected at 
runtime.

Thanks and best regards,
-Stanton



From:   zhoresh@gmail.com
To:     dev-remailer@shindig.apache.org, johnfargo@gmail.com, 
lindner@inuus.com, Stanton Sievers/Westford/IBM@Lotus, 
Cc:     reply@codereview.appspotmail.com
Date:   03/10/2011 19:10
Subject:        Handle metadata requet with wrong view (issue4272043)



Reviewers: dev-remailer_shindig.apache.org, johnfargo, Paul Lindner, 
ssievers_us.ibm.com,

Description:
When asking for gadget iframe url with a view that does not exists in
the gadget, and there is no default view, you get an NPE.

The fix here is basically check for view before trying to get rendered
url by the gadget handler, and return processing error if not available.

The problem is that the iframe url depend on the view - in case of type
url view, put user prefs in fragment or query params, and put token in
fragment or query params.
I guess an alternate solution would be to default to worst case - put in
query params.



Please review this at http://codereview.appspot.com/4272043/

Affected files:
 
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
 
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/FakeProcessor.java
 
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java


### Eclipse Workspace Patch 1.0
#P shindig-project
Index: 
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
===================================================================
--- 
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java

 
(revision 1079640)
+++ 
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java

 
(working copy)
@@ -163,9 +163,12 @@

      GadgetContext context = new MetadataGadgetContext(request);
      Gadget gadget = processor.process(context);
-    String iframeUrl =
-        isFieldIncluded(fields, "iframeurl")  ?
-            iframeUriManager.makeRenderingUri(gadget).toString() : null;
+    boolean needIfrUrl = isFieldIncluded(fields, "iframeurl");
+    if (needIfrUrl && gadget.getCurrentView() == null) {
+      throw new ProcessingException("View " + request.getView() + " does 
not exists",
+          HttpResponse.SC_BAD_REQUEST);
+    }
+    String iframeUrl = needIfrUrl ? 
iframeUriManager.makeRenderingUri(gadget).toString() : null;
      Boolean needsTokenRefresh =
          isFieldIncluded(fields, "needstokenrefresh") ?
              gadget.getAllFeatures().contains("auth-refresh") : null;
@@ -402,6 +405,11 @@
      public boolean getSanitize() {
        return (request.getRenderingType() == 
GadgetsHandlerApi.RenderingType.SANITIZED);
      }
+
+    @Override
+    public boolean getCajoled() {
+      return (request.getRenderingType() == 
GadgetsHandlerApi.RenderingType.IFRAME_CAJOLED);
+    }
    }

    private SecurityToken convertAuthContext(GadgetsHandlerApi.AuthContext 
 
authContext,
Index: 
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/FakeProcessor.java
===================================================================
--- 
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/FakeProcessor.java

 
(revision 1079640)
+++ 
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/FakeProcessor.java

 
(working copy)
@@ -36,6 +36,7 @@
    protected final Map<Uri, String> gadgets = Maps.newHashMap();
    public static final Uri SPEC_URL = Uri.parse("http://example.org/g.xml
");
    public static final Uri SPEC_URL2 = 
Uri.parse("http://example.org/g2.xml");
+  public static final Uri SPEC_URL3 = 
Uri.parse("http://example.org/g3.xml");
    public static final String SPEC_TITLE = "JSON-TEST";
    public static final String SPEC_TITLE2 = "JSON-TEST2";
    public static final int PREFERRED_HEIGHT = 100;
@@ -65,6 +66,12 @@
            "<Content type=\"html\">Hello, world</Content>" +
            "</Module>";

+  public static final String SPEC_XML3 =
+      "<Module>" +
+      "<ModulePrefs title=\"" + SPEC_TITLE2 + "\"/>" +
+      "<Content name=\"canvas\">Hello, world</Content>" +
+      "</Module>";
+
    private final FeatureRegistry featureRegistry;

    public FakeProcessor() {
@@ -75,6 +82,7 @@
      super(null, null, null, null, null);
      this.gadgets.put(FakeProcessor.SPEC_URL, FakeProcessor.SPEC_XML);
      this.gadgets.put(FakeProcessor.SPEC_URL2, FakeProcessor.SPEC_XML2);
+    this.gadgets.put(FakeProcessor.SPEC_URL3, FakeProcessor.SPEC_XML3);
      this.featureRegistry = featureRegistry;
    }

Index: 
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java
===================================================================
--- 
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java

 
(revision 1079640)
+++ 
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java

 
(working copy)
@@ -105,7 +105,7 @@
    @Test
    public void testGetMetadata() throws Exception {
      GadgetsHandlerApi.MetadataRequest request = createMetadataRequest(
-        FakeProcessor.SPEC_URL, CONTAINER, "view",
+        FakeProcessor.SPEC_URL, CONTAINER, "default",
          createAuthContext(null, null), ImmutableList.of("*"));
      EasyMock.expect(mockRegistry.getFeatures(EasyMock.isA(List.class)))
          .andReturn(Lists.newArrayList("auth-refresh"));
@@ -145,6 +145,16 @@
    }

    @Test(expected = ProcessingException.class)
+  public void testGetMetadataNoView() throws Exception {
+    GadgetsHandlerApi.MetadataRequest request = createMetadataRequest(
+        FakeProcessor.SPEC_URL3, CONTAINER, "view",
+        createAuthContext(null, null), ImmutableList.of("*"));
+    replay();
+    GadgetsHandlerApi.MetadataResponse response =
+        gadgetHandler.getMetadata(request);
+  }
+
+  @Test(expected = ProcessingException.class)
    public void testGetMetadataNoContainer() throws Exception {
      GadgetsHandlerApi.MetadataRequest request = createMetadataRequest(
          FakeProcessor.SPEC_URL, null, null,
@@ -185,7 +195,7 @@
    @Test
    public void testGetMetadataNoToken() throws Exception {
      GadgetsHandlerApi.MetadataRequest request = createMetadataRequest(
-        FakeProcessor.SPEC_URL, CONTAINER, "view", null, 
ImmutableList.of("*"));
+        FakeProcessor.SPEC_URL, CONTAINER, "default", null, 
ImmutableList.of("*"));
      EasyMock.expect(mockRegistry.getFeatures(EasyMock.isA(List.class)))
          .andReturn(Lists.newArrayList("auth-refresh"));
      replay();

[mhermanto, 2011-03-11 22:19:55]

http://codereview.appspot.com/4272043/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
(right):

http://codereview.appspot.com/4272043/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:168:
throw new ProcessingException("View " + request.getView() + " does not exists",
Like strict checking here, and letting users decide what to do with exception.
s/exists/exist.

http://codereview.appspot.com/4272043/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:411:
return (request.getRenderingType() ==
GadgetsHandlerApi.RenderingType.IFRAME_CAJOLED);
Looks safe, related to this change?

http://codereview.appspot.com/4272043/diff/1/java/gadgets/src/test/java/org/a...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java
(right):

http://codereview.appspot.com/4272043/diff/1/java/gadgets/src/test/java/org/a...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java:150:
FakeProcessor.SPEC_URL3, CONTAINER, "view",
Suggesting to clarify with s/"view"/"invalid_view"/

[mhermanto, 2011-03-11 22:36:42]

On Fri, Mar 11, 2011 at 2:19 PM, <mhermanto@gmail.com> wrote:

>
>
>
http://codereview.appspot.com/4272043/diff/1/java/gadgets/src/main/java/org/a...
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
> (right):
>
>
>
http://codereview.appspot.com/4272043/diff/1/java/gadgets/src/main/java/org/a...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:168:
> throw new ProcessingException("View " + request.getView() + " does not
> exists",
> Like strict checking here, and letting users decide what to do with
> exception. s/exists/exist.
>

... instead of NPE'ing. LGTM.

>
>
>
http://codereview.appspot.com/4272043/diff/1/java/gadgets/src/main/java/org/a...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:411:
> return (request.getRenderingType() ==
> GadgetsHandlerApi.RenderingType.IFRAME_CAJOLED);
> Looks safe, related to this change?
>
>
>
http://codereview.appspot.com/4272043/diff/1/java/gadgets/src/test/java/org/a...
> File
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java
> (right):
>
>
>
http://codereview.appspot.com/4272043/diff/1/java/gadgets/src/test/java/org/a...
>
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java:150:
> FakeProcessor.SPEC_URL3, CONTAINER, "view",
> Suggesting to clarify with s/"view"/"invalid_view"/
>




> http://codereview.appspot.com/4272043/
>

[zhoresh, 2011-03-12 00:10:13]

Stanton, yes the extra message was a leak from my other pending change. I
submitted both, so it should work now.

http://codereview.appspot.com/4272043/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
(right):

http://codereview.appspot.com/4272043/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:411:
return (request.getRenderingType() ==
GadgetsHandlerApi.RenderingType.IFRAME_CAJOLED);
On 2011/03/11 22:19:55, mhermanto wrote:
> Looks safe, related to this change?

Grrr, I have another change that effect same file
Maybe I can get away with it by getting lgtm on both :-)

http://codereview.appspot.com/4272043/diff/1/java/gadgets/src/test/java/org/a...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java
(right):

http://codereview.appspot.com/4272043/diff/1/java/gadgets/src/test/java/org/a...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java:150:
FakeProcessor.SPEC_URL3, CONTAINER, "view",
On 2011/03/11 22:19:55, mhermanto wrote:
> Suggesting to clarify with s/"view"/"invalid_view"/

Done.