Skip to content
This repository has been archived by the owner on Jul 24, 2023. It is now read-only.

Breaking google change, results in "Server https://www.google.com/accounts/o8/ud responds that the 'check_authentication' call is not valid" #51

Closed
cortfr opened this issue Apr 18, 2013 · 18 comments

Comments

@cortfr
Copy link

cortfr commented Apr 18, 2013

I believe the fix is to no longer include 'assoc_handle' when creating requests with mode checkid_immediate or checkid_setup. See https://groups.google.com/forum/?fromgroups=#!topic/google-federated-login-api/qXZDD7_K7jU .

@kendagriff
Copy link
Contributor

I can confirm the same error. Traced it to the same Google Group message...

@kendagriff
Copy link
Contributor

Okay, I'm certain this patch demonstrates a gross misunderstanding of the OpenID specifications, but it's a fix (for me, at least):

kendagriff@ef88f27

(Additional update: kendagriff@79beaa4)

@kendagriff
Copy link
Contributor

Let me know if this is worthy of a pull request (somehow I doubt it).

@ConradIrwin
Copy link

Thanks for this @kendagriff! Your patch is working for us at https://rapportive.com/ :D. I've emailed our contact at Google to see if they know what's causing this, will let you know if I get a shareable response.

@sathappan
Copy link

Thanks for the patch @kendagriff..

@tispratik
Copy link

Hey @kendagriff. Thanks for the patch!

@dvandersluis
Copy link

We ran into this change as well, and after a lot of trying to figure out what was going on (especially with the unfortunately small amount of detail on the google group), it seems that an association is determined during "step 1" of going through OpenID (ie. building the request to send to google), and if one is found, its handle is included in the URL. Having this association handle in the URL is what causes Google to return invalid.

I'm not sure if @kendagriff's patch will cause problems for any other OpenID providers, and possibly will make the OpenID store completely useless? I'm not exactly sure how that part works.

Anyways, before I saw this issue, this was the monkey patch I wrote, which is limited to OpenID requests for google:

module OpenID
  class Consumer
    class AssociationManager
      def get_association_with_skip_for_google
        # Google no longer wants the assoc_handle parameter when authenticating by OpenID, and passing it in causes
        # it to always respond that the request is invalid. To get around this, if we're using google, just don't
        # collect/create an association, so no handle will be used in the request.
        # See: https://groups.google.com/forum/#!topic/google-federated-login-api/qXZDD7_K7jU
        return nil if @server_url == "https://www.google.com/accounts/o8/ud"

        get_association_without_skip_for_google
      end

      alias_method_chain :get_association, :skip_for_google
    end
  end
end

I'm not really sure how to determine in a universal way if the assoc_handle is desired or not.

@metavida
Copy link

@kendagriff's patch is working for us, in production. We currently use OpenID exclusively for Google sign on so I'm not sure whether the patch affects other OpenID providers, as @dvandersluis suggested. Hopefully enough folks can try this out over the next few days to get a solid patch integrated into this gem ASAP.

@cortfr
Copy link
Author

cortfr commented Apr 19, 2013

Thanks @dvandersluis and @kendagriff! I'm using @dvandersluis's monkey patch in production for now, at least until there is clarity about how other openid providers expect this to be handled. Thanks again!

@challengee
Copy link

Thanks @kendagriff, Working for us at http://www.piesync.com. Cost me quite some time to figure out this was the issue...

Thanks again! See what google comes up with now ...

@tmaher
Copy link

tmaher commented Apr 19, 2013

@dvandersluis as best I can tell, yesterday's Google change is limited to OpenID 2 requests in stateless mode - that is, requests that by definition shouldn't be using a store. For stateful mode, the whole point is to avoid having the RP (you and me) make an extra server-side request to the OP (Google) to validate the assertion. That's probably the proper universal way.

We (Heroku) are using @kendagriff's patch in a couple places internally, though not in prod. We've so far limited our deployment to just apps that were set up in stateless mode initially. Our ruby-openid using apps that are doing stateful OpenID (as a dependency from OmniAuth or Warden) weren't impacted by Google's change.

@kendagriff
Copy link
Contributor

Can the contributors chime in? IMO, Google is a sufficiently important OpenID provider – and given that they appear to implement the specification correctly – that ruby-openid should support OpenID2 requests out of the box – i.e. without a monkey patch.

Or, perhaps ruby-openid could support "compatibility" mode for providers who require the association?

@veesahni
Copy link

@dennisreimann could you comment on this one?

@cortfr
Copy link
Author

cortfr commented May 29, 2013

Update on using the patch posted by @dvandersluis:

I realized today that I needed to add a check for https://www.google.com/accounts/o8/site-xrds to fix our Google Apps OpenID Login.

So:

module OpenID
  class Consumer
    class AssociationManager
      def get_association_with_skip_for_google
        # Google no longer wants the assoc_handle parameter when authenticating by OpenID, and passing it in causes
        # it to always respond that the request is invalid. To get around this, if we're using google, just don't
        # collect/create an association, so no handle will be used in the request.
        # See: https://groups.google.com/forum/#!topic/google-federated-login-api/qXZDD7_K7jU
        return nil if @server_url == "https://www.google.com/accounts/o8/ud"

        return nil if @server_url and @server_url.starts_with?("https://www.google.com/accounts/o8/site-xrds")

        get_association_without_skip_for_google
      end

      alias_method_chain :get_association, :skip_for_google
    end
  end
end

@kcm
Copy link

kcm commented Aug 13, 2013

This bit us too. Can this get reviewed/merged ASAP? Though, this monkeypatch from cortfr worked for us instead of pegging the ruby-openid version.

@thegcat
Copy link

thegcat commented Aug 19, 2013

I can confirm @cortfr's monkey patch also works for us, thanks!

@reynardmh
Copy link

So this patch has not been merged to the latest version of the gem?

@dennisreimann
Copy link
Contributor

I finally found the time to fix the tests and merge this and other pull requests.

Please try v2.3.0 which was just released and see if it works in production :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests