[
https://issues.apache.org/jira/browse/SHINDIG-1636?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13132988#comment-13132988
]
[email protected] commented on SHINDIG-1636:
--------------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2362/#review2750
-----------------------------------------------------------
I've reviewed the code and provided feedback on a few items, but other than
those I think the code is ok. Although I will say that the interactions
between the ValueProvider and its collaborators feel a bit complicated to me
and I'm a little concerned that I'm not fully understanding whats supposed to
be happening where/when -- but as long as Dan and Stanton understand it that's
good enough for me.
However I still feel like this is really complicated and heavy-weight for doing
pretty much what ContainerConfig already does today. The only things I see
that are added over what ContainerConfig does now are providing the old value
during the change event and providing strong types -- but everything else
appears to be essentially just re-implementing a bunch of the existing
functionality. And we still have a full standalone interface (KeyProvider) and
implementation (KeyFileKeyProvider) for something that is only ever going to be
used in one place. And it doesn't cleanly replace ContainerConfig -- so now
classes that want to use this strategy for configuration need to take
ContainerConfig and an arbitrary configuration-type interface (in this case
KeyProvider but in other cases it would be different). I'm not saying I'd
actually want to abstract ContainerConfig away though -- I'm just pointing out
that now we have to provide two configuration-providing components to those who
want to use this new strategy which seems sub-optimal to me.
ContainerConfig is already in fairly wide usage throughout the codebase. If we
wanted to reuse this new strategy in all the places ContainerConfig is used
we'd end up with something like 30ish more KeyProvider and
KeyFileKeyProvider-like (but not exactly the same) interfaces and
implementations.
Is the strong typing and old value being passed on change a dealbreaker for you
guys? It seems configuration of a component is usually a handful of primitive
values, or at most a structured set of primitive values (JSON object?) so it
doesn't seem that strong typing would really be critical. And it seems the
configuration-consuming impls themselves would have the old values available to
them internally if they really needed them. IMO I still think doing something
like the prototype I had put together where we're passing configuration
contributors to the ContainerConfig is a simpler/cleaner/more generic way to
go. I know there were still some questions about being able to contribute
multiple configuration values but I think we could solve those pretty easily in
a number of different ways. Are there other reasons why you guys don't want to
go down that path?
Sorry for being the squeaky wheel on this set of changes. I was really hoping
that I'd open up these change sets and it would all just make sense to me and
be the right thing so I could just say "ship it!" and move on... It's really
really hard for me to continue saying "I don't get it" (cause I'm afraid I'm
missing something obvious and I don't want to hold you guys up), but,
unfortunately, I still don't get it.
If you guys feel strongly though that this is the right way to go please feel
free to go ahead and open it up to the community to get additional feedback on
your new strategy.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyFileKeyProvider.java
<https://reviews.apache.org/r/2362/#comment6191>
Arrays.asList is a varargs method -- no need to wrap the parameters in an
array
- Jesse
On 2011-10-21 16:23:00, Stanton Sievers wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/2362/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-10-21 16:23:00)
bq.
bq.
bq. Review request for Ryan Baxter, Dan Dumont and Jesse Ciancetta.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Currently,
org.apache.shindig.auth.BlobCrypterSecurityTokenCodec.loadContainers(ContainerConfig,
Collection<String>, Map<String, BlobCrypter>, Map<String, String>) reads an
encryption key from a keyfile to instantiate the BlobCrypter. The keyfile is
defined in the container.js. An improvement to this behavior would be to
provide an injectable KeyProvider class that can return the key. This would
allow the key to reside anywhere instead of in a static keyfile.
bq.
bq. Initial review to Dan, Ryan, and Jesse. Once we've decided that this
seems like a rational approach, I'll add the dev list.
bq.
bq.
bq. This addresses bug SHINDIG-1636.
bq. https://issues.apache.org/jira/browse/SHINDIG-1636
bq.
bq.
bq. Diffs
bq. -----
bq.
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BasicSecurityTokenCodec.java
1187375
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodec.java
1187375
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/DefaultSecurityTokenCodec.java
1187375
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyFileKeyProvider.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/KeyProvider.java
PRE-CREATION
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/BlobCrypterSecurityTokenCodecTest.java
1187375
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/DefaultSecurityTokenCodecTest.java
1187375
bq.
http://svn.apache.org/repos/asf/shindig/trunk/java/common/src/test/java/org/apache/shindig/auth/KeyFileKeyProviderTest.java
PRE-CREATION
bq.
bq. Diff: https://reviews.apache.org/r/2362/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Updated and ran existing JUnits.
bq. Created new JUnits for the new KeyFileKeyProvider.
bq. Performed manual functional tests with encrypted security tokens in the
sample common container.
bq.
bq.
bq. Thanks,
bq.
bq. Stanton
bq.
bq.
> Create a KeyProvider to provide an encryption key to the SecurityToken
> workflow
> -------------------------------------------------------------------------------
>
> Key: SHINDIG-1636
> URL: https://issues.apache.org/jira/browse/SHINDIG-1636
> Project: Shindig
> Issue Type: Improvement
> Components: Java
> Reporter: Stanton Sievers
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> Currently,
> org.apache.shindig.auth.BlobCrypterSecurityTokenCodec.loadContainers(ContainerConfig,
> Collection<String>, Map<String, BlobCrypter>, Map<String, String>) reads an
> encryption key from a keyfile to instantiate the BlobCrypter. The keyfile is
> defined in the container.js. An improvement to this behavior would be to
> provide an injectable KeyProvider class that can return the key. This would
> allow the key to reside anywhere instead of in a static keyfile.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
