-
Notifications
You must be signed in to change notification settings - Fork 1
Implement mox #10
base: master
Are you sure you want to change the base?
Implement mox #10
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,36 +1,3 @@ | ||
defmodule SessionService do | ||
@moduledoc """ | ||
Describe a SessionService implementation. Use for handling auth derived | ||
from bearer tokens. | ||
""" | ||
|
||
@callback validate_api_token(String.t()) :: Tuple.t() | ||
@callback handle_bearer_token(Map.t(), Map.t(), String.t(), String.t()) :: Tuple.t() | ||
end | ||
|
||
defmodule Spacesuit.MockSessionService do | ||
@behaviour SessionService | ||
@moduledoc """ | ||
Mock session service used in testing the AuthHandler | ||
""" | ||
|
||
def validate_api_token(token) do | ||
case token do | ||
"ok" -> :ok | ||
"error" -> :error | ||
_ -> :error | ||
end | ||
end | ||
|
||
def handle_bearer_token(req, env, token, _url) do | ||
case token do | ||
"ok" -> {:ok, req, env} | ||
"error" -> {:stop, req} | ||
_ -> {:ok, req, env} | ||
end | ||
end | ||
end | ||
|
||
defmodule Spacesuit.SessionService do | ||
@moduledoc """ | ||
Implementation of a SessionService that calls out to an external service | ||
|
@@ -40,7 +7,8 @@ defmodule Spacesuit.SessionService do | |
require Logger | ||
use Elixometer | ||
|
||
@behaviour SessionService | ||
@callback validate_api_token(String.t()) :: Tuple.t() | ||
@callback handle_bearer_token(Map.t(), Map.t(), String.t(), String.t()) :: Tuple.t() | ||
|
||
@http_server Application.get_env(:spacesuit, :http_server) | ||
# How many milliseconds before we timeout call to session-service | ||
|
@@ -63,15 +31,13 @@ defmodule Spacesuit.SessionService do | |
{:ok, _, _} -> | ||
result | ||
|
||
{:error, type, code, error} -> | ||
{:error, type, code, error} when is_binary(error) -> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding the guard clause allows us to treat this as a separate case instead of nesting flow control. |
||
Logger.error("Session-service #{inspect(type)} error: #{inspect(error)}") | ||
@http_server.reply(code, %{}, error, req) | ||
{:stop, req} | ||
|
||
if is_binary(error) do | ||
@http_server.reply(code, %{}, error, req) | ||
else | ||
error_reply(req, 503, "Upstream error") | ||
end | ||
|
||
{:error, _type, _code, _error} -> | ||
error_reply(req, 503, "Upstream error") | ||
{:stop, req} | ||
|
||
{:error, type, error} -> | ||
|
@@ -139,7 +105,8 @@ defmodule Spacesuit.SessionService do | |
{:ok, %HTTPoison.Response{status_code: 200, body: body}} -> | ||
{:ok, body} | ||
|
||
{:ok, %HTTPoison.Response{status_code: code, body: body}} when code >= 400 and code <= 499 -> | ||
{:ok, %HTTPoison.Response{status_code: code, body: body}} | ||
when code >= 400 and code <= 499 -> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry. This is from the 1.6.4 formatter. I can remove it if you want, but I recommend updating and starting to use the formatter. |
||
{:error, :http, code, body} | ||
|
||
{:error, %HTTPoison.Error{reason: reason}} -> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,16 +24,8 @@ defmodule Spacesuit.Mixfile do | |
# Type "mix help compile.app" for more information | ||
def application do | ||
[ | ||
applications: [ | ||
:logger, | ||
:cowboy, | ||
:hackney, | ||
:crypto, | ||
:jose, | ||
:exometer_newrelic_reporter, | ||
:elixometer | ||
], | ||
mod: {Spacesuit, []} | ||
mod: {Spacesuit, []}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As of 1.4(?) you no longer want to explicitly pass applications that need to be started. While this kind of sucks since it takes away the explicitness, it means that you can't mess up by forgetting to add a new application to the list when you start using it. In my case, I needed to clean this up or mox wasn't getting started. |
||
extra_applications: [] | ||
] | ||
end | ||
|
||
|
@@ -62,6 +54,7 @@ defmodule Spacesuit.Mixfile do | |
# Test only | ||
{:excoveralls, "~> 0.6", only: :test}, | ||
{:mock, "~> 0.1.1", only: :test}, | ||
{:mox, "~> 0.3.2", only: :test}, | ||
{:apex, "~>1.1.0"} | ||
] | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,91 +1,167 @@ | ||
defmodule SpacesuitAuthMiddlewareTest do | ||
use ExUnit.Case | ||
import Mox | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. importing gives us |
||
|
||
doctest Spacesuit.AuthMiddleware | ||
|
||
setup_all do | ||
token = "eyJhbGciOiJIUzM4NCIsInR5cCI6IkpXVCJ9.eyJhY2N0IjoiMSIsImF6cCI6ImthcmwubWF0dGhpYXNAZ29uaXRyby5jb20iLCJkZWxlZ2F0ZSI6IiIsImV4cCI6IjIwMTctMDItMDNUMTU6MDc6MTRaIiwiZmVhdHVyZXMiOlsidGVhbWRvY3MiLCJjb21iaW5lIiwiZXNpZ24iXSwiaWF0IjoiMjAxNy0wMi0wM1QxNDowNzoxNC40MTMyMTg2OTNaIiwianRpIjoiNTU2ZmU1MTgtYTk0Mi00YTQ3LTkyZmMtNWNmNmVkOWY0YWFhIiwicGVybXMiOlsiYWNjb3VudHM6cmVhZCIsImdyb3VwczpyZWFkIiwidXNlcnM6d3JpdGUiXSwic3ViIjoiY3NzcGVyc29uQGdvbml0cm8uY29tIn0.6eWCzu6yHhgzuvUPaNloNl09uUfaN6nqhK1W--TQwtMk29tf5C5SV-hTT2pxnSxe" | ||
token = | ||
"eyJhbGciOiJIUzM4NCIsInR5cCI6IkpXVCJ9.eyJhY2N0IjoiMSIsImF6cCI6ImthcmwubWF0dGhpYXNAZ29uaXRyby5jb20iLCJkZWxlZ2F0ZSI6IiIsImV4cCI6IjIwMTctMDItMDNUMTU6MDc6MTRaIiwiZmVhdHVyZXMiOlsidGVhbWRvY3MiLCJjb21iaW5lIiwiZXNpZ24iXSwiaWF0IjoiMjAxNy0wMi0wM1QxNDowNzoxNC40MTMyMTg2OTNaIiwianRpIjoiNTU2ZmU1MTgtYTk0Mi00YTQ3LTkyZmMtNWNmNmVkOWY0YWFhIiwicGVybXMiOlsiYWNjb3VudHM6cmVhZCIsImdyb3VwczpyZWFkIiwidXNlcnM6d3JpdGUiXSwic3ViIjoiY3NzcGVyc29uQGdvbml0cm8uY29tIn0.6eWCzu6yHhgzuvUPaNloNl09uUfaN6nqhK1W--TQwtMk29tf5C5SV-hTT2pxnSxe" | ||
|
||
{:ok, token: token} | ||
end | ||
|
||
describe "handling non-bearer tokens" do | ||
test "passes through OK when there is no auth header" do | ||
assert {:ok, %{}, %{}} = Spacesuit.AuthMiddleware.execute(%{}, %{}) | ||
assert {:ok, %{}, %{}} = Spacesuit.AuthMiddleware.execute(%{}, %{}) | ||
end | ||
|
||
test "'authorization' header is stripped when present" do | ||
req = %{ headers: %{ "authorization" => "sometoken" }} | ||
req = %{headers: %{"authorization" => "sometoken"}} | ||
env = %{} | ||
|
||
assert {:ok, %{ headers: %{} }, ^env} = Spacesuit.AuthMiddleware.execute(req, env) | ||
assert {:ok, %{headers: %{}}, ^env} = Spacesuit.AuthMiddleware.execute(req, env) | ||
end | ||
end | ||
|
||
describe "handling bearer tokens" do | ||
test "with a valid token", state do | ||
req = %{ headers: %{ "authorization" => "Bearer #{state[:token]}" }, pid: self(), streamid: 1, method: "GET" } | ||
test "with a valid token", %{token: token} do | ||
req = %{ | ||
headers: %{"authorization" => "Bearer #{token}"}, | ||
pid: self(), | ||
streamid: 1, | ||
method: "GET" | ||
} | ||
|
||
env = %{} | ||
|
||
assert {:ok, %{ headers: _headers }, ^env} = Spacesuit.AuthMiddleware.execute(req, env) | ||
stub(MockSessionService, :handle_bearer_token, fn req, env, ^token, _url -> | ||
{:ok, req, env} | ||
end) | ||
|
||
assert {:ok, %{headers: _headers}, ^env} = Spacesuit.AuthMiddleware.execute(req, env) | ||
end | ||
|
||
test "with invalid bearer token and without session service" do | ||
Application.put_env(:spacesuit, :session_service, %{ enabled: false }) | ||
req = %{ headers: %{ "authorization" => "Bearer balloney" }, pid: self(), streamid: 1, method: "GET" } | ||
Application.put_env(:spacesuit, :session_service, %{enabled: false}) | ||
|
||
req = %{ | ||
headers: %{"authorization" => "Bearer balloney"}, | ||
pid: self(), | ||
streamid: 1, | ||
method: "GET" | ||
} | ||
|
||
env = %{} | ||
|
||
stub(MockSessionService, :handle_bearer_token, fn req, env, _token, _url -> | ||
{:ok, req, env} | ||
end) | ||
|
||
# Should just pass through unaffected | ||
assert {:ok, ^req, ^env} = Spacesuit.AuthMiddleware.execute(req, env) | ||
end | ||
|
||
test "with an invalid token when session service is enabled" do | ||
Application.put_env(:spacesuit, :session_service, %{ enabled: true, impl: Spacesuit.MockSessionService }) | ||
Application.put_env(:spacesuit, :session_service, %{enabled: true, impl: MockSessionService}) | ||
|
||
bad_token = "imabadtoken" | ||
|
||
req = %{ | ||
headers: %{"authorization" => "Bearer #{bad_token}"}, | ||
pid: self(), | ||
streamid: 1, | ||
method: "GET" | ||
} | ||
|
||
req = %{ headers: %{ "authorization" => "Bearer error" }, pid: self(), streamid: 1, method: "GET" } | ||
env = %{} | ||
|
||
stub(MockSessionService, :handle_bearer_token, fn req, _env, ^bad_token, _url -> | ||
{:stop, req} | ||
end) | ||
|
||
# Unrecognized, we pass it on as is | ||
assert {:stop, ^req} = Spacesuit.AuthMiddleware.execute(req, env) | ||
end | ||
|
||
test "with a valid token when session service is enabled" do | ||
Application.put_env(:spacesuit, :session_service, %{ enabled: true, impl: Spacesuit.MockSessionService }) | ||
test "with a valid token when session service is enabled", %{token: token} do | ||
Application.put_env(:spacesuit, :session_service, %{enabled: true, impl: MockSessionService}) | ||
|
||
stub(MockSessionService, :handle_bearer_token, fn req, env, ^token, _url -> | ||
{:ok, req, env} | ||
end) | ||
|
||
req = %{ | ||
headers: %{"authorization" => "Bearer #{token}"}, | ||
pid: self(), | ||
streamid: 1, | ||
method: "GET" | ||
} | ||
|
||
req = %{ headers: %{ "authorization" => "Bearer ok" }, pid: self(), streamid: 1, method: "GET" } | ||
env = %{} | ||
|
||
# Unrecognized, we pass it on as is | ||
assert {:ok, ^req, ^env} = Spacesuit.AuthMiddleware.execute(req, env) | ||
end | ||
|
||
test "with a missing token when session service is enabled" do | ||
Application.put_env(:spacesuit, :session_service, %{ enabled: true, impl: Spacesuit.MockSessionService }) | ||
Application.put_env(:spacesuit, :session_service, %{enabled: true, impl: MockSessionService}) | ||
|
||
empty_token = "" | ||
|
||
stub(MockSessionService, :handle_bearer_token, fn req, env, ^empty_token, _url -> | ||
{:ok, req, env} | ||
end) | ||
|
||
req = %{ headers: %{ "authorization" => "Bearer " }, pid: self(), streamid: 1, method: "GET" } | ||
req = %{headers: %{"authorization" => "Bearer "}, pid: self(), streamid: 1, method: "GET"} | ||
env = %{} | ||
|
||
# Unrecognized, we pass it on as is | ||
assert {:ok, ^req, ^env} = Spacesuit.AuthMiddleware.execute(req, env) | ||
end | ||
|
||
test "with a valid token on a bypassed path" do | ||
Application.put_env(:spacesuit, :session_service, %{ enabled: true, impl: Spacesuit.MockSessionService }) | ||
Application.put_env(:handler_opts, :middleware, %{ session_service: :disabled }) | ||
test "with a valid token on a bypassed path", %{token: token} do | ||
Application.put_env(:spacesuit, :session_service, %{enabled: true, impl: MockSessionService}) | ||
|
||
Application.put_env(:handler_opts, :middleware, %{session_service: :disabled}) | ||
|
||
req = %{ | ||
headers: %{"authorization" => "Bearer #{token}"}, | ||
pid: self(), | ||
streamid: 1, | ||
method: "GET" | ||
} | ||
|
||
req = %{ headers: %{ "authorization" => "Bearer ok" }, pid: self(), streamid: 1, method: "GET" } | ||
env = %{} | ||
|
||
stub(MockSessionService, :handle_bearer_token, fn req, env, ^token, _url -> | ||
{:ok, req, env} | ||
end) | ||
|
||
# pass it on as is | ||
assert {:ok, ^req, ^env} = Spacesuit.AuthMiddleware.execute(req, env) | ||
end | ||
|
||
test "with an invalid token on a bypassed path" do | ||
Application.put_env(:spacesuit, :session_service, %{ enabled: true, impl: Spacesuit.MockSessionService }) | ||
Application.put_env(:handler_opts, :middleware, %{ session_service: :disabled }) | ||
Application.put_env(:spacesuit, :session_service, %{enabled: true, impl: MockSessionService}) | ||
|
||
Application.put_env(:handler_opts, :middleware, %{session_service: :disabled}) | ||
|
||
bad_token = "iamabadtoken" | ||
|
||
req = %{ | ||
headers: %{"authorization" => "Bearer #{bad_token}"}, | ||
pid: self(), | ||
streamid: 1, | ||
method: "GET" | ||
} | ||
|
||
req = %{ headers: %{ "authorization" => "Bearer error" }, pid: self(), streamid: 1, method: "GET" } | ||
env = %{} | ||
|
||
stub(MockSessionService, :handle_bearer_token, fn req, _env, ^bad_token, _url -> | ||
{:stop, req} | ||
end) | ||
|
||
# Unrecognized, we pass it on as is | ||
assert {:stop, ^req} = Spacesuit.AuthMiddleware.execute(req, env) | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,3 @@ | ||
ExUnit.start() | ||
|
||
Mox.defmock(MockSessionService, for: Spacesuit.SessionService) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the only reason for a behaviour is for testing, it's pretty common (and acceptable) to move the callbacks/behaviour definition into the module that implements it in prod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super useful because I didn't know about Mox and so I'll definitely check it out. I'm a little lost about how it works ATM moment but I'm sure some reading will clear it up.
In this case we only had one implementation and a mock, but this module is not that generic and the idea was that people could provide their own implementation of a
SessionService
if they needed to handle it differently. Given that I'd like to support that, but that also nobody else seems to be interested, I'm on the fence about merging.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, not a problem. I will update the PR to leave the callback in the SessionService behaviour. That is technically a cleaner implementation and definitely the way to go if you want the
interfacebehaviour for more than just testing. Mox will work just fine that way, too.