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

Jump to bottom

Prefer HTTP standard header "From" over custom "X-On-Behalf-Of" for impersonation #3113

Closed
thorsteneckel opened this issue Jul 13, 2020 · 1 comment
Closed

Prefer HTTP standard header "From" over custom "X-On-Behalf-Of" for impersonation #3113

thorsteneckel opened this issue Jul 13, 2020 · 1 comment
Assignees
@rolfschmidt
Labels
API authentication 🚧 refactoring 🚧 deprecation good first issue hacktoberfest help appreciated specification required verified
Projects
OLD Workflow
Milestone
5.0

Comments

@thorsteneckel
Copy link
Contributor

Infos:

  • Used Zammad version: 3.4.x
  • Installation method (source, package, ..): any
  • Operating system: any
  • Database + version: any
  • Elasticsearch version: any
  • Browser + version: any

We introduced the X-On-Behalf-Of header back in 2018 with 6daaecb in order to fix #1805 . We were not aware that there is a HTTP standard header called From that is designated for the same purpose. @snuggs let us know via a comment below commit 847cf02.
Being conform to standards is one of the main goals of Zammad. However, in difference to the From head we support User lookup via id, login and email.

We should evaluate:

  • if we need to support all of these attributes
  • document why we need to support all of these attributes
  • if we can/should (soft) deprecate the usage of X-On-Behalf-Of in favour of the From header
thorsteneckel referenced this issue Jul 13, 2020
@thorsteneckel
Testing: Enhance authenticated_as spec helper
847cf02 
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.
@snuggs
Copy link

snuggs commented Jul 13, 2020

@thorsteneckel LOVE THIS! Was a shot in the dark. Didn't think you'd be interested. I just recently learned of it as well.
It doesn't have too many use cases but WOW is is great when .... you need an EMAIL...FROM another party the request is on behalf of. ;-)

@thorsteneckel thorsteneckel added this to the 5.0 milestone Jul 29, 2021
@thorsteneckel thorsteneckel added this to To do in OLD Workflow Jul 29, 2021
@rolfschmidt rolfschmidt self-assigned this Aug 10, 2021
@rolfschmidt rolfschmidt moved this from To do to In progress in OLD Workflow Aug 10, 2021
@thorsteneckel thorsteneckel moved this from In progress to Done in OLD Workflow Aug 10, 2021
@dvuckovic dvuckovic mentioned this issue Mar 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rolfschmidt rolfschmidt

Labels
API authentication 🚧 refactoring 🚧 deprecation good first issue hacktoberfest help appreciated specification required verified
Projects
No open projects
OLD Workflow
  
Done
Milestone
5.0
Development

No branches or pull requests
3 participants
@snuggs @thorsteneckel @rolfschmidt