[
https://issues.apache.org/jira/browse/SHINDIG-290?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12609396#action_12609396
]
Brian Eaton commented on SHINDIG-290:
-------------------------------------
I've reviewed the code. Comments:
BasicOAuthTokenStore and BasicOAuthConsumerStore
- this is very similarly named to something in the gadgets side of the tree,
can we pick a better name for either or both?
OAuthTokenIdentityMapper
- Passing in OAuthMessage rather than just the consumer key and access token
would be useful here. That way if someone extends the protocol to include
additional information they have it available.
- throwing ServletException strikes me as a little weird. Do you need to think
through the errors this interface should throw a little more?
OAuthServletFilterTest
- you aren't actually verifying that the HTTP responses returned from the
filter have the proper data in them. For example, what test would catch the
problem if OAuthServlet.handleException suddenly stopped returning HTTP 401 and
started returning "HTTP 503 Your mother is a hedgehog"
OAuthServletFilter
- excellent documentation, thank you.
- Comment on case (1): this seems really weird. Are we trusting the gadget
security token, or the signed fetch data? They may be in conflict, so which
one do we trust?
- Comment on case (2) has typo, "allows containers to this OAuth token" should
be (I think) "allows containers to link this OAuth token"
- OAuthValidator validator; is this thread safe?
- OAuth errors should return additional human readable text in the
oauth_problem_advice field.
- (warning: my pet style peeve, feel free to ignore) Reviewing this code in a
text editor is a little annoying because a few functions are implemented far
away from where they are used. For example handleRequest calls
getOAuthMessageFromRequest. I search around in the file to find the
implementation of getOAuthMessageFromRequest, and discover it seven or eight
method declarations later. If methods are declared in roughly the order they
are used, it makes reading the code more linear.
- handleException
I'm not sure OAuthServlet.handleException will really do what you want for
all time. The contract for that function is really vague. You may want to add
a test or two so that if OAuthServlet.handleException changes, you notice and
can correct your code.
- getOAuthMessageFromRequest seems problematic, because it relies on
req.getRequestURL. Is that going to be reliable? For example, what happens if
there is a front-end in front of the social data server that modifies the URL?
e.g. social request arrives on https://www.example.com/social, but is resent to
http://internal.example.com:7777/social. The OAuth signature checks will fail
unless you pass in the right URL. I'm really not certain of the best way to
handle this, maybe what you've got (a protected function so that someone can
override) is as good as it is going to get.
- is allowUnauthenticated really the right name for the configuration option?
Or should it really be "oauth.allow-non-oauth-requests". For example, someone
may implement another servlet filter that uses HTTP basic auth for access
control in the event that OAuth is not available.
- getSecurityToken
The error handling here looks wrong.
1) You probably want to log the SecurityTokenException. As is the log
message discards useful information, e.g. you have no idea whether the security
token was expired, malformed, or something else.
2) You probably want to log the request the client was attempting, so you
have some notion of what kind of request caused the problem.
3) You probably don't want to ignore the error, you should probably abort
request processing. If someone sent a security token and it doesn't work, they
need to know about it!
- getRequestorId
Should you return id, or id.trim()?
- handleConsumerRequest
About this comment: // we're using the viewer as the principal
I can see that from reading the code. A comment explaining why would be
useful.
- handleExplicitIdentity
allowConsumerRequest: this seems like a very strange parameter, or maybe it
is just strangely named. Would a better name be "allowAuthnType", perhaps? Or
maybe it would be better if this logic was moved up a place or two in the call
stack. For example, the allowSecurityToken check could be implemented in
getSecurityToken, and the allowRequestorId check in getRequestorId.
- It's unclear to me why you are requiring an OAuth consumer signature to use
the gadget security token. Why? A gadget can make the same API calls using
only the security token, right? I'm sure there is some reasoning behind the
check, I just can't figure out what it is.
- all: you are throwing away some important authentication information in this
code. For example, at the end of the day you have lost the OAuth consumer
information, you have lost the gadget security token, and you are reduced to to
just a user identity. That's likely to pose a problem for some use cases. For
example, the user has some privileges that a server impersonating the user does
not have. You can't enforce that access control if you throw away the
information about delegation.
- I think you need to define a tighter API contract for the getUserPrincipal
method. If your code creates a Principal, what will the class of that
Principal be, and what methods will be available? As a concrete example:
someone doing an access check will want to know whether an OAuth token was for
read/write vs read-only access. How are they supposed to get that information
out of your Principal object? I'd suggest implementing a subclass of Principal
that has some additional methods like "getGadgetSecurityToken" and
"getOAuthRequest". They should return the information that you used to do the
authentication. getPrincipal().getName() is the bare minimum information you
need to provide, but there is no rule saying you have to throw away everything
else.
> Add OAuth and Gadget Token access control systems to API server
> ---------------------------------------------------------------
>
> Key: SHINDIG-290
> URL: https://issues.apache.org/jira/browse/SHINDIG-290
> Project: Shindig
> Issue Type: New Feature
> Components: RESTful API (Java)
> Reporter: David Primmer
> Attachments: rest_api_server_auth_system.png,
> rest_api_server_auth_system.svg, restful-oauth1.txt, restful-oauth2.patch
>
>
> The server should be able to get auth info from both the gadget token and an
> oauth access token and after inspecting them, figure out the attributes
> necessary to pass on to the backend. There may be complicated rules for
> attribute precedence depending on the context of the request. A servlet
> filter is assumed to be the implementation and its also assumed that this
> would not be a throw-away system, as few of these actually exist, it might as
> well be a decent one. Current social soken handling can also be moved to a
> servlet filter for parity.
> In addition, there should be a simple Access Management system that can store
> access control lists and potentially delegations that the API server can
> refer to for data access decisions. This Policy Decision Point should be of
> limited scope and it's assumed it will be based on the standard Java security
> libraries. Policy enforcement will still happen in the social api data
> service layer.
> And Identity provider / login mechanism and GUI for delegating permissions
> (needed for the OAuth three-legged flow) is the most "out of scope" for
> shindig and it should be developed as a very simple and separate system to
> take credentials, take a delegation decision and store it in the Access
> Managment system.
> (I write this rather elaborate feature request knowing that I have a diagram
> illustrating this.) ;-)
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
