diff --git a/config/dev.secrets.exs.example b/config/dev.secrets.exs.example index 94801fc3c..2c59d3780 100644 --- a/config/dev.secrets.exs.example +++ b/config/dev.secrets.exs.example @@ -26,6 +26,17 @@ config :cadet, # # You may need to write your own claim extractor for other providers # claim_extractor: Cadet.Auth.Providers.CognitoClaimExtractor # }}, + # # To use authentication with GitHub + # "github" => + # {Cadet.Auth.Providers.GitHub, + # %{ + # # A map of GitHub client_id => client_secret + # clients: %{ + # "client_id" => "client_secret" + # }, + # token_url: "https://github.com/login/oauth/access_token", + # user_api: "https://api.github.com/user" + # }}, "test" => {Cadet.Auth.Providers.Config, [ diff --git a/lib/cadet/auth/provider.ex b/lib/cadet/auth/provider.ex index 3901c3c4c..3f916e5b5 100644 --- a/lib/cadet/auth/provider.ex +++ b/lib/cadet/auth/provider.ex @@ -13,6 +13,8 @@ defmodule Cadet.Auth.Provider do @type redirect_uri :: String.t() @type error :: :upstream | :invalid_credentials | :other @type provider_instance :: String.t() + @type username :: String.t() + @type prefix :: String.t() @doc "Exchanges the OAuth2 authorisation code for a token and the user ID." @callback authorise(any(), code, client_id, redirect_uri) :: @@ -53,4 +55,9 @@ defmodule Cadet.Auth.Provider do _ -> {:error, :other, "Invalid or nonexistent provider config"} end end + + @spec namespace(username, prefix) :: String.t() + def namespace(username, prefix) do + prefix <> "/" <> username + end end diff --git a/lib/cadet/auth/providers/config.ex b/lib/cadet/auth/providers/config.ex index 1d64d1140..d322f8dc0 100644 --- a/lib/cadet/auth/providers/config.ex +++ b/lib/cadet/auth/providers/config.ex @@ -20,8 +20,11 @@ defmodule Cadet.Auth.Providers.Config do | {:error, Provider.error(), String.t()} def authorise(config, code, _client_id, _redirect_uri) do case Enum.find(config, nil, fn %{code: this_code} -> code == this_code end) do - %{token: token, username: username} -> {:ok, %{token: token, username: username}} - _ -> {:error, :invalid_credentials, "Invalid code"} + %{token: token, username: username} -> + {:ok, %{token: token, username: Provider.namespace(username, "test")}} + + _ -> + {:error, :invalid_credentials, "Invalid code"} end end diff --git a/lib/cadet/auth/providers/github.ex b/lib/cadet/auth/providers/github.ex new file mode 100644 index 000000000..1a1fbb341 --- /dev/null +++ b/lib/cadet/auth/providers/github.ex @@ -0,0 +1,87 @@ +defmodule Cadet.Auth.Providers.GitHub do + @moduledoc """ + Provides identity using GitHub OAuth. + """ + alias Cadet.Auth.Provider + + @behaviour Provider + + @type config :: %{ + clients: %{}, + token_url: String.t(), + user_api: String.t() + } + + @spec authorise(config(), Provider.code(), Provider.client_id(), Provider.redirect_uri()) :: + {:ok, %{token: Provider.token(), username: String.t()}} + | {:error, Provider.error(), String.t()} + def authorise(config, code, client_id, redirect_uri) do + token_headers = [ + {"Content-Type", "application/x-www-form-urlencoded"}, + {"Accept", "application/json"} + ] + + token_url = config.token_url + user_api = config.user_api + + with {:validate_client, {:ok, client_secret}} <- + {:validate_client, Map.fetch(config.clients, client_id)}, + {:token_query, token_query} <- + {:token_query, + URI.encode_query(%{ + client_id: client_id, + client_secret: client_secret, + code: code, + redirect_uri: redirect_uri + })}, + {:token, {:ok, %{body: body, status_code: 200}}} <- + {:token, HTTPoison.post(token_url, token_query, token_headers)}, + {:token_response, %{"access_token" => token}} <- {:token_response, Jason.decode!(body)}, + {:user, {:ok, %{"login" => username}}} <- {:user, api_call(user_api, token)} do + {:ok, %{token: token, username: Provider.namespace(username, "github")}} + else + {:validate_client, :error} -> + {:error, :invalid_credentials, "Invalid client id"} + + {:token, {:ok, %{status_code: status}}} -> + {:error, :upstream, "Status code #{status} from GitHub"} + + {:token_response, %{"error" => error}} -> + {:error, :invalid_credentials, "Error from GitHub: #{error}"} + + {:user, {:error, _, _} = error} -> + error + end + end + + @spec get_name(config(), Provider.token()) :: + {:ok, String.t()} | {:error, Provider.error(), String.t()} + def get_name(config, token) do + user_api = config.user_api + + case api_call(user_api, token) do + {:ok, %{"name" => name}} -> + {:ok, name} + + {:error, _, _} = error -> + error + end + end + + def get_role(_config, _claims) do + # There is no role specified for the GitHub provider + {:error, :invalid_credentials, "No role specified in token"} + end + + defp api_call(url, token) do + headers = [{"Authorization", "token " <> token}] + + case HTTPoison.get(url, headers) do + {:ok, %{body: body, status_code: 200}} -> + {:ok, Jason.decode!(body)} + + {:ok, %{status_code: status}} -> + {:error, :upstream, "Status code #{status} from GitHub"} + end + end +end diff --git a/lib/cadet/auth/providers/google_claim_extractor.ex b/lib/cadet/auth/providers/google_claim_extractor.ex index 25358b4e6..ab0d41a7a 100644 --- a/lib/cadet/auth/providers/google_claim_extractor.ex +++ b/lib/cadet/auth/providers/google_claim_extractor.ex @@ -13,7 +13,9 @@ defmodule Cadet.Auth.Providers.GoogleClaimExtractor do end end - def get_name(_claims), do: nil + def get_name(claims) do + claims["name"] + end def get_role(_claims), do: nil diff --git a/lib/cadet/auth/providers/luminus.ex b/lib/cadet/auth/providers/luminus.ex index 627f4a1a4..7caa73d89 100644 --- a/lib/cadet/auth/providers/luminus.ex +++ b/lib/cadet/auth/providers/luminus.ex @@ -36,7 +36,7 @@ defmodule Cadet.Auth.Providers.LumiNUS do {:verify_jwt, {:ok, _}} <- {:verify_jwt, Guardian.Token.Jwt.Verify.verify_claims(Cadet.Auth.EmptyGuardian, claims, nil)} do - {:ok, %{token: token, username: username}} + {:ok, %{token: token, username: Provider.namespace(username, "luminus")}} else {:token, {:ok, %{body: body, status_code: status}}} -> {:error, :upstream, "Status code #{status} from LumiNUS: #{body}"} diff --git a/lib/cadet/auth/providers/openid.ex b/lib/cadet/auth/providers/openid.ex index 77a4f9410..1d16c6088 100644 --- a/lib/cadet/auth/providers/openid.ex +++ b/lib/cadet/auth/providers/openid.ex @@ -30,8 +30,15 @@ defmodule Cadet.Auth.Providers.OpenID do nil )} do case claim_extractor.get_username(claims) do - nil -> {:error, :invalid_credentials, "No username specified in token"} - username -> {:ok, %{token: token, username: username}} + nil -> + {:error, :invalid_credentials, "No username specified in token"} + + username -> + {:ok, + %{ + token: token, + username: Provider.namespace(username, Atom.to_string(openid_provider)) + }} end else {:token, {:error, _, _}} -> diff --git a/lib/cadet_web/controllers/auth_controller.ex b/lib/cadet_web/controllers/auth_controller.ex index 8571db6cf..3b589274c 100644 --- a/lib/cadet_web/controllers/auth_controller.ex +++ b/lib/cadet_web/controllers/auth_controller.ex @@ -34,7 +34,7 @@ defmodule CadetWeb.AuthController do {:authorise, {:error, :upstream, reason}} -> conn |> put_status(:bad_request) - |> text("Unable to retrieve token from ADFS: #{reason}") + |> text("Unable to retrieve token from authentication provider: #{reason}") {:authorise, {:error, :invalid_credentials, reason}} -> conn diff --git a/test/cadet/auth/guardian_test.exs b/test/cadet/auth/guardian_test.exs index 90211f252..e445a96e2 100644 --- a/test/cadet/auth/guardian_test.exs +++ b/test/cadet/auth/guardian_test.exs @@ -1,6 +1,7 @@ defmodule Cadet.Auth.GuardianTest do use Cadet.DataCase + import Cadet.TestHelper alias Cadet.Auth.Guardian test "token subject is user id" do @@ -19,7 +20,9 @@ defmodule Cadet.Auth.GuardianTest do "sub" => "2000" } - assert Guardian.resource_from_claims(good_claims) == {:ok, user} + assert Guardian.resource_from_claims(good_claims) == + {:ok, remove_preload(user, :latest_viewed)} + assert Guardian.resource_from_claims(bad_claims) == {:error, :not_found} end end diff --git a/test/cadet/auth/provider_test.exs b/test/cadet/auth/provider_test.exs index 7836323b7..3d59ae01e 100644 --- a/test/cadet/auth/provider_test.exs +++ b/test/cadet/auth/provider_test.exs @@ -10,7 +10,6 @@ defmodule Cadet.Auth.ProviderTest do test "with valid provider" do assert {:ok, _} = Provider.authorise("test", "student_code", nil, nil) assert {:ok, _} = Provider.get_name("test", "student_token") - assert {:ok, _} = Provider.get_role("test", "student_token") end test "with invalid provider" do diff --git a/test/cadet/auth/providers/config_test.exs b/test/cadet/auth/providers/config_test.exs index b2ad33aef..c75ffa3a4 100644 --- a/test/cadet/auth/providers/config_test.exs +++ b/test/cadet/auth/providers/config_test.exs @@ -7,6 +7,7 @@ defmodule Cadet.Auth.Providers.ConfigTest do @token "token" @name "Test Name" @username "testusername" + @namespaced_username "test/testusername" @role :student @config [ @@ -21,7 +22,7 @@ defmodule Cadet.Auth.Providers.ConfigTest do describe "authorise" do test "successfully" do - assert {:ok, %{token: @token, username: @username}} = + assert {:ok, %{token: @token, username: @namespaced_username}} = Config.authorise(@config, @code, nil, nil) end diff --git a/test/cadet/auth/providers/github_test.exs b/test/cadet/auth/providers/github_test.exs new file mode 100644 index 000000000..582a183dd --- /dev/null +++ b/test/cadet/auth/providers/github_test.exs @@ -0,0 +1,105 @@ +defmodule Cadet.Auth.Providers.GitHubTest do + use ExUnit.Case, async: false + + alias Cadet.Auth.Providers.GitHub + alias Plug.Conn, as: PlugConn + + @username "username" + @namespaced_username "github/username" + @name "name" + + @dummy_access_token "dummy_access_token" + + setup_all do + Application.ensure_all_started(:bypass) + bypass = Bypass.open() + + {:ok, bypass: bypass} + end + + defp config(bypass) do + %{ + clients: %{"dummy_client_id" => "dummy_client_secret"}, + token_url: "http://localhost:#{bypass.port}/login/oauth/access_token", + user_api: "http://localhost:#{bypass.port}/user" + } + end + + defp bypass_return_token(bypass) do + Bypass.stub(bypass, "POST", "login/oauth/access_token", fn conn -> + conn + |> PlugConn.put_resp_header("content-type", "application/json") + |> PlugConn.resp(200, ~s({"access_token":"#{@dummy_access_token}"})) + end) + end + + defp bypass_api_call(bypass) do + Bypass.stub(bypass, "GET", "user", fn conn -> + conn + |> PlugConn.put_resp_header("content-type", "application/json") + |> PlugConn.resp(200, ~s({"login":"#{@username}","name":"#{@name}"})) + end) + end + + test "successful", %{bypass: bypass} do + bypass_return_token(bypass) + bypass_api_call(bypass) + + assert {:ok, %{token: @dummy_access_token, username: @namespaced_username}} == + GitHub.authorise(config(bypass), "", "dummy_client_id", "") + end + + test "invalid github client id", %{bypass: bypass} do + bypass_return_token(bypass) + bypass_api_call(bypass) + + assert {:error, :invalid_credentials, "Invalid client id"} == + GitHub.authorise(config(bypass), "", "invalid_client_id", "") + end + + test "non-successful HTTP status (access token)", %{bypass: bypass} do + Bypass.stub(bypass, "POST", "login/oauth/access_token", fn conn -> + PlugConn.resp(conn, 403, "") + end) + + assert {:error, :upstream, "Status code 403 from GitHub"} == + GitHub.authorise(config(bypass), "", "dummy_client_id", "") + end + + test "error token response", %{bypass: bypass} do + Bypass.stub(bypass, "POST", "login/oauth/access_token", fn conn -> + conn + |> PlugConn.put_resp_header("content-type", "application/json") + |> PlugConn.resp(200, ~s({"error":"bad_verification_code"})) + + assert {:error, :invalid_credentials, "Error from GitHub: bad_verification_code"} == + GitHub.authorise(config(bypass), "", "dummy_client_id", "") + end) + end + + test "non-successful HTTP status (user api call)", %{bypass: bypass} do + bypass_return_token(bypass) + + Bypass.stub(bypass, "GET", "user", fn conn -> + PlugConn.resp(conn, 401, "") + end) + + assert {:error, :upstream, "Status code 401 from GitHub"} + GitHub.authorise(config(bypass), "", "dummy_client_id", "") + end + + test "get_name successful", %{bypass: bypass} do + bypass_api_call(bypass) + + assert {:ok, @name} == GitHub.get_name(config(bypass), @dummy_access_token) + end + + test "get_name non-successful HTTP status", %{bypass: bypass} do + Bypass.stub(bypass, "GET", "user", fn conn -> + PlugConn.resp(conn, 401, "") + end) + + assert {:error, :upstream, "Status code 401 from GitHub"} == + GitHub.get_name(config(bypass), "invalid_access_token") + end +end diff --git a/test/cadet/auth/providers/openid_test.exs b/test/cadet/auth/providers/openid_test.exs index 27d0aed70..6526a395b 100644 --- a/test/cadet/auth/providers/openid_test.exs +++ b/test/cadet/auth/providers/openid_test.exs @@ -33,6 +33,7 @@ defmodule Cadet.Auth.Providers.OpenIDTest do """ @username "username" + @namespaced_username "test/username" @role :admin @openid_provider_name :test @@ -103,7 +104,7 @@ defmodule Cadet.Auth.Providers.OpenIDTest do bypass_return_token(bypass, @okay_token) - assert {:ok, %{token: @okay_token, username: @username}} = + assert {:ok, %{token: @okay_token, username: @namespaced_username}} = OpenID.authorise(@config, "dummy_code", "", "") assert {:ok, @username} == OpenID.get_name(@config, @okay_token) diff --git a/test/factories/accounts/user_factory.ex b/test/factories/accounts/user_factory.ex index ebc2950ff..d51abc482 100644 --- a/test/factories/accounts/user_factory.ex +++ b/test/factories/accounts/user_factory.ex @@ -14,7 +14,7 @@ defmodule Cadet.Accounts.UserFactory do username: sequence( :nusnet_id, - &"E#{&1 |> Integer.to_string() |> String.pad_leading(7, "0")}" + &"test/E#{&1 |> Integer.to_string() |> String.pad_leading(7, "0")}" ), game_states: %{} } @@ -27,7 +27,7 @@ defmodule Cadet.Accounts.UserFactory do username: sequence( :nusnet_id, - &"E#{&1 |> Integer.to_string() |> String.pad_leading(7, "0")}" + &"test/E#{&1 |> Integer.to_string() |> String.pad_leading(7, "0")}" ), game_states: %{} } diff --git a/test/test_helper.exs b/test/test_helper.exs index 30e668048..2a7b91426 100644 --- a/test/test_helper.exs +++ b/test/test_helper.exs @@ -6,3 +6,19 @@ ExUnit.start() Faker.start() Ecto.Adapters.SQL.Sandbox.mode(Cadet.Repo, :manual) + +defmodule Cadet.TestHelper do + @doc """ + Removes a preloaded Ecto association. + """ + def remove_preload(struct, field, cardinality \\ :one) do + %{ + struct + | field => %Ecto.Association.NotLoaded{ + __field__: field, + __owner__: struct.__struct__, + __cardinality__: cardinality + } + } + end +end