Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fetch access scope via #dig in callback controller #1210

Merged
merged 1 commit into from
Mar 12, 2021

Conversation

kirillplatonov
Copy link
Contributor

Problem

After upgrading to v17.1.0 I noticed that controller tests are not passing. I use OmniAuth mock in controller tests to login as shop:

class ActionDispatch::IntegrationTest
  def login(shop)
    OmniAuth.config.test_mode = true
    OmniAuth.config.add_mock(:shopify,
      provider: 'shopify',
      uid: shop.shopify_domain,
      credentials: { token: shop.shopify_token })
    Rails.application.env_config["omniauth.auth"] = OmniAuth.config.mock_auth[:shopify]

    get "/auth/shopify"
    follow_redirect!
  end
end

This code started raising an error:
CleanShot 2021-03-11 at 12 48 56@2x

What this PR does

Changes the syntax for fetching access_scope in CallbackController to be tolerant to empty auth_hash['extra'].
Hash#dig is supported since Ruby 2.3 so it shouldn't break anything (since the library requires ruby >= 2.5).

@kirillplatonov kirillplatonov requested a review from a team as a code owner March 11, 2021 09:52
@ghost ghost added the cla-needed label Mar 11, 2021
Copy link
Contributor

@NabeelAhsen NabeelAhsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this! Can confirm this results in an error currently for Omniauth mocks

(Also can confirm that this change fixes the resulting errors)

@rezaansyed
Copy link
Contributor

@kirillplatonov you'll need to sign the CLA in order to contribute to the gem.

@kirillplatonov
Copy link
Contributor Author

@rezaansyed Already signed CLA but the check not updated. Do I need to do anything else?
CleanShot 2021-03-12 at 00 19 01@2x

@NabeelAhsen
Copy link
Contributor

@rezaansyed Already signed CLA but the check not updated. Do I need to do anything else?
CleanShot 2021-03-12 at 00 19 01@2x

🤔 Hmm, try pushing up an empty commit to re-trigger the CLI?

@kirillplatonov
Copy link
Contributor Author

Done. CLA check is green now.

@rezaansyed
Copy link
Contributor

@kirillplatonov we pushed a fix so that the builds can run. Could you rebase your fork and push that up again?

@kirillplatonov
Copy link
Contributor Author

@rezaansyed Done. All green now.

@rezaansyed rezaansyed merged commit 651a8f6 into Shopify:master Mar 12, 2021
@rezaansyed rezaansyed mentioned this pull request Mar 12, 2021
4 tasks
@rezaansyed rezaansyed temporarily deployed to rubygems March 12, 2021 18:09 Inactive
@rezaansyed
Copy link
Contributor

@kirillplatonov thanks again for the fix! We've shipped this to the latest 17.1.1 version

@kirillplatonov kirillplatonov deleted the fix-omniauth-mock branch October 11, 2023 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants