Fix strategy configuration #1
Conversation
| def callback_url | ||
| full_host + script_name + callback_path | ||
| end |
There was a problem hiding this comment.
We got an error saying it was missing required parameters which is the same error we got with SurveyMonkey so we tried the same remedy and it worked
There was a problem hiding this comment.
is this overriding a method from super or something?
There was a problem hiding this comment.
Correct. I can see if I can find the original explanation of it for SurveyMonkey...
There was a problem hiding this comment.
Here's the same change we made for SurveyMonkey user-interviews/omniauth-surveymonkey-oauth2#1
Based on an identical change for the twitch gem
Here's the original omniauth change that broke things for a lot of gems omniauth/omniauth-oauth2#70
| # Override authorize_params so that we can be deliberate about the value for state | ||
| # and not use the session which is unavailable inside of an iframe for some | ||
| # browsers (ie Safari) | ||
| def authorize_params | ||
| # Only set state if it hasn't already been set | ||
| options.authorize_params[:state] ||= SecureRandom.hex(24) | ||
| params = options.authorize_params.merge(options_for("authorize")) | ||
| options.authorize_params[:scope] = 'manage:all' |
There was a problem hiding this comment.
This is not ideal--we'd rather it have just the scopes that we need, which you should be able to do by setting them in the omniauth config...but I tried that and it didn't work. We can try to look into a better solution in the future.
|
@michaeldewolf85 we looked at this together so I'm going to assume you're ok with the changes. Also since it's a dependency anyway it's inherently lower risk so I feel confident about merging it in at this point and we can make more changes if we need to |
This omniauth strategy, like the SurveyMonkey one before it, didn't quite work out of the box. This PR contains the tweaks we needed in order to get it to work in our testing locally