Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Testing: Enhance authenticated_as spec helper
Originally, the `authenticated_as` spec helper
made it impossible to persist session state in request specs.
This made it impossible to test external_credentials#callback
or any other controller actions that rely on session state.

This commit fixes that.

=== How exactly was `authenticated_as` broken?

`authenticated_as` passes user credentials via request headers.
In practice, this auth method is for API requests by machine clients,
since they don't have cookies or store session state.

Because machine clients don't use cookies or session state anyway,
Zammad disables them for this auth method:

    # app/controllers/application_controller/authenticates.rb
     1 module ApplicationController::Authenticates
    42 def authentication_check_only(auth_param = {})
    57     request.session_options[:skip] = true

As a result, in tests using `authenticated_as`,
session data is discarded between every single request.
Some controller actions will not work without session data,
so this limitation made testing these actions impossible.

=== Why couldn't we just set the session values directly in test setup?

This is possible in controller tests,
but the Rails & RSpec team soft-deprecated controller tests
in favor of integration tests (request specs) a long time ago.

For more details, consider the following comments:

> The session is an internal structure for the controller.
> Integration test is more black box than controller tests were.
> I don't think we should be testing internals of the controller like this.
> I would rework the test to test something visible in the UI instead.
>
> --@dhh via rails/rails#23386

> Integration tests doesn't allow you to set session.
>
> --@rafaelfranca via rails/rails#25394

=== Solution

This commit adds a `via: :browser` option to the spec helper
which simply prepends a valid sign-in request to the spec example.
  • Loading branch information
Ryan Lue authored and thorsteneckel committed Apr 28, 2020
1 parent 494474f commit 847cf02
Showing 1 changed file with 16 additions and 11 deletions.
27 changes: 16 additions & 11 deletions spec/support/request.rb
Expand Up @@ -64,25 +64,30 @@ def json_response
# authenticated_as(nil, login: 'not_existing', password: 'wrongpw' )
#
# @return nil
def authenticated_as(user, login: nil, password: nil, token: nil, on_behalf_of: nil)
password ||= user.password
login ||= user.login
def authenticated_as(user, via: :api_client, **options)
password = options[:password] || user.password.to_s
login = options[:login] || user.login

# mock authentication otherwise login won't
# if user has no password (which is expensive to create)
if password.nil?
if password.blank?
allow(User).to receive(:authenticate).with(login, '') { user.update_last_login }.and_return(user)
end

# if we want to authenticate by token
if token.present?
credentials = "Token token=#{token.name}"
case via
when :api_client
# if we want to authenticate by token
if options[:token].present?
credentials = "Token token=#{options[:token].name}"

return add_headers('Authorization' => credentials)
end
return add_headers('Authorization' => credentials)
end

credentials = ActionController::HttpAuthentication::Basic.encode_credentials(login, password)
add_headers('Authorization' => credentials, 'X-On-Behalf-Of' => on_behalf_of)
credentials = ActionController::HttpAuthentication::Basic.encode_credentials(login, password)
add_headers('Authorization' => credentials, 'X-On-Behalf-Of' => options[:on_behalf_of])
when :browser
post '/api/v1/signin', params: { username: login, password: password, fingerprint: Faker::Number.number(9) }
end
end

# Provides a Hash of attributes for the given FactoryBot
Expand Down

2 comments on commit 847cf02

@snuggs
Copy link

@snuggs snuggs commented on 847cf02 Jul 10, 2020

Choose a reason for hiding this comment

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

@thorsteneckel
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @snuggs - thanks for the heads up. I created an issue to discuss your proposal: #3113

Please sign in to comment.