Use the current Roller session during OAuth authorization - #165
Conversation
Derive the approving identity from the Roller session, as the rest of the UI does, and require the account to be enabled. Without a session the request goes to the login flow as before. A consumer key bound to a specific user may still only be approved by that user; a site-wide key is approved as whoever is logged in. Clients that continue to post the identity are accepted when the value agrees with the session and refused otherwise. Add OAuthManager.authorizeRequestToken(consumerKey, requestToken, userName), backed by a named update that matches the consumer key, the exact request token, an unauthorized record, and no access token, and reports whether one row changed. Approval is therefore one-shot, with no read-then-write window. markAsAuthorized is deprecated: it keyed on the consumer alone and did not name the token being approved. Refusals share one response so callers cannot tell refusals apart. Drop the identity field from the consent form and give it the standard salt field, and validate that token on the consent URL only. The request-token and access-token endpoints carry an OAuth signature and are left out of that mapping. Tests: AuthorizationServletTest. Claude-Session: https://claude.ai/code/session_01A1fhY1E2PCFU6UAPXu2WtV
There was a problem hiding this comment.
Reviewed with the multi-agent find-and-verify pass. Binding approval to the session and making it one-shot are both right, and the salt gate is a good addition. The reason I'm holding approval is the attack this PR is named for: the classic OAuth 1.0 session fixation still works end to end, because approval only flips a boolean and the access-token exchange accepts any authorized token no matter who started the flow. The fix that closes it is the standard one from OAuth 1.0a: mint an oauth_verifier on approval, hand it to the consumer via the callback, and require it at the access-token exchange. Details inline, along with the open-redirect callback (which leaks the freshly authorized token), the login forward that actually lands on an access-denied page, and a few regressions from the new conditional update and salt filter.
Coordination note: #154 removes OAuth 1.0a entirely (net.oauth is javax-only and unmaintained), so this servlet disappears with the Jakarta migration. If the plan is a security release from master before that lands, the verifier is the piece that makes this PR close the hole; if the migration lands first, this may not need to merge at all. Happy to go either way, but worth deciding explicitly.
| accessor.setProperty("userId", userId); | ||
| accessor.setProperty("authorized", Boolean.TRUE); | ||
|
|
||
| returnToConsumer(request, response, accessor); |
There was a problem hiding this comment.
This is where the fixation attack survives. Attacker starts the consumer flow, gets request token T, and sends a logged-in victim /roller-services/oauth/authorize?oauth_token=T. The victim clicks Authorize, this conditional update binds T to the victim, and the attacker finishes the exchange at the consumer: AccessTokenServlet checks only authorized == TRUE, so it hands out an access token acting as the victim. The session check and one-shot approval don't help because the attacker never posts here. OAuth 1.0a fixed exactly this with oauth_verifier: generate one on approval, store it on the accessor, return it to the consumer through the callback, and require it on the access-token exchange.
|
|
||
| private void sendToAuthorizePage(HttpServletRequest request, | ||
| HttpServletResponse response, OAuthAccessor accessor) | ||
| throws IOException, ServletException{ |
There was a problem hiding this comment.
returnToConsumer (line 190) redirects to the oauth_callback request parameter, which is attacker-controllable and seeded into the consent form's hidden field from the original GET link. A victim sent ...authorize?oauth_token=T&oauth_callback=https://evil.example/ gets 302'd there with the freshly authorized token on the query string, and for an already-authorized T the GET path redirects with no click at all. Pre-existing, but this PR reworks the success path, so it's the moment to validate the callback against the consumer's registered one.
| // approve on behalf of, so send the caller through the login flow. | ||
| User user = getAuthenticatedUser(request); | ||
| if (user == null) { | ||
| sendToAuthorizePage(request, response, accessor); |
There was a problem hiding this comment.
The comment says a session-less request goes through the login flow, but sendToAuthorizePage forwards to /roller-ui/oauthAuthorize.rol, which isn't a Spring-protected URL: UISecurityInterceptor returns DENIED and the user lands on the access-denied tile with no way to log in or resume. A user whose login expired mid-consent, or a first-time consent with no session, dead-ends there and the consumer's request token is stranded. A redirect to the login page with a saved request would do what the comment describes.
| q.setParameter(2, new Timestamp(new Date().getTime())); | ||
| q.setParameter(3, consumerKey); | ||
| q.setParameter(4, requestToken); | ||
| return q.executeUpdate() == 1; |
There was a problem hiding this comment.
markAsAuthorized was idempotent; this update requires authorized to be null or false, so approving a token that's already authorized but not yet exchanged affects zero rows and the servlet answers a bare 403 permission_denied instead of returning the user to the consumer. That happens on a retried or double-submitted approval. Treating already-authorized-by-the-same-user as success keeps the one-shot guarantee without breaking retries.
|
|
||
| @Override | ||
| public void doGet(HttpServletRequest request, HttpServletResponse response) | ||
| throws IOException, ServletException { |
There was a problem hiding this comment.
doGet still dereferences accessor without the null guard doPost gained (line 66): GET ...authorize?oauth_consumer_key= before any request token exists NPEs on accessor.getProperty and surfaces as a container 500, where doPost now returns the uniform permission_denied.
| signature and must not be included. --> | ||
| <filter-mapping> | ||
| <filter-name>ValidateSaltFilter</filter-name> | ||
| <url-pattern>/roller-services/oauth/authorize</url-pattern> |
There was a problem hiding this comment.
Good to gate the consent POST, but ValidateSaltFilter throws ServletException("Security Violation") when the salt is missing, expired (SaltCache entries live 60 minutes, and cache.salt.size=5000 evicts), already consumed, or issued to a different user. A user who leaves the consent page open for an hour and then clicks Authorize gets the container's 500 page rather than the consent form again or an OAuth problem response. Same on a cluster without a shared SaltCache. Catching that case in the servlet (re-render with a fresh salt) would keep the protection without the cliff.
| returnToConsumer(request, response, accessor); | ||
|
|
||
|
|
||
| } catch (OAuthProblemException e) { |
There was a problem hiding this comment.
Previously OAuthProblemException from getAccessor (token_expired, token_rejected) went through OAuthServlet.handleException, which sends the problem-specific status and a WWW-Authenticate: OAuth realm header. This catch collapses everything to a bare 403 with no realm header, while doGet on the same endpoint still reports the old way, so the same token state is described two different ways depending on method.
| * Callers should not distinguish these cases to the client. | ||
| * @throws OAuthException on persistence failure | ||
| */ | ||
| boolean authorizeRequestToken(String consumerKey, String requestToken, String userName) |
There was a problem hiding this comment.
Minor: adding an abstract method to this interface while keeping markAsAuthorized deprecated 'for callers outside the project' is a bit contradictory; if external implementations are a concern, a default method covers them, and if they aren't, markAsAuthorized can just go.
|
|
||
| servlet.doPost(request, response); | ||
|
|
||
| verifyNothingAuthorizedFor("admin"); |
There was a problem hiding this comment.
The mismatched-identity tests assert that nothing was authorized for admin, but not the 403, and not that alice wasn't authorized either. As written they'd pass if the mismatch were silently accepted for alice. Asserting the response status and the absence of any authorization pins the behavior the description promises.
2 participants
The OAuth 1.0a consent step should authorize the user who is signed in, the way
the rest of Roller's admin and editor UI resolves identity from the session.
This change moves it onto that model and tightens request-token approval into a
single conditional update.
What changed
user. A request with no session goes through the normal login flow.
userId/xoauth_requestor_idrequestparameter is still accepted when it agrees with the session user, and
rejected otherwise; it is not used to choose the identity.
exactly one row changed — rather than a separate load then store, so approval
is one-shot.
oauth_problem=permission_denied(403) for every refusal, sothe response does not vary with the reason.
markAsAuthorizedin favour ofauthorizeRequestToken.Tests
AuthorizationServletTestcovers identity taken from the session, a mismatcheduserIdparameter, disabled accounts, the bound-consumer refusal, and thegeneric refusal path.
JPAOAuthManagerTestexercises the conditional updateagainst Derby: a mismatched token changes no row, the exact pending token
succeeds once, and a second use of the same token changes no row.