Issue 850046: make trusted parameters override existing parameters
|
Created:
14 years ago by jcai Modified:
14 years ago Base URL:
http://svn.apache.org/repos/asf/shindig/trunk/ Visibility:
Public. |
Patch Set 1 #Patch Set 2 : a #
Total comments: 19
Patch Set 3 : aa #
Total comments: 2
MessagesTotal messages: 11
overrideParameters is using an N^2 algorithm due to searches and removals from an ArrayList. The method should be linear time. I'm also not sure the code needs to be this complicated. Can it just be params.addAll(trustedParams)? Variables called "overrided" -> overridden Variable names should be camelCase instead of with_underbars. Please leave "testTrustedParams" test unmodified, add new tests instead. Please add test cases for: - setting params that don't start with opensocial/oauth/xoauth. Test what happens if a caller specifies "foo=bar". I don't care what happens for this case, but I do want the behavior tested so it doesn't change accidentally. - setting multiple parameters with the same name. I don't care what happens for this case, but I do want the behavior tested so it doesn't change accidentally. - overriding special parameters like oauth_nonce I don't care what happens for this case, but I do want the behavior tested so it doesn't change accidentally. - trusted parameters when signOwner and signViewer are both false I do care about this - the parameters should be sent no matter what. - trusted parameters that start with oauth, xoauth, opensocial I do care about this - setting all of those parameters should work.
Sign in to reply to this message.
On 2010/04/14 21:17:56, beaton wrote: > overrideParameters is using an N^2 algorithm due to searches and > removals from an ArrayList. The method should be linear time. > > I'm also not sure the code needs to be this complicated. Can it just > be params.addAll(trustedParams)? The code is subtle here. I want to override&deduplicate all oauth_* xoauth-* opensocial_* and don't affect parameters in url and postbody, since many unittest depends on duplicated paramaters in url or postbody. addAll is definitely not working, it just append to the end of a list, no override. > > Variables called "overrided" -> overridden done > > Variable names should be camelCase instead of with_underbars. done > > > Please leave "testTrustedParams" test unmodified, add new tests instead. Change a little bit here is inevitably, since logic in FakeOAuthServiceProvider has changed. > > Please add test cases for: > - setting params that don't start with opensocial/oauth/xoauth. Test > what happens if a caller specifies "foo=bar". > I don't care what happens for this case, but I do want the behavior > tested so it doesn't change accidentally. signature check fail > > - setting multiple parameters with the same name. > I don't care what happens for this case, but I do want the behavior > tested so it doesn't change accidentally. the latter will win. > > - overriding special parameters like oauth_nonce > I don't care what happens for this case, but I do want the behavior > tested so it doesn't change accidentally. > > - trusted parameters when signOwner and signViewer are both false > I do care about this - the parameters should be sent no matter what. > > - trusted parameters that start with oauth, xoauth, opensocial > I do care about this - setting all of those parameters should work. >
Sign in to reply to this message.
http://codereview.appspot.com/850046/diff/4001/5002 File java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java (right): http://codereview.appspot.com/850046/diff/4001/5002#newcode408 java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java:408: private void overrideParameters(List<Parameter> params) { rename this parameter authParams? http://codereview.appspot.com/850046/diff/4001/5002#newcode414 java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java:414: for (int i = 0; i < params.size(); i++) { Use "for (Parameter param : params)" instead of array index. http://codereview.appspot.com/850046/diff/4001/5002#newcode418 java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java:418: for (int i = 0; i < trustedParams.size(); i++) { for (Parameter param : trustedParams) http://codereview.appspot.com/850046/diff/4001/5002#newcode425 java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java:425: while (it.hasNext()) { for (String key : paramMap.keySet()) http://codereview.appspot.com/850046/diff/4001/5001 File java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthRequestTest.java (right): http://codereview.appspot.com/850046/diff/4001/5001#newcode1945 java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthRequestTest.java:1945: * 4) trusted parameter can override existing parameter. 3, not 4? http://codereview.appspot.com/850046/diff/4001/5001#newcode1946 java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthRequestTest.java:1946: * @throws Exception drop @throws http://codereview.appspot.com/850046/diff/4001/5001#newcode1972 java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthRequestTest.java:1972: * @throws Exception drop @throws, it's not important enough to put in a comment. http://codereview.appspot.com/850046/diff/4001/5001#newcode2004 java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthRequestTest.java:2004: HttpResponse response = client.sendGet(FakeOAuthServiceProvider.RESOURCE_URL); OK, so I know I said that whatever behavior is fine, but it would be nicer for developers if we blocked them from setting "foo" as a trusted parameter rather than just getting the signature wrong. Let's modify OAuthRequest to return an error if someone passes a trusted param that is not oauth/opensocial/xoauth. You can use the same error handling as is in sanitize(). throw responseParams.oauthRequestException(OAuthError.INVALID_REQUEST, "invalid parameter name " + name + "invalid parameter name " + name + ", applications may not override opensocial or oauth parameters"); ", applications may not override opensocial or oauth parameters");
Sign in to reply to this message.
http://codereview.appspot.com/850046/diff/4001/5002 File java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java (right): http://codereview.appspot.com/850046/diff/4001/5002#newcode408 java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java:408: private void overrideParameters(List<Parameter> params) { On 2010/04/15 02:40:19, beaton wrote: > rename this parameter authParams? Done. http://codereview.appspot.com/850046/diff/4001/5002#newcode414 java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java:414: for (int i = 0; i < params.size(); i++) { On 2010/04/15 02:40:19, beaton wrote: > Use "for (Parameter param : params)" instead of array index. Done. http://codereview.appspot.com/850046/diff/4001/5002#newcode418 java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java:418: for (int i = 0; i < trustedParams.size(); i++) { On 2010/04/15 02:40:19, beaton wrote: > for (Parameter param : trustedParams) Done. http://codereview.appspot.com/850046/diff/4001/5002#newcode425 java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java:425: while (it.hasNext()) { On 2010/04/15 02:40:19, beaton wrote: > for (String key : paramMap.keySet()) Done. http://codereview.appspot.com/850046/diff/4001/5001 File java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthRequestTest.java (right): http://codereview.appspot.com/850046/diff/4001/5001#newcode1945 java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthRequestTest.java:1945: * 4) trusted parameter can override existing parameter. On 2010/04/15 02:40:19, beaton wrote: > 3, not 4? Done. http://codereview.appspot.com/850046/diff/4001/5001#newcode1946 java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthRequestTest.java:1946: * @throws Exception On 2010/04/15 02:40:19, beaton wrote: > drop @throws Done. http://codereview.appspot.com/850046/diff/4001/5001#newcode1972 java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthRequestTest.java:1972: * @throws Exception On 2010/04/15 02:40:19, beaton wrote: > drop @throws, it's not important enough to put in a comment. removed ..generated by eclipse. http://codereview.appspot.com/850046/diff/4001/5001#newcode2004 java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthRequestTest.java:2004: HttpResponse response = client.sendGet(FakeOAuthServiceProvider.RESOURCE_URL); On 2010/04/15 02:40:19, beaton wrote: > OK, so I know I said that whatever behavior is fine, but it would be nicer for > developers if we blocked them from setting "foo" as a trusted parameter rather > than just getting the signature wrong. Let's modify OAuthRequest to return an > error if someone passes a trusted param that is not oauth/opensocial/xoauth. > > You can use the same error handling as is in sanitize(). > > throw responseParams.oauthRequestException(OAuthError.INVALID_REQUEST, > "invalid parameter name " + name + "invalid parameter name " + name > + > ", applications may not override opensocial or oauth parameters"); > ", applications may not override opensocial or oauth parameters"); done test updated
Sign in to reply to this message.
http://codereview.appspot.com/850046/diff/4001/5002 File java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java (right): http://codereview.appspot.com/850046/diff/4001/5002#newcode425 java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java:425: while (it.hasNext()) { Maybe use for (Map.Entry<String, String> param : paramMap.entrySet()) { params.add(new Parameter(param.getKey(), param.getValue()); } Personal preference here though. Does this retain order? If the method is accepting a List then it is implied that order is retained but params seem to be added at the end and we're using a LinkedHashMap which afaik is unordered. http://codereview.appspot.com/850046/diff/4001/5001 File java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthRequestTest.java (right): http://codereview.appspot.com/850046/diff/4001/5001#newcode1974 java/gadgets/src/test/java/org/apache/shindig/gadgets/oauth/OAuthRequestTest.java:1974: @Test If we are checking for a specific exception it may be better to explicitly specify this in the annotation with @Test(expected = SpecificException.class) and then, rather than using the try {} catch {} block, putting a fail("Exception expected") or something. At the very least, we should be putting a fail() in either the end of the try {} block or in the catch{} block because at the moment either behaviour is expected.
Sign in to reply to this message.
http://codereview.appspot.com/850046/diff/4001/5002 File java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java (right): http://codereview.appspot.com/850046/diff/4001/5002#newcode425 java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java:425: while (it.hasNext()) { On 2010/04/15 07:45:25, mat.mannion wrote: > Maybe use > > for (Map.Entry<String, String> param : paramMap.entrySet()) { > params.add(new Parameter(param.getKey(), param.getValue()); > } done:) you should take a look at the new snapshot. > > Personal preference here though. Does this retain order? If the method is > accepting a List then it is implied that order is retained but params seem to be > added at the end and we're using a LinkedHashMap which afaik is unordered. Iteration order on LinkedHashMap is the insertion order, I am trying to maintain the order here.
Sign in to reply to this message.
http://codereview.appspot.com/850046/diff/9001/7003 File java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java (right): http://codereview.appspot.com/850046/diff/9001/7003#newcode424 java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java:424: + ", trusted parameter must start with 'oauth' 'xoauth'or 'opensocial' "); Minor nit. I'm not sure if this is entirely correct. Shouldn't trusted oauth parameters be prefixed with oauth_? http://tools.ietf.org/html/draft-hammer-oauth-10#section-3.5 http://tools.ietf.org/html/draft-hammer-oauth-10#section-3.1 http://codereview.appspot.com/850046/diff/9001/7003#newcode552 java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java:552: // authParams are parameters prefixed with 'xoauth_' 'oauth_' or 'opensocial_', This isn't consistent with line 424 and 398.
Sign in to reply to this message.
Fixed the comment. Re: trusted parameters start with "oauth" vs "xoauth" or "opensocial"... that ship has long-since sailed.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
