Spring Security OAuth
  1. Spring Security OAuth
  2. SECOAUTH-42

OAuth2ProtectedResourceFilter incorrectly thinks an OAuth 1.0a user authorization callback is an OAuth 2 protected resource access request

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Complete
    • Affects Version/s: 1.0.0.M2, 1.0.0.M3
    • Fix Version/s: 1.0.0.M4
    • Component/s: OAuth 2
    • Labels:
      None

      Description

      I have implemented an OAuth 1.0a consumer using the Scribe library and I have also implemented an OAuth 2 provider using Spring Security OAuth. The class OAuth2ProtectedResourceFilter in Spring Security OAuth 2 filters all requests to my web server. If I am connecting to Twitter (or any other OAuth 1.0a provider it seems), after the user authorizes my application and Twitter redirects said user back to my application, the filter OAuth2ProtectedResourceFilter incorrectly processes it as an OAuth 2 request to a protected resource since "oauth_signature_method" is not present when an OAuth 1.0a provider callsback after the user has authorized an application.

      The check takes place currently in parseHeaderToken(), but it seems that we cannot rely on just the presence of "oauth_signature_method" in the header in this case. Or am I missing something?

        Activity

        Hide
        Ryan Heaton added a comment -

        You're not missing anything. This logic was based on an older version of the oauth2 spec (see section 5.1.1 of draft 10 at http://tools.ietf.org/html/draft-ietf-oauth-v2-05#section-5.1).

        Newer versions have change the header name, so this just needs to be fixed. Thanks for bringing it to our attention.

        Show
        Ryan Heaton added a comment - You're not missing anything. This logic was based on an older version of the oauth2 spec (see section 5.1.1 of draft 10 at http://tools.ietf.org/html/draft-ietf-oauth-v2-05#section-5.1 ). Newer versions have change the header name, so this just needs to be fixed. Thanks for bringing it to our attention.
        Hide
        Ryan Heaton added a comment -

        resolved in master

        Show
        Ryan Heaton added a comment - resolved in master
        Hide
        Ryan Heaton added a comment -

        closed with the release of 1.0.0.M3

        Show
        Ryan Heaton added a comment - closed with the release of 1.0.0.M3
        Hide
        Vincent Tsao added a comment -

        This bug still exists in M3.

        In OAuth2ProtectedResourceFilter, the parseToken() method will still try to extract an oauth_token from an OAuth 1.0a authorization callback request if the token does not exist in the header. For example Twitter doesn't send the oauth_token for an OAuth 1.0a authorization callback in the header so the parseToken() method will think it's an OAuth 2.0 access token, which is incorrect.

        What I did to fix this temporarily was also check for the presence of an "oauth_verifier" parameter since this only exists in OAuth 1.0a and not OAuth 2.0 to distinguish the callback requests. I'm not sure if this is the best way of solving this problem.

        Show
        Vincent Tsao added a comment - This bug still exists in M3. In OAuth2ProtectedResourceFilter, the parseToken() method will still try to extract an oauth_token from an OAuth 1.0a authorization callback request if the token does not exist in the header. For example Twitter doesn't send the oauth_token for an OAuth 1.0a authorization callback in the header so the parseToken() method will think it's an OAuth 2.0 access token, which is incorrect. What I did to fix this temporarily was also check for the presence of an "oauth_verifier" parameter since this only exists in OAuth 1.0a and not OAuth 2.0 to distinguish the callback requests. I'm not sure if this is the best way of solving this problem.
        Hide
        Ryan Heaton added a comment -

        Fixed at a76efe2

        Show
        Ryan Heaton added a comment - Fixed at a76efe2
        Hide
        Vincent Tsao added a comment -

        Sorry to reopen this issue again, but this still isn't fixed.

        In the OAuth2ProtectedResourceFilter class in the parseHeaderToken() method, when I'm receiving a callback from a Google OAuth 1.0a process, Google does not return any headers called "Authorization". Thus it doesn't work to check if the authHeaderValue contains "oauth_verifier" in this method.

        I had to check this in the parseToken method in the if (token == null) condition. For example:

        if (token == null)

        { if (request.getParameter("oauth_verifier") != null) return null; token = request.getParameter("oauth_token"); }
        Show
        Vincent Tsao added a comment - Sorry to reopen this issue again, but this still isn't fixed. In the OAuth2ProtectedResourceFilter class in the parseHeaderToken() method, when I'm receiving a callback from a Google OAuth 1.0a process, Google does not return any headers called "Authorization". Thus it doesn't work to check if the authHeaderValue contains "oauth_verifier" in this method. I had to check this in the parseToken method in the if (token == null) condition. For example: if (token == null) { if (request.getParameter("oauth_verifier") != null) return null; token = request.getParameter("oauth_token"); }
        Hide
        Dave Syer added a comment -

        There was a spec change for OAuth2, so the OAuth2ProtectedResourceFilter no longer checks for "oauth_token" (which is an OAuth1 legacy name). Can you grab a snapshot or buidl from source and check if it works for you? If I'm reading your requirement correctly you need both the spring-security-oauth and spring-security-oauth2 jars, now that they are split.

        Show
        Dave Syer added a comment - There was a spec change for OAuth2, so the OAuth2ProtectedResourceFilter no longer checks for "oauth_token" (which is an OAuth1 legacy name). Can you grab a snapshot or buidl from source and check if it works for you? If I'm reading your requirement correctly you need both the spring-security-oauth and spring-security-oauth2 jars, now that they are split.
        Hide
        Vincent Tsao added a comment -

        Thanks Dave the latest build works for me.

        Show
        Vincent Tsao added a comment - Thanks Dave the latest build works for me.
        Hide
        Ryan Heaton added a comment -

        Closed with 1.0.0.M4.

        Show
        Ryan Heaton added a comment - Closed with 1.0.0.M4.

          People

          • Assignee:
            Dave Syer
            Reporter:
            Vincent Tsao
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: