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.
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.
| 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.