Issue 287970043: Restrict transforms to GSA, fully trusted clients (Closed)
|
Created:
8 years, 2 months ago by myk Modified:
1 year, 5 months ago CC:
connector-cr_google.com Visibility:
Public. |
DescriptionRestrict transforms to GSA, fully trusted clients
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed PJ's comments from Patch Set 1 #
Total comments: 16
MessagesTotal messages: 7
Thank you. https://codereview.appspot.com/287970043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/DocumentHandler.java (right): https://codereview.appspot.com/287970043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/DocumentHandler.java:772: .createPipeline(os, originalContentType, metadata); It kind of looks like this method wants to have one return at the end. What do you think? I think we should change the return on 769 and change the return on 771. https://codereview.appspot.com/287970043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/DocumentHandler.java:994: log.log(Level.FINER, "Not performing Acl Transform."); s/Acl/ACL/ s/Transform/transform/ https://codereview.appspot.com/287970043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/DocumentHandlerTest.java (right): https://codereview.appspot.com/287970043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/DocumentHandlerTest.java:442: .setSamlServiceProvider(samlServiceProvider) do we need the samlServiceProvider? https://codereview.appspot.com/287970043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/DocumentHandlerTest.java:451: ex.getResponseHeaders().getFirst("X-Gsa-External-Metadata")); add check that this is also all the metadata https://codereview.appspot.com/287970043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/DocumentHandlerTest.java:790: .setSamlServiceProvider(samlServiceProvider) is saml provider necessary? https://codereview.appspot.com/287970043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/DocumentHandlerTest.java:885: .setSamlServiceProvider(samlServiceProvider) do we need saml service provider?
Sign in to reply to this message.
Made all recommended changes to DocumentHandler.java; made change that was possible (and explained why the others were not) in DocumentHandlerTest.java. https://codereview.appspot.com/287970043/diff/1/src/com/google/enterprise/ada... File src/com/google/enterprise/adaptor/DocumentHandler.java (right): https://codereview.appspot.com/287970043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/DocumentHandler.java:772: .createPipeline(os, originalContentType, metadata); On 2016/01/29 06:03:51, pjo wrote: > It kind of looks like this method wants to have one return at the end. What do > you think? > > I think we should change the return on 769 and change the return on 771. Done. https://codereview.appspot.com/287970043/diff/1/src/com/google/enterprise/ada... src/com/google/enterprise/adaptor/DocumentHandler.java:994: log.log(Level.FINER, "Not performing Acl Transform."); On 2016/01/29 06:03:51, pjo wrote: > s/Acl/ACL/ > s/Transform/transform/ Done. https://codereview.appspot.com/287970043/diff/1/test/com/google/enterprise/ad... File test/com/google/enterprise/adaptor/DocumentHandlerTest.java (right): https://codereview.appspot.com/287970043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/DocumentHandlerTest.java:442: .setSamlServiceProvider(samlServiceProvider) On 2016/01/29 06:03:51, pjo wrote: > do we need the samlServiceProvider? Without it, DocumentHandler.authzed() returns false, and thus handler.handle(ex) will fail. So, short answer -- We *do* need it. https://codereview.appspot.com/287970043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/DocumentHandlerTest.java:451: ex.getResponseHeaders().getFirst("X-Gsa-External-Metadata")); On 2016/01/29 06:03:51, pjo wrote: > add check that this is also all the metadata Done. (Changed getFirst() to get(), and made the expected value a List containing the expected string). https://codereview.appspot.com/287970043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/DocumentHandlerTest.java:790: .setSamlServiceProvider(samlServiceProvider) On 2016/01/29 06:03:51, pjo wrote: > is saml provider necessary? same as above -- yes https://codereview.appspot.com/287970043/diff/1/test/com/google/enterprise/ad... test/com/google/enterprise/adaptor/DocumentHandlerTest.java:885: .setSamlServiceProvider(samlServiceProvider) On 2016/01/29 06:03:51, pjo wrote: > do we need saml service provider? same as above -- yes.
Sign in to reply to this message.
PJ, Please take another look at this today. Thanks!
Sign in to reply to this message.
LGTM. Thank you. I think these comments are mostly for a follow up CL which would clarify that previous transform word refered to metadata. If you do make some changes please ask for another review. If you submit then please make follow up with renames. Thank you https://codereview.appspot.com/287970043/diff/20001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/DocumentHandler.java (right): https://codereview.appspot.com/287970043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/DocumentHandler.java:244: boolean considerSkippingTransforms(HttpExchange ex) { why this location in the file for this method? https://codereview.appspot.com/287970043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/DocumentHandler.java:697: transform(); s/transform()/transformMetadata()/ https://codereview.appspot.com/287970043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/DocumentHandler.java:813: return; i liked the else. there was a nice symmetry in the assignment that was visible. this version is good too. https://codereview.appspot.com/287970043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/DocumentHandler.java:815: transformedContentType = contentTransformFactory maybe call transfromedContentType finalContentType? or deliveredContentType? https://codereview.appspot.com/287970043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/DocumentHandler.java:1184: if (!requestIsFromFullyTrustedClient(ex)) { Looks like this check is now redundant. I am thinking lets leave it tho. https://codereview.appspot.com/287970043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/DocumentHandler.java:1218: "unexpected state for transform: " + state); s/transform/metadata transform/ https://codereview.appspot.com/287970043/diff/20001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/DocumentHandlerTest.java (right): https://codereview.appspot.com/287970043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/DocumentHandlerTest.java:441: .setTransform(transform) s/setTransform/setMetadataTransform/ can be in follow up CL https://codereview.appspot.com/287970043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/DocumentHandlerTest.java:450: assertEquals(Arrays.asList("testing%20key=testing%20value", ""), There is a 2nd empty item?
Sign in to reply to this message.
Answering your questions here, but not making any code changes since Patch Set 2. All requested code changes will be in a follow-on CL. https://codereview.appspot.com/287970043/diff/20001/src/com/google/enterprise... File src/com/google/enterprise/adaptor/DocumentHandler.java (right): https://codereview.appspot.com/287970043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/DocumentHandler.java:244: boolean considerSkippingTransforms(HttpExchange ex) { On 2016/02/02 02:22:07, pjo wrote: > why this location in the file for this method? It was a tossup between here (just after the requestIsFromFullyTrustedClient method) or just before/just after the "considerNotSending" method. At the end, it seemed more "tied into" requestIsFromFullyTrustedClient() [which really is the whole of this method] than similar to "considerNotSending" (which is a void and not a boolean). If you think it belongs closer to the transform() method, which is calling this one to determine whether or not to do it, that's a third option. https://codereview.appspot.com/287970043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/DocumentHandler.java:697: transform(); On 2016/02/02 02:22:07, pjo wrote: > s/transform()/transformMetadata()/ will do this in the follow-up CL. https://codereview.appspot.com/287970043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/DocumentHandler.java:813: return; On 2016/02/02 02:22:07, pjo wrote: > i liked the else. there was a nice symmetry in the assignment that was visible. > this version is good too. Acknowledged. https://codereview.appspot.com/287970043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/DocumentHandler.java:815: transformedContentType = contentTransformFactory On 2016/02/02 02:22:07, pjo wrote: > maybe call transfromedContentType finalContentType? or deliveredContentType? I prefer the former -- will make the change in the follow-on CL. https://codereview.appspot.com/287970043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/DocumentHandler.java:1184: if (!requestIsFromFullyTrustedClient(ex)) { On 2016/02/02 02:22:07, pjo wrote: > Looks like this check is now redundant. I am thinking lets leave it tho. Acknowledged. https://codereview.appspot.com/287970043/diff/20001/src/com/google/enterprise... src/com/google/enterprise/adaptor/DocumentHandler.java:1218: "unexpected state for transform: " + state); On 2016/02/02 02:22:07, pjo wrote: > s/transform/metadata transform/ Will do (in the follow-on CL). https://codereview.appspot.com/287970043/diff/20001/test/com/google/enterpris... File test/com/google/enterprise/adaptor/DocumentHandlerTest.java (right): https://codereview.appspot.com/287970043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/DocumentHandlerTest.java:441: .setTransform(transform) On 2016/02/02 02:22:07, pjo wrote: > s/setTransform/setMetadataTransform/ > > can be in follow up CL Acknowledged. https://codereview.appspot.com/287970043/diff/20001/test/com/google/enterpris... test/com/google/enterprise/adaptor/DocumentHandlerTest.java:450: assertEquals(Arrays.asList("testing%20key=testing%20value", ""), On 2016/02/02 02:22:07, pjo wrote: > There is a 2nd empty item? Yes. X-Gsa-External-Metadata is always an array of 2 strings. For nearly all the test cases in this suite (that don't simply call .getFirst()), at least one of the two is the empty string. testMetadataGsa() is the one exception where both strings are non-empty.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||
