Fix SAML regression (#1371)

Regression was introduced in #1350: the path order did not allow auth
callback to be ever called.

Additionally, acceptance tests for SAML are added and we added a
blacklist of SAML config ids to prevent further route collisions.

Fixes #1362

.github/workflows/test.yml

@@ -290,6 +290,18 @@ jobs:
           - 8200:8200/tcp
         options:
           --cap-add=IPC_LOCK
+      saml-idp:
+        image: vihangk1/docker-test-saml-idp:latest
+        env:
+          SIMPLESAMLPHP_SP_ENTITY_ID: 'urn:firezone.dev:firezone-app'
+          SIMPLESAMLPHP_SP_ASSERTION_CONSUMER_SERVICE: 'http://localhost:4002/auth/saml/sp/consume/mysamlidp'
+          SIMPLESAMLPHP_SP_SINGLE_LOGOUT_SERVICE: 'http://localhost:4002/auth/saml/sp/logout/mysamlidp'
+          SIMPLESAMLPHP_SP_NAME_ID_FORMAT: 'urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress'
+          SIMPLESAMLPHP_SP_NAME_ID_ATTRIBUTE: 'email'
+          SIMPLESAMLPHP_IDP_AUTH: 'example-userpass'
+        ports:
+          - 8400:8080/tcp
+          - 8443:8443/tcp
     steps:
       - uses: nanasess/setup-chromedriver@v1
       - run: |

apps/fz_http/lib/fz_http/configurations/configuration/saml_identity_provider.ex

@@ -5,6 +5,12 @@ defmodule FzHttp.Configurations.Configuration.SAMLIdentityProvider do
   use FzHttp, :schema
   import Ecto.Changeset
 
+  @reserved_config_ids [
+    "identity",
+    "saml",
+    "magic_link"
+  ]
+
   @primary_key false
   embedded_schema do
     field :id, :string
@@ -40,6 +46,8 @@ defmodule FzHttp.Configurations.Configuration.SAMLIdentityProvider do
     ])
     |> FzHttp.Validator.validate_uri(:base_url)
     |> validate_metadata()
+    # Don't allow users to enter reserved config ids
+    |> validate_exclusion(:id, @reserved_config_ids)
   end
 
   def validate_metadata(changeset) do

apps/fz_http/lib/fz_http_web/controllers/auth_controller.ex

@@ -58,6 +58,17 @@ defmodule FzHttpWeb.AuthController do
     with {:ok, user} <-
            UserFromAuth.find_or_create(:saml, idp, %{"email" => assertion.subject.name}) do
       do_sign_in(conn, user, %{provider: idp})
+    else
+      {:error, %{errors: [email: {"is invalid email address", _metadata}]}} ->
+        conn
+        |> put_flash(
+          :error,
+          "SAML provider did not return a valid email address in `name` assertion"
+        )
+        |> redirect(to: ~p"/")
+
+      other ->
+        other
     end
   end
 

apps/fz_http/lib/fz_http_web/router.ex

@@ -52,6 +52,25 @@ defmodule FzHttpWeb.Router do
     plug FzHttpWeb.Plug.SamlyTargetUrl
   end
 
