You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@shindig.apache.org by Stanton Sievers <si...@gmail.com> on 2011/09/20 18:09:13 UTC

Review Request: BlobCrypterSecurityTokenCodec tries to use "instanceof" when the parameter is a Proxied object

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1981/
-----------------------------------------------------------

Review request for shindig and Henry Saputra.

Summary
-------

See the JIRA for a description of the problem: https://issues.apache.org/jira/browse/SHINDIG-1626

This fix is based off a fix Doug Davies implemented with some changes around the parameter checking in BlobCrypterSecurityToken.encodeToken. The check is sufficient because DefaultSecurityTokenCodec creates the correct SecurityTokenCode (Basic or Blob) depending on the container config values of "insecure" or "secure", respectively. We should never get into this code if we're not using a secure configuration; therefore, an authentication mode of SECURITY_TOKEN_URL_PARAMETER implies that we have a BlobCrypterSecurityToken and not some other token, such as Anonymous.

This addresses bug SHINDIG-1626.
https://issues.apache.org/jira/browse/SHINDIG-1626

Diffs
-----

http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1173205
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1173205

Diff: https://reviews.apache.org/r/1981/diff

Testing
-------

Tested with a sample gadget that utilizes the osapi feature to print the viewer's name in a secure configuration. The security token is encoded properly in the modified code.

Any other testing recommendations are welcome. :)

Thanks,

Stanton

Re: Review Request: BlobCrypterSecurityTokenCodec tries to use "instanceof"when the parameter is a Proxied object

Posted by daviesd <da...@oclc.org>.

Thanks for getting this committed.  I am now able to store my other info in
trustedJson and have accessible in the gadget security token. Nice!

doug

On 9/21/11 4:24 PM, "Henry Saputra" <he...@gmail.com> wrote:

> Doug, your vote count, thanks =)
> 
> Any review to help patches better is welcomed.
> 
> - Henry
> 
>

Re: Review Request: BlobCrypterSecurityTokenCodec tries to use "instanceof"when the parameter is a Proxied object

Posted by Henry Saputra <he...@gmail.com>.

Doug, your vote count, thanks =)

Any review to help patches better is welcomed.

- Henry

On Wed, Sep 21, 2011 at 1:13 PM, daviesd <da...@oclc.org> wrote:
> Not that I'm allowd to vote, but yes I'd love to have this in the next build
> asap.
>
> doug
>
>
> On 9/21/11 2:13 PM, "Henry Saputra" <hs...@apache.org> wrote:
>
>>
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/1981/#review2005
>> -----------------------------------------------------------
>>
>> Ship it!
>>
>>
>> +1
>>
>> LGTM, thanks Stanton.
>>
>> We have to add the new getter methods to AuthContext, I dont see other way
>>
>> - Henry
>>
>>
>> On 2011-09-21 16:28:56, Stanton Sievers wrote:
>>>
>>> -----------------------------------------------------------
>>> This is an automatically generated e-mail. To reply, visit:
>>> https://reviews.apache.org/r/1981/
>>> -----------------------------------------------------------
>>>
>>> (Updated 2011-09-21 16:28:56)
>>>
>>>
>>> Review request for shindig and Henry Saputra.
>>>
>>>
>>> Summary
>>> -------
>>>
>>> See the JIRA for a description of the problem:
>>> https://issues.apache.org/jira/browse/SHINDIG-1626
>>>
>>> This fix is based off a fix Doug Davies implemented with some changes around
>>> the parameter checking in BlobCrypterSecurityToken.encodeToken.  The check is
>>> sufficient because DefaultSecurityTokenCodec creates the correct
>>> SecurityTokenCode (Basic or Blob) depending on the container config values of
>>> "insecure" or "secure", respectively.  We should never get into this code if
>>> we're not using a secure configuration; therefore, an authentication mode of
>>> SECURITY_TOKEN_URL_PARAMETER implies that we have a BlobCrypterSecurityToken
>>> and not some other token, such as Anonymous.
>>>
>>>
>>> This addresses bug SHINDIG-1626.
>>>     https://issues.apache.org/jira/browse/SHINDIG-1626
>>>
>>>
>>> Diffs
>>> -----
>>>
>>>
>>> http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/a
>>> pache/shindig/auth/BlobCrypterSecurityToken.java 1173205
>>>
>>> http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/a
>>> pache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1173205
>>>
>>> http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/a
>>> pache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1173205
>>>
>>> http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/a
>>> pache/shindig/auth/BlobCrypterSecurityTokenTest.java 1173205
>>>
>>> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/
>>> apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1173205
>>>
>>> Diff: https://reviews.apache.org/r/1981/diff
>>>
>>>
>>> Testing
>>> -------
>>>
>>> Tested with a sample gadget that utilizes the osapi feature to print the
>>> viewer's name in a secure configuration.  The security token is encoded
>>> properly in the modified code.
>>>
>>> Any other testing recommendations are welcome. :)
>>>
>>>
>>> Thanks,
>>>
>>> Stanton
>>>
>>>
>>
>
>
>

