Use the current Roller session during OAuth authorization - #165
Use the current Roller session during OAuth authorization#165snoopdave wants to merge 2 commits into
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. The session-binding, one-time approval, and validation changes are moving in the right direction. Please address the compatibility and error-handling notes before merge. Coordination with #154 remains necessary because both changes touch the same flow.
| 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.
| 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.
2 participants
The OAuth 1.0a consent step should authorize the user who is signed in, the way the rest of Roller resolves identity from the session. This change moves it onto that model and keeps request-token approval atomic.
What changed
oauth_problem=permission_deniedinstead of a container error.markAsAuthorizedmanager method for compatibility.PR #167 remains the merge prerequisite. This branch reuses its
SaltValidatorimplementation byte-for-byte, so that shared file drops from this PR after #167 lands.Tests
AuthorizationServletTest: 13 passing cases for session identity, refusal behavior, login continuation, missing state, and stale consent forms.JPAOAuthManagerTest: 2 passing persistence cases, including same-user retry and different-user refusal.