Re: Add support for RpcTokens, which, if set, are sent with each RPCRequest to (issue1107801)

[j...@google.com]

You have line length issues in various places.

http://gwt-code-reviews.appspot.com/1107801/diff/1/7
File
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStream.java
(right):

http://gwt-code-reviews.appspot.com/1107801/diff/1/7#newcode52
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStream.java:52:
public static final int SERIALIZATION_STREAM_MIN_VERSION = 5;
I think the version number needs to be bumped, since I don't believe the
implementation fails if any unknown flag bits are set.

To avoid having to bump the version in similar cases in the future, it
should probably check if any unknown flags values are set and fail the
same was as a protocol version mismatch.

However, I remember complications when Dan Rice increased the version
number for changing the long representation, so we should look back over
those and see if we can avoid the problems.

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4002
File user/src/com/google/gwt/user/client/rpc/RpcToken.java (right):

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4002#newcode37
user/src/com/google/gwt/user/client/rpc/RpcToken.java:37: public
@interface Class {
Since usually people will have this imported, I would prefer a better
name - @Class on an interface isn't going to be clear what it refers to.
How about @RpcTokenImplementation?

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4008
File
user/src/com/google/gwt/user/client/rpc/impl/RequestCallbackAdapter.java
(right):

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4008#newcode170
user/src/com/google/gwt/user/client/rpc/impl/RequestCallbackAdapter.java:170:
String methodName, RpcStatsContext statsContext, AsyncCallback<T>
callback,
Line length > 80.

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4008#newcode172
user/src/com/google/gwt/user/client/rpc/impl/RequestCallbackAdapter.java:172:
this(streamFactory, methodName, statsContext, callback, null,
responseReader);
Line length > 80.

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4008#newcode230
user/src/com/google/gwt/user/client/rpc/impl/RequestCallbackAdapter.java:230:
} else if (tokenExceptionHandler != null && caught instanceof
RpcTokenException) {
Line length.

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4009
File user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java (right):

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4009#newcode189
user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java:189:
stob.addRootType(logger, rteType);
Should this be conditional on finding an RpcToken subtype?

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4009#newcode497
user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java:497: //
setRpcToken()
Should this check instead be done at setRpcToken time? Perhaps have
setRpcToken call protected boolean checkRpcTokenType(RpcToken) which by
default returns true, and generate a check there?

I know I previously said over chat that a cast was fine, but thinking
about it more I think we need a better error here. So, I think you want
an instanceof check and then throw an exception with more details about
the problem.

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4009#newcode593
user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java:593:
returnStatement.append("return new " +
FailingRequestBuilder.class.getName() + "("
If you are going to use a StringBuffer here, don't mix regular + -- use
append to add the pieces together.

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4009#newcode605
user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java:605:
w.println("} catch(ClassCastException " + exceptionName + " ) {");
I think if the RpcToken type is checked at setRpcToken time, all of
these changes aren't needed, right?

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4010
File user/src/com/google/gwt/user/server/rpc/HybridServiceServlet.java
(right):

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4010#newcode137
user/src/com/google/gwt/user/server/rpc/HybridServiceServlet.java:137:
log("An RpcTokenException was thrown while processing this call.",
tokenException);
Line length.

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4018
File user/test/com/google/gwt/user/client/rpc/RpcTokenTest.java (right):

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4018#newcode67
user/test/com/google/gwt/user/client/rpc/RpcTokenTest.java:67: fail();
Should include the exception to help debugging a failure just from the
logs.

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4018#newcode133
user/test/com/google/gwt/user/client/rpc/RpcTokenTest.java:133:
((ServiceDefTarget) service).setRpcToken(token);
As mentioned earlier, it really seems like this error should be caught
here, rather than when the service is actually called.

http://gwt-code-reviews.appspot.com/1107801/show

[me...@google.com]

http://gwt-code-reviews.appspot.com/1107801/show

[me...@google.com]

http://gwt-code-reviews.appspot.com/1107801/show

[me...@google.com]

http://gwt-code-reviews.appspot.com/1107801/diff/1/7
File
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStream.java
(right):

http://gwt-code-reviews.appspot.com/1107801/diff/1/7#newcode52
user/src/com/google/gwt/user/client/rpc/impl/AbstractSerializationStream.java:52:
public static final int SERIALIZATION_STREAM_MIN_VERSION = 5;

On 2010/11/15 16:02:21, jat wrote:
> I think the version number needs to be bumped, since I don't believe
the
> implementation fails if any unknown flag bits are set.

Version bumped.

> To avoid having to bump the version in similar cases in the future, it
should
> probably check if any unknown flags values are set and fail the same
was as a
> protocol version mismatch.

Added checks for unknown flags.

> However, I remember complications when Dan Rice increased the version
number for
> changing the long representation, so we should look back over those
and see if
> we can avoid the problems.

Any pointers for this?

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4002
File user/src/com/google/gwt/user/client/rpc/RpcToken.java (right):

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4002#newcode37
user/src/com/google/gwt/user/client/rpc/RpcToken.java:37: public
@interface Class {

On 2010/11/15 16:02:21, jat wrote:
> Since usually people will have this imported, I would prefer a better
name -
> @Class on an interface isn't going to be clear what it refers to. How
about
> @RpcTokenImplementation?

Done.

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4008
File
user/src/com/google/gwt/user/client/rpc/impl/RequestCallbackAdapter.java
(right):

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4008#newcode170
user/src/com/google/gwt/user/client/rpc/impl/RequestCallbackAdapter.java:170:
String methodName, RpcStatsContext statsContext, AsyncCallback<T>
callback,

On 2010/11/15 16:02:21, jat wrote:
> Line length > 80.

Done.

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4008#newcode172
user/src/com/google/gwt/user/client/rpc/impl/RequestCallbackAdapter.java:172:
this(streamFactory, methodName, statsContext, callback, null,
responseReader);

On 2010/11/15 16:02:21, jat wrote:
> Line length > 80.

Done.

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4008#newcode230
user/src/com/google/gwt/user/client/rpc/impl/RequestCallbackAdapter.java:230:
} else if (tokenExceptionHandler != null && caught instanceof
RpcTokenException) {

On 2010/11/15 16:02:21, jat wrote:
> Line length.

Done.

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4009
File user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java (right):

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4009#newcode189
user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java:189:
stob.addRootType(logger, rteType);

On 2010/11/15 16:02:21, jat wrote:
> Should this be conditional on finding an RpcToken subtype?

Done.

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4009#newcode497
user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java:497: //
setRpcToken()

On 2010/11/15 16:02:21, jat wrote:
> Should this check instead be done at setRpcToken time? Perhaps have
setRpcToken
> call protected boolean checkRpcTokenType(RpcToken) which by default
returns
> true, and generate a check there?

> I know I previously said over chat that a cast was fine, but thinking
about it
> more I think we need a better error here. So, I think you want an
instanceof
> check and then throw an exception with more details about the problem.

Done.

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4009#newcode593
user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java:593:
returnStatement.append("return new " +
FailingRequestBuilder.class.getName() + "("

On 2010/11/15 16:02:21, jat wrote:
> If you are going to use a StringBuffer here, don't mix regular + --
use append
> to add the pieces together.

Done.

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4009#newcode605
user/src/com/google/gwt/user/rebind/rpc/ProxyCreator.java:605:
w.println("} catch(ClassCastException " + exceptionName + " ) {");

On 2010/11/15 16:02:21, jat wrote:
> I think if the RpcToken type is checked at setRpcToken time, all of
these
> changes aren't needed, right?

Done.

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4010
File user/src/com/google/gwt/user/server/rpc/HybridServiceServlet.java
(right):

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4010#newcode137
user/src/com/google/gwt/user/server/rpc/HybridServiceServlet.java:137:
log("An RpcTokenException was thrown while processing this call.",
tokenException);

On 2010/11/15 16:02:21, jat wrote:
> Line length.

Done.

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4018
File user/test/com/google/gwt/user/client/rpc/RpcTokenTest.java (right):

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4018#newcode67
user/test/com/google/gwt/user/client/rpc/RpcTokenTest.java:67: fail();

On 2010/11/15 16:02:21, jat wrote:
> Should include the exception to help debugging a failure just from the
logs.

Done.

http://gwt-code-reviews.appspot.com/1107801/diff/3001/4018#newcode133
user/test/com/google/gwt/user/client/rpc/RpcTokenTest.java:133:
((ServiceDefTarget) service).setRpcToken(token);

On 2010/11/15 16:02:21, jat wrote:
> As mentioned earlier, it really seems like this error should be caught
here,
> rather than when the service is actually called.

Done.

http://gwt-code-reviews.appspot.com/1107801/show

[BobV]

Has anyone looked at applying this to RequestFactory? Would it be
sufficient for the RequestTransport to maintain the token state?

--
Bob Vawter
Google Web Toolkit Team

[Meder Kydyraliev]

I haven't look at RequestFactory, but it should be possible to have RequestFactory propagate the object to RequestContext and include it in the payload.

[Ray Ryan]

Meder, are you able to take on the RequestFactory task?

[BobV]

I guess the big question is whether or not the XSRF protection is a
function of the payload envelope or needs deeper support. If it can
be done at the transport layer, extending DefaultRequestTransport
seems like easy way to mix it in.

[Meder Kydyraliev]

On Thu, Nov 18, 2010 at 6:23 AM, Ray Ryan <rj...@google.com> wrote:

Meder, are you able to take on the RequestFactory task?

Yes I can do it, should it be in a separate CL? 

[Ray Ryan]

Yes please.

[Meder Kydyraliev]

On Thu, Nov 18, 2010 at 6:35 AM, BobV <bo...@google.com> wrote:

I guess the big question is whether or not the XSRF protection is a
function of the payload envelope or needs deeper support.  If it can
be done at the transport layer, extending DefaultRequestTransport
seems like easy way to mix it in.

RPC XSRF protection allows for arbitrary serializable implementations to be sent, I'll take a look at the DefaultRequestTransport and see where would be the best place to add this. I'll get on it once this code is in.

Meder