Re: Review Request: BlobCrypterSecurityTokenCodec tries to use "instanceof"when the parameter is a Proxied object

Posted by daviesd <da...@oclc.org>.

Not that I'm allowd to vote, but yes I'd love to have this in the next build
asap.

doug


On 9/21/11 2:13 PM, "Henry Saputra" <hs...@apache.org> wrote:

> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1981/#review2005
> -----------------------------------------------------------
> 
> Ship it!
> 
> 
> +1
> 
> LGTM, thanks Stanton.
> 
> We have to add the new getter methods to AuthContext, I dont see other way
> 
> - Henry
> 
> 
> On 2011-09-21 16:28:56, Stanton Sievers wrote:
>> 
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/1981/
>> -----------------------------------------------------------
>> 
>> (Updated 2011-09-21 16:28:56)
>> 
>> 
>> Review request for shindig and Henry Saputra.
>> 
>> 
>> Summary
>> -------
>> 
>> See the JIRA for a description of the problem:
>> https://issues.apache.org/jira/browse/SHINDIG-1626
>> 
>> This fix is based off a fix Doug Davies implemented with some changes around
>> the parameter checking in BlobCrypterSecurityToken.encodeToken.  The check is
>> sufficient because DefaultSecurityTokenCodec creates the correct
>> SecurityTokenCode (Basic or Blob) depending on the container config values of
>> "insecure" or "secure", respectively.  We should never get into this code if
>> we're not using a secure configuration; therefore, an authentication mode of
>> SECURITY_TOKEN_URL_PARAMETER implies that we have a BlobCrypterSecurityToken
>> and not some other token, such as Anonymous.
>> 
>> 
>> This addresses bug SHINDIG-1626.
>>     https://issues.apache.org/jira/browse/SHINDIG-1626
>> 
>> 
>> Diffs
>> -----
>> 
>>   
>> http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/a
>> pache/shindig/auth/BlobCrypterSecurityToken.java 1173205
>>   
>> http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/a
>> pache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1173205
>>   
>> http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/a
>> pache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1173205
>>   
>> http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/a
>> pache/shindig/auth/BlobCrypterSecurityTokenTest.java 1173205
>>   
>> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/
>> apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1173205
>> 
>> Diff: https://reviews.apache.org/r/1981/diff
>> 
>> 
>> Testing
>> -------
>> 
>> Tested with a sample gadget that utilizes the osapi feature to print the
>> viewer's name in a secure configuration.  The security token is encoded
>> properly in the modified code.
>> 
>> Any other testing recommendations are welcome. :)
>> 
>> 
>> Thanks,
>> 
>> Stanton
>> 
>> 
>

Re: Review Request: BlobCrypterSecurityTokenCodec tries to use "instanceof" when the parameter is a Proxied object

Posted by Henry Saputra <hs...@apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1981/#review2005
-----------------------------------------------------------

Ship it!


+1

LGTM, thanks Stanton.

We have to add the new getter methods to AuthContext, I dont see other way

- Henry


On 2011-09-21 16:28:56, Stanton Sievers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1981/
> -----------------------------------------------------------
> 
> (Updated 2011-09-21 16:28:56)
> 
> 
> Review request for shindig and Henry Saputra.
> 
> 
> Summary
> -------
> 
> See the JIRA for a description of the problem: https://issues.apache.org/jira/browse/SHINDIG-1626
> 
> This fix is based off a fix Doug Davies implemented with some changes around the parameter checking in BlobCrypterSecurityToken.encodeToken.  The check is sufficient because DefaultSecurityTokenCodec creates the correct SecurityTokenCode (Basic or Blob) depending on the container config values of "insecure" or "secure", respectively.  We should never get into this code if we're not using a secure configuration; therefore, an authentication mode of SECURITY_TOKEN_URL_PARAMETER implies that we have a BlobCrypterSecurityToken and not some other token, such as Anonymous.
> 
> 
> This addresses bug SHINDIG-1626.
>     https://issues.apache.org/jira/browse/SHINDIG-1626
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityToken.java 1173205 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1173205 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1173205 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenTest.java 1173205 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1173205 
> 
> Diff: https://reviews.apache.org/r/1981/diff
> 
> 
> Testing
> -------
> 
> Tested with a sample gadget that utilizes the osapi feature to print the viewer's name in a secure configuration.  The security token is encoded properly in the modified code.
> 
> Any other testing recommendations are welcome. :)
> 
> 
> Thanks,
> 
> Stanton
> 
>

Re: Review Request: BlobCrypterSecurityTokenCodec tries to use "instanceof" when the parameter is a Proxied object

Posted by Stanton Sievers <si...@gmail.com>.

(Updated 2011-09-21 16:28:56.153311)

Review request for shindig and Henry Saputra.

Changes
-------

As promised, a new version that relies on the BlobCrypterSecurityTokenCodec passing in the BlobCrypter to the BlobCrypterSecurityToken. This allows BCSTC to call a static encrypt method on BCST instead of relying on instantiating a BCST itself.

We still have the problem of the AuthContext needing to have the appropriate getters. I don't see a way around this.

Summary
-------

See the JIRA for a description of the problem: https://issues.apache.org/jira/browse/SHINDIG-1626

This addresses bug SHINDIG-1626.
https://issues.apache.org/jira/browse/SHINDIG-1626

Diffs (updated)
-----

http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityToken.java 1173205
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1173205
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java 1173205
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenTest.java 1173205
http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1173205

Diff: https://reviews.apache.org/r/1981/diff

Testing
-------

Tested with a sample gadget that utilizes the osapi feature to print the viewer's name in a secure configuration. The security token is encoded properly in the modified code.

Any other testing recommendations are welcome. :)

Thanks,

Stanton

Re: Review Request: BlobCrypterSecurityTokenCodec tries to use "instanceof" when the parameter is a Proxied object

Posted by Stanton Sievers <si...@gmail.com>.


> On 2011-09-20 22:07:14, Henry Saputra wrote:
> > Yeah its getting hard to maintain. The only reason we want to cast it to BlobCrypterSecurityToken is to call BlobCrypterSecurityToken.encrypt. 
> > 
> > I think we should move out the encrypt and decrypt business out from token and delegate it to BlobCrypterSecurityTokenCodec. So BlobCrypterSecurityToken does not contain crypter at all.

I'm not sure that moving that logic out would solve the problem.  We still need the values map to pass to the crypter.  That values map is currently constructed in the BlobCrypterSecurityToken using the local fields.  We still can't get all of those values because we're proxied through an AuthContext interface.

I do like the idea of moving the knowledge of the crypter out of the BlobCrypterSecurityToken so the BlobCrypterSecurityTokenCodec doesn't have to construct the BlobCrypterSecurityToken.  Regardless, I think the data getters in SecurityToken are going to need to also be added to AuthContext.

I'll post a patch tomorrow that illustrates this idea if it doesn't quite make sense.


- Stanton


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1981/#review1981
-----------------------------------------------------------


On 2011-09-20 16:35:32, Stanton Sievers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1981/
> -----------------------------------------------------------
> 
> (Updated 2011-09-20 16:35:32)
> 
> 
> Review request for shindig and Henry Saputra.
> 
> 
> Summary
> -------
> 
> See the JIRA for a description of the problem: https://issues.apache.org/jira/browse/SHINDIG-1626
> 
> This fix is based off a fix Doug Davies implemented with some changes around the parameter checking in BlobCrypterSecurityToken.encodeToken.  The check is sufficient because DefaultSecurityTokenCodec creates the correct SecurityTokenCode (Basic or Blob) depending on the container config values of "insecure" or "secure", respectively.  We should never get into this code if we're not using a secure configuration; therefore, an authentication mode of SECURITY_TOKEN_URL_PARAMETER implies that we have a BlobCrypterSecurityToken and not some other token, such as Anonymous.
> 
> 
> This addresses bug SHINDIG-1626.
>     https://issues.apache.org/jira/browse/SHINDIG-1626
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1173205 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1173205 
> 
> Diff: https://reviews.apache.org/r/1981/diff
> 
> 
> Testing
> -------
> 
> Tested with a sample gadget that utilizes the osapi feature to print the viewer's name in a secure configuration.  The security token is encoded properly in the modified code.
> 
> Any other testing recommendations are welcome. :)
> 
> 
> Thanks,
> 
> Stanton
> 
>

Re: Review Request: BlobCrypterSecurityTokenCodec tries to use "instanceof" when the parameter is a Proxied object

Posted by Henry Saputra <hs...@apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1981/#review1981
-----------------------------------------------------------


Yeah its getting hard to maintain. The only reason we want to cast it to BlobCrypterSecurityToken is to call BlobCrypterSecurityToken.encrypt. 

I think we should move out the encrypt and decrypt business out from token and delegate it to BlobCrypterSecurityTokenCodec. So BlobCrypterSecurityToken does not contain crypter at all.

- Henry


On 2011-09-20 16:35:32, Stanton Sievers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1981/
> -----------------------------------------------------------
> 
> (Updated 2011-09-20 16:35:32)
> 
> 
> Review request for shindig and Henry Saputra.
> 
> 
> Summary
> -------
> 
> See the JIRA for a description of the problem: https://issues.apache.org/jira/browse/SHINDIG-1626
> 
> This fix is based off a fix Doug Davies implemented with some changes around the parameter checking in BlobCrypterSecurityToken.encodeToken.  The check is sufficient because DefaultSecurityTokenCodec creates the correct SecurityTokenCode (Basic or Blob) depending on the container config values of "insecure" or "secure", respectively.  We should never get into this code if we're not using a secure configuration; therefore, an authentication mode of SECURITY_TOKEN_URL_PARAMETER implies that we have a BlobCrypterSecurityToken and not some other token, such as Anonymous.
> 
> 
> This addresses bug SHINDIG-1626.
>     https://issues.apache.org/jira/browse/SHINDIG-1626
> 
> 
> Diffs
> -----
> 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1173205 
>   http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1173205 
> 
> Diff: https://reviews.apache.org/r/1981/diff
> 
> 
> Testing
> -------
> 
> Tested with a sample gadget that utilizes the osapi feature to print the viewer's name in a secure configuration.  The security token is encoded properly in the modified code.
> 
> Any other testing recommendations are welcome. :)
> 
> 
> Thanks,
> 
> Stanton
> 
>

Re: Review Request: BlobCrypterSecurityTokenCodec tries to use "instanceof" when the parameter is a Proxied object

Posted by Stanton Sievers <si...@gmail.com>.

(Updated 2011-09-20 16:35:32.884028)

Review request for shindig and Henry Saputra.

Changes
-------

In the first patch we were losing some of the data from the token. I've updated this to try to save as much data as possible. This is getting messy, however, because AuthContext's methods have started looking like a SecurityToken. More specifically it's getters now match the BlobCrypterSecurityToken's setters.

Is there a better way to do this? Presumably we need the proxy for the SecurityToken object in the first place.

Summary
-------

See the JIRA for a description of the problem: https://issues.apache.org/jira/browse/SHINDIG-1626

This addresses bug SHINDIG-1626.
https://issues.apache.org/jira/browse/SHINDIG-1626

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/1981/diff

Testing
-------

Tested with a sample gadget that utilizes the osapi feature to print the viewer's name in a secure configuration. The security token is encoded properly in the modified code.

Any other testing recommendations are welcome. :)

Thanks,

Stanton

Re: Review Request: BlobCrypterSecurityTokenCodec tries to use "instanceof" when the parameter is a Proxied object

Posted by daviesd <da...@oclc.org>.

I've run into a problem with this in OUR implementation (yours is fine).  We
add a couple of fields to the security token (OurBlobCrypterSecurityToken).
Thus I really need to cast to MY implementation to retrieve those values.
But I can't cast the proxy.  I don't want to add the getters to the base
SecurityContext interface.  I'm actually thinking I might need to use
reflection to get to the getters I need.  Yuck.

doug


On 9/20/11 12:09 PM, "Stanton Sievers" <si...@gmail.com> wrote:

> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/1981/
> -----------------------------------------------------------
> 
> Review request for shindig and Henry Saputra.
> 
> 
> Summary
> -------
> 
> See the JIRA for a description of the problem:
> https://issues.apache.org/jira/browse/SHINDIG-1626
> 
> This fix is based off a fix Doug Davies implemented with some changes around
> the parameter checking in BlobCrypterSecurityToken.encodeToken.  The check is
> sufficient because DefaultSecurityTokenCodec creates the correct
> SecurityTokenCode (Basic or Blob) depending on the container config values of
> "insecure" or "secure", respectively.  We should never get into this code if
> we're not using a secure configuration; therefore, an authentication mode of
> SECURITY_TOKEN_URL_PARAMETER implies that we have a BlobCrypterSecurityToken
> and not some other token, such as Anonymous.
> 
> 
> This addresses bug SHINDIG-1626.
>     https://issues.apache.org/jira/browse/SHINDIG-1626
> 
> 
> Diffs
> -----
> 
>   
> http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/ap
> ache/shindig/auth/BlobCrypterSecurityTokenCodec.java 1173205
>   
> http://svn.apache.org/repos/asf/shindig/trunk/java/gadgets/src/main/java/org/a
> pache/shindig/gadgets/servlet/GadgetsHandlerApi.java 1173205
> 
> Diff: https://reviews.apache.org/r/1981/diff
> 
> 
> Testing
> -------
> 
> Tested with a sample gadget that utilizes the osapi feature to print the
> viewer's name in a secure configuration.  The security token is encoded
> properly in the modified code.
> 
> Any other testing recommendations are welcome. :)
> 
> 
> Thanks,
> 
> Stanton
>