On Fri, Sep 5, 2008 at 2:49 PM, Brian Eaton <[EMAIL PROTECTED]> wrote:
> [+shindig-dev, with John's permission] > > Please don't have an anonymous security token return real values for > any of the fields that identify a user or a gadget. Having watched > this play out, I have a strong preference for removing the anonymous > token entirely. > > As I recall, the original rationale for the token was that it was > easier to check "securityToken.isAnonymous()" than "securityToken != > null". > > Then someone (I haven't checked who) submitted code to Shindig that > crashed all unauthenticated gadget renders because they did not change > existing code to check securityToken.isAnonymous() instead of > securityToken != null. > > In order to fix that, we're now talking about making an anonymous > security token that acts and behaves almost exactly like a real > security token. If someone forgets to check "isAnonymous", they are > creating a security problem. For example, as far as the signed fetch > and OAuth code are concerned "" is a completely legitimate user id. Yes, and that legitimate id will map to the same user at all times, resulting in the same effect as if no auth was used at all. So what are the implications? Signed fetch looks like this: ...?opensocial_owner_id=&opensocial_viewer_id=&opensocial_app_id=&opensocial_app_url=&.... (i.e. a useless request) Three legged oauth simply fails (no stored tokens for the anonymous user). API contracts that require the user to call a method are very error prone. What we're stating here is that every single piece of code that ever touches SecurityTokens (which is roughly 2/3rds of the code base) must call isAnonymous() before extracting any piece of data. That isn't happening now (not by a long shot), and it's not likely going to happen either. > > > This entire direction seems like a mistake, and a poorly tested one at > that. "AnymousSecurityToken" is a backwards idea. We should go back > to using null. > > Cheers, > Brian > > On Fri, Sep 5, 2008 at 2:32 PM, John Hjelmstad <[EMAIL PROTECTED]> wrote: > > FYI -- I was interested in both your opinion on whether this change is > the > > right handling of this situation, or if perhaps null should instead be > > returned for Strings. I wanted to err on the side of usage safety to > avoid > > NPEs but don't want to bump into weird side effects if nulls are actually > > expected. > > Thx, > > John > > > > ---------- Forwarded message ---------- > > From: <[EMAIL PROTECTED]> > > Date: Fri, Sep 5, 2008 at 2:28 PM > > Subject: svn commit: r692557 - > > > /incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AnonymousSecurityToken.java > > To: [EMAIL PROTECTED] > > > > > > Author: johnh > > Date: Fri Sep 5 14:28:17 2008 > > New Revision: 692557 > > > > URL: http://svn.apache.org/viewvc?rev=692557&view=rev > > Log: > > Returning safe default values from AnonymousSecurityToken rather than > > throwing UnsupportedOperationException. This prevents Shindig run in Full > > mode (which has UrlAuthHandler and AnonymousAuthHandler configured) not > to > > barf when direct-rendering a gadget (/gadgets/ifr?url=foo.xml) without a > > security token. > > > > > > Modified: > > > > > > incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AnonymousSecurityToken.java > > > > Modified: > > > incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AnonymousSecurityToken.java > > URL: > > > http://svn.apache.org/viewvc/incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AnonymousSecurityToken.java?rev=692557&r1=692556&r2=692557&view=diff > > > ============================================================================== > > --- > > > incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AnonymousSecurityToken.java > > (original) > > +++ > > > incubator/shindig/trunk/java/common/src/main/java/org/apache/shindig/auth/AnonymousSecurityToken.java > > Fri Sep 5 14:28:17 2008 > > @@ -32,38 +32,38 @@ > > } > > > > public String toSerialForm() { > > - throw new UnsupportedOperationException(); > > + return ""; > > } > > > > public String getOwnerId() { > > - throw new UnsupportedOperationException(); > > + return ""; > > } > > > > public String getViewerId() { > > - throw new UnsupportedOperationException(); > > + return ""; > > } > > > > public String getAppId() { > > - throw new UnsupportedOperationException(); > > + return ""; > > } > > > > public String getDomain() { > > - throw new UnsupportedOperationException(); > > + return ""; > > } > > > > public String getAppUrl() { > > - throw new UnsupportedOperationException(); > > + return ""; > > } > > > > public long getModuleId() { > > - throw new UnsupportedOperationException(); > > + return 0L; > > } > > > > public String getUpdatedToken() { > > - throw new UnsupportedOperationException(); > > + return ""; > > } > > > > public String getTrustedJson() { > > - throw new UnsupportedOperationException(); > > + return ""; > > } > > } > > \ No newline at end of file > > > > > > > > >