+  # Local auth routes
+  scope "/auth", FzHttpWeb do
+    pipe_through [
+      :browser,
+      :html_auth,
+      :require_unauthenticated,
+      :require_local_auth
+    ]
+
+    get "/reset_password", AuthController, :reset_password
+    post "/magic_link", AuthController, :magic_link
+    get "/magic/:user_id/:token", AuthController, :magic_sign_in
+
+    get "/:provider", AuthController, :request
+    get "/:provider/callback", AuthController, :callback
+    post "/:provider/callback", AuthController, :callback
+  end
+
+  # XXX: Those routes conflict with the ones above, we must change them in 1.0
   # OIDC auth routes
   scope "/auth/oidc", FzHttpWeb do
     pipe_through [
@@ -71,24 +90,6 @@ defmodule FzHttpWeb.Router do
     forward "/", Samly.Router
   end
 
-  # Local auth routes
-  scope "/auth", FzHttpWeb do
-    pipe_through [
-      :browser,
-      :html_auth,
-      :require_unauthenticated,
-      :require_local_auth
-    ]
-
-    get "/reset_password", AuthController, :reset_password
-    post "/magic_link", AuthController, :magic_link
-    get "/magic/:user_id/:token", AuthController, :magic_sign_in
-
-    get "/:provider", AuthController, :request
-    get "/:provider/callback", AuthController, :callback
-    post "/:provider/callback", AuthController, :callback
-  end
-
   # Unauthenticated routes
   scope "/", FzHttpWeb do
     pipe_through [

apps/fz_http/test/fz_http_web/acceptance/authentication_test.exs

@@ -100,7 +100,38 @@ defmodule FzHttpWeb.Acceptance.AuthenticationTest do
     end
   end
 
-  describe "using OIDC provider" do
+  describe "using SAML provider" do
+    feature "creates a user when auto_create_users is true", %{session: session} do
+      :ok = SimpleSAML.setup_saml_provider()
+
+      session
+      |> visit(~p"/")
+      |> assert_el(Query.text("Sign In", minimum: 1))
+      |> click(Query.link("Sign in with test-saml-idp"))
+      |> assert_el(Query.link("Enter your username and password"))
+      |> fill_in(Query.fillable_field("username"), with: "user1")
+      |> fill_in(Query.fillable_field("password"), with: "user1pass")
+      |> click(Query.button("Login"))
+      |> assert_el(Query.text("Your Devices"))
+    end
+
+    feature "does not create new users when auto_create_users is false", %{session: session} do
+      FzHttp.Configurations.put!(:local_auth_enabled, false)
+      :ok = SimpleSAML.setup_saml_provider(%{"auto_create_users" => false})
+
+      session
+      |> visit(~p"/")
+      |> assert_el(Query.text("Sign In", minimum: 1))
+      |> click(Query.link("Sign in with test-saml-idp"))
+      |> assert_el(Query.link("Enter your username and password"))
+      |> fill_in(Query.fillable_field("username"), with: "user1")
+      |> fill_in(Query.fillable_field("password"), with: "user1pass")
+      |> click(Query.button("Login"))
+      |> assert_el(Query.text("Local auth disabled"))
+    end
+  end
+
+  describe "using OpenID Connect provider" do
     feature "creates a user when auto_create_users is true", %{session: session} do
       oidc_login = "firezone-1"
       oidc_password = "firezone1234_oidc"
@@ -111,6 +142,7 @@ defmodule FzHttpWeb.Acceptance.AuthenticationTest do
 
       session
       |> visit(~p"/")
+      |> assert_el(Query.text("Sign In", minimum: 1))
       |> click(Query.link("OIDC Vault"))
       |> Vault.userpass_flow(oidc_login, oidc_password)
       |> assert_el(Query.text("Your Devices"))
@@ -133,6 +165,7 @@ defmodule FzHttpWeb.Acceptance.AuthenticationTest do
 
       session
       |> visit(~p"/")
+      |> assert_el(Query.text("Sign In", minimum: 1))
       |> click(Query.link("OIDC Vault"))
       |> Vault.userpass_flow(oidc_login, oidc_password)
       |> find(Query.text("Users", count: 2), fn _ -> :ok end)
@@ -155,6 +188,7 @@ defmodule FzHttpWeb.Acceptance.AuthenticationTest do
 
       session
       |> visit(~p"/")
+      |> assert_el(Query.text("Sign In", minimum: 1))
       |> click(Query.link("OIDC Vault"))
       |> Vault.userpass_flow(oidc_login, oidc_password)
       |> assert_error_flash("Error signing in: user not found and auto_create_users disabled")

apps/fz_http/test/support/acceptance_case.ex

@@ -10,7 +10,7 @@ defmodule FzHttpWeb.AcceptanceCase do
       use FzHttpWeb, :verified_routes
       import FzHttpWeb.AcceptanceCase
       alias FzHttp.Repo
-      alias FzHttpWeb.AcceptanceCase.{Vault, Auth}
+      alias FzHttpWeb.AcceptanceCase.{Vault, SimpleSAML, Auth}
 
       # The default endpoint for testing
       @endpoint FzHttpWeb.Endpoint

apps/fz_http/test/support/acceptance_case/simple_saml.ex

@@ -0,0 +1,29 @@
+defmodule FzHttpWeb.AcceptanceCase.SimpleSAML do
+  @endpoint "http://localhost:8400"
+
+  def fetch_metadata!(endpoint) do
+    metadata_url = "#{endpoint}/simplesaml/saml2/idp/metadata.php"
+    {:ok, 200, _headers, metadata} = :hackney.request(:get, metadata_url, [], "", [:with_body])
+    metadata
+  end
+
+  def setup_saml_provider(attrs_overrides \\ %{}) do
+    metadata = fetch_metadata!(@endpoint)
+
+    FzHttp.Configurations.put!(:saml_identity_providers, [
+      %{
+        "id" => "mysamlidp",
+        "label" => "test-saml-idp",
+        "auto_create_users" => true,
+        "sign_requests" => true,
+        "sign_metadata" => true,
+        "signed_assertion_in_resp" => true,
+        "signed_envelopes_in_resp" => true,
+        "metadata" => metadata
+      }
+      |> Map.merge(attrs_overrides)
+    ])
+
+    :ok
+  end
+end

docker-compose.yml

@@ -84,6 +84,22 @@ services:
     networks:
       - app
 
+  saml-idp:
+    # This is a container with this PR merged: https://github.com/kristophjunge/docker-test-saml-idp/pull/27
+    image: vihangk1/docker-test-saml-idp:latest
+    environment:
+      SIMPLESAMLPHP_SP_ENTITY_ID: 'urn:firezone.dev:firezone-app'
+      SIMPLESAMLPHP_SP_ASSERTION_CONSUMER_SERVICE: 'http://localhost:4002/auth/saml/sp/consume/mysamlidp'
+      SIMPLESAMLPHP_SP_SINGLE_LOGOUT_SERVICE: 'http://localhost:4002/auth/saml/sp/logout/mysamlidp'
+      SIMPLESAMLPHP_SP_NAME_ID_FORMAT: 'urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress'
+      SIMPLESAMLPHP_SP_NAME_ID_ATTRIBUTE: 'email'
+      SIMPLESAMLPHP_IDP_AUTH: 'example-userpass'
+    ports:
+      - 8400:8080/tcp
+      - 8443:8443/tcp
+    networks:
+      - app
+
   # Unfortunately the Linux VM kernel for Docker Desktop is not compiled with
   # Dynamic Debug enabled, so we're unable to enable WireGuard debug logging.
   # Since WireGuard is designed to be silent by default, this basically does