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.

Sign up for GitHub

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

Update omniauthable tests for OmniAuth 2.0 #5331

Merged
merged 1 commit into from
Jan 19, 2021
Merged

Update omniauthable tests for OmniAuth 2.0 #5331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ gem "responders", "~> 3.0"

group :test do
gem "omniauth-facebook"
gem "omniauth-openid"
gem "omniauth-openid", git: 'https://github.com/jkowens/omniauth-openid', branch: 'patch-1'
gem "timecop"
gem "webrat", "0.7.3", require: false
gem "mocha", "~> 1.1", require: false
Expand Down
35 changes: 24 additions & 11 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
GIT
remote: https://github.com/rails/activemodel-serializers-xml.git
remote: git://github.com/rails/activemodel-serializers-xml.git
revision: 694f4071c6b16e4c8597cc323c241b5f787b3ea8
specs:
activemodel-serializers-xml (1.0.2)
Expand All @@ -8,14 +8,23 @@ GIT
builder (~> 3.1)

GIT
remote: https://github.com/rails/rails-controller-testing.git
remote: git://github.com/rails/rails-controller-testing.git
revision: 4b15c86e82ee380f2a7cc009e470368f7520560a
specs:
rails-controller-testing (1.0.5)
actionpack (>= 5.0.1.rc1)
actionview (>= 5.0.1.rc1)
activesupport (>= 5.0.1.rc1)

GIT
remote: https://github.com/jkowens/omniauth-openid
revision: c70d35f266a814340b01f6f5649bb664a78743f4
branch: patch-1
specs:
omniauth-openid (2.0.0)
omniauth (>= 1.0, < 3.0)
rack-openid (~> 1.4.0)

PATH
remote: .
specs:
Expand Down Expand Up @@ -89,8 +98,11 @@ GEM
concurrent-ruby (1.1.7)
crass (1.0.6)
erubi (1.9.0)
faraday (1.0.1)
faraday (1.3.0)
faraday-net_http (~> 1.0)
multipart-post (>= 1.2, < 3)
ruby2_keywords
faraday-net_http (1.0.1)
globalid (0.4.2)
activesupport (>= 4.2.0)
hashie (4.1.0)
Expand Down Expand Up @@ -122,22 +134,22 @@ GEM
multi_json (~> 1.3)
multi_xml (~> 0.5)
rack (>= 1.2, < 3)
omniauth (1.9.1)
omniauth (2.0.1)
hashie (>= 3.4.6)
rack (>= 1.6.2, < 3)
rack-protection
omniauth-facebook (7.0.0)
omniauth-oauth2 (~> 1.2)
omniauth-oauth2 (1.7.0)
omniauth-oauth2 (1.7.1)
oauth2 (~> 1.4)
omniauth (~> 1.9)
omniauth-openid (1.0.1)
omniauth (~> 1.0)
rack-openid (~> 1.3.1)
omniauth (>= 1.9, < 3)
orm_adapter (0.5.0)
rack (2.2.3)
rack-openid (1.3.1)
rack-openid (1.4.2)
rack (>= 1.1.0)
ruby-openid (>= 2.1.8)
rack-protection (2.1.0)
rack
rack-test (1.1.0)
rack (>= 1.0, < 3)
rails (6.0.3.3)
Expand Down Expand Up @@ -172,6 +184,7 @@ GEM
actionpack (>= 5.0)
railties (>= 5.0)
ruby-openid (2.9.2)
ruby2_keywords (0.0.2)
sprockets (4.0.2)
concurrent-ruby (~> 1.0)
rack (> 1, < 3)
Expand Down Expand Up @@ -206,7 +219,7 @@ DEPENDENCIES
omniauth
omniauth-facebook
omniauth-oauth2
omniauth-openid
omniauth-openid!
rails (~> 6.0.0)
rails-controller-testing!
rdoc
Expand Down
2 changes: 1 addition & 1 deletion app/views/devise/shared/_links.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,6 @@

<%- if devise_mapping.omniauthable? %>
<%- resource_class.omniauth_providers.each do |provider| %>
<%= link_to "Sign in with #{OmniAuth::Utils.camelize(provider)}", omniauth_authorize_path(resource_name, provider) %><br />
<%= link_to "Sign in with #{OmniAuth::Utils.camelize(provider)}", omniauth_authorize_path(resource_name, provider), method: :post %><br />
Copy link
Member

Choose a reason for hiding this comment

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

@BobbyMcWho mind if I ask for some clarification here?

In the CVE wiki it mentions using link_to with method=post or a button_to should be enough, but in the release notes for OmniAuth v2 you mention specifically:

If you are using hyperlinks or buttons styled to redirect to your login route, you should update these to be a submit input or a submit type button wrapped in a form.

I'm assuming you meant those as being GET requests, that should be updated to using POST instead? (since Rails button_to or link_to + method=post use forms under the hood?)

Copy link

Choose a reason for hiding this comment

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

We want to make sure that whatever way we send the POST, we're including a CSRF token to be validated in the request_validation_phase. I suggested wrapping inputs with a rails form helper since by default those automatically generate a per-form token that can be validated by Rails' RequestForgeryProtection class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A link_to with method=post will submit a form with an authenticity token. I assume it is a per-form token, but not sure 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Right, thanks for the clarification 👍. I believe link_to + method=post or a button_to should be fine then, like @jkowens mentioned they should also include the authenticity token by default in Rails. (whether they are per-form or not is controlled by the application via the individual form and/or global Rails config iirc.)

<% end %>
<% end %>
44 changes: 26 additions & 18 deletions test/integration/omniauthable_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ class OmniauthableIntegrationTest < Devise::IntegrationTest
"extra" => {"user_hash" => FACEBOOK_INFO}
}
OmniAuth.config.add_camelization 'facebook', 'FaceBook'
if OmniAuth.config.respond_to?(:request_validation_phase)
OmniAuth.config.request_validation_phase = ->(env) {}
end
end

teardown do
Expand All @@ -45,17 +48,17 @@ def stub_action!(name)
test "omniauth sign in should not run model validations" do
stub_action!(:sign_in_facebook) do
create_user
visit "/users/sign_in"
click_link "Sign in with FaceBook"
post "/users/auth/facebook"
follow_redirect!
assert warden.authenticated?(:user)

refute User.validations_performed
end
end

test "can access omniauth.auth in the env hash" do
visit "/users/sign_in"
click_link "Sign in with FaceBook"
post "/users/auth/facebook"
follow_redirect!

json = ActiveSupport::JSON.decode(response.body)

Expand All @@ -68,8 +71,8 @@ def stub_action!(name)

test "cleans up session on sign up" do
assert_no_difference "User.count" do
visit "/users/sign_in"
click_link "Sign in with FaceBook"
post "/users/auth/facebook"
follow_redirect!
end

assert session["devise.facebook_data"]
Expand All @@ -89,8 +92,8 @@ def stub_action!(name)

test "cleans up session on cancel" do
assert_no_difference "User.count" do
visit "/users/sign_in"
click_link "Sign in with FaceBook"
post "/users/auth/facebook"
follow_redirect!
end

assert session["devise.facebook_data"]
Expand All @@ -100,8 +103,8 @@ def stub_action!(name)

test "cleans up session on sign in" do
assert_no_difference "User.count" do
visit "/users/sign_in"
click_link "Sign in with FaceBook"
post "/users/auth/facebook"
follow_redirect!
end

assert session["devise.facebook_data"]
Expand All @@ -110,23 +113,28 @@ def stub_action!(name)
end

test "sign in and send remember token if configured" do
visit "/users/sign_in"
click_link "Sign in with FaceBook"
post "/users/auth/facebook"
follow_redirect!
assert_nil warden.cookies["remember_user_token"]

stub_action!(:sign_in_facebook) do
create_user
visit "/users/sign_in"
click_link "Sign in with FaceBook"
post "/users/auth/facebook"
follow_redirect!
assert warden.authenticated?(:user)
assert warden.cookies["remember_user_token"]
end
end

test "generates a link to authenticate with provider" do
visit "/users/sign_in"
assert_select "a[href=?][data-method='post']", "/users/auth/facebook", text: "Sign in with FaceBook"
end

test "generates a proper link when SCRIPT_NAME is set" do
header 'SCRIPT_NAME', '/q'
visit "/users/sign_in"
assert_select "a", href: "/q/users/auth/facebook"
assert_select "a[href=?][data-method='post']", "/q/users/auth/facebook", text: "Sign in with FaceBook"
end

test "handles callback error parameter according to the specification" do
Expand All @@ -139,10 +147,10 @@ def stub_action!(name)
test "handles other exceptions from OmniAuth" do
OmniAuth.config.mock_auth[:facebook] = :invalid_credentials

visit "/users/sign_in"
click_link "Sign in with FaceBook"
post "/users/auth/facebook"
follow_redirect!
follow_redirect!

assert_current_url "/users/sign_in"
assert_contain 'Could not authenticate you from FaceBook because "Invalid credentials".'
end
end