Migrate unregister_device queries to Ecto

Use Ecto to compose and run the query.

This introduces a change in how the API responds to user requests for
unregistering devices.

Before this change, unregistering an existing device returned
successfully regardless of the device registration status,
and unregistering a non-existing device returned `{:error,
:device_not_registered}` which was translated as a 403 Forbidden error
from Pairing API.

Now, the idempotency on existing devices is kept as before, but Pairing
returns `{:error, :device_not_found}` for a non-existing device, which
is translated by Pairing API as a 404 Not Found error.

This change better reflects the error that actually happened instead of
giving misleading feedback to API users. Moreover, for all concerns
about authorization or security concerns, Pairing API already returns
403 Forbidden errors when the API user requests an operation on a device
that is not allowed with the permissions contained in the auth token.

This commit also ensures that `{:error, :device_not_found}` errors are
correctly handled in a similar way by Pairing API in the case of other
operations that return that error. Indeed, the APIs to get device
credentials, verify device credentials, and get device info, may all
return such an error, as the change was introduced in PR #1082.

Signed-off-by: Davide Briani <davide.briani@secomind.com>

Author: davidebriani

apps/astarte_pairing/lib/astarte_pairing/queries.ex

@@ -123,14 +123,10 @@ defmodule Astarte.Pairing.Queries do
   end
 
   def unregister_device(realm_name, device_id) do
-    with :ok <- verify_already_registered_device(realm_name, device_id),
-         :ok <- do_unregister_device(realm_name, device_id) do
+    with {:ok, device} <- fetch_device(realm_name, device_id),
+         {:ok, _device} <- do_unregister_device(realm_name, device) do
       :ok
     else
-      %{acc: _acc, msg: msg} ->
-        Logger.warning("DB error: #{inspect(msg)}")
-        {:error, :database_error}
-
       {:error, reason} ->
         Logger.warning("Unregister error: #{inspect(reason)}")
         {:error, reason}
@@ -147,44 +143,16 @@ defmodule Astarte.Pairing.Queries do
     end
   end
 
-  defp verify_already_registered_device(realm_name, device_id) do
-    case fetch_device(realm_name, device_id) do
-      {:ok, _device} -> :ok
-      {:error, :device_not_found} -> {:error, :device_not_registered}
-      {:error, reason} -> {:error, reason}
-    end
-  end
-
-  defp do_unregister_device(realm_name, device_id) do
-    statement = """
-    INSERT INTO devices
-    (device_id, first_credentials_request, credentials_secret)
-    VALUES (:device_id, :first_credentials_request, :credentials_secret)
-    """
-
-    query =
-      Query.new()
-      |> Query.statement(statement)
-      |> Query.put(:device_id, device_id)
-      |> Query.put(:first_credentials_request, nil)
-      |> Query.put(:credentials_secret, nil)
-      |> Query.consistency(:quorum)
-
+  defp do_unregister_device(realm_name, %Device{} = device) do
     keyspace_name =
       CQLUtils.realm_name_to_keyspace_name(realm_name, Config.astarte_instance_id!())
 
-    cqex_options =
-      Config.cqex_options!()
-      |> Keyword.put(:keyspace, keyspace_name)
-
-    with {:ok, client} <-
-           CQEx.Client.new(
-             Config.cassandra_node!(),
-             cqex_options
-           ),
-         {:ok, _res} <- Query.call(client, query) do
-      :ok
-    end
+    device
+    |> Ecto.Changeset.change(
+      first_credentials_request: nil,
+      credentials_secret: nil
+    )
+    |> Repo.update(prefix: keyspace_name, consistency: :quorum)
   end
 
   def fetch_device(realm_name, device_id) do

apps/astarte_pairing/test/astarte_pairing/engine_test.exs

@@ -20,8 +20,6 @@ defmodule Astarte.Pairing.EngineTest do
   use ExUnit.Case
 
   alias Astarte.Core.Device
-  alias Astarte.Pairing.Config
-  alias Astarte.Core.CQLUtils
   alias Astarte.Pairing.CredentialsSecret
   alias Astarte.Pairing.DatabaseTestHelper
   alias Astarte.Pairing.Engine
@@ -226,7 +224,7 @@ defmodule Astarte.Pairing.EngineTest do
     test "fails with never registered device_id" do
       device_id = DatabaseTestHelper.unregistered_128_bit_hw_id()
 
-      assert {:error, :device_not_registered} = Engine.unregister_device(@test_realm, device_id)
+      assert {:error, :device_not_found} = Engine.unregister_device(@test_realm, device_id)
     end
 
     test "succeeds with registered and confirmed device_id, and makes it possible to register it again" do

apps/astarte_pairing_api/lib/astarte_pairing_api/credentials/credentials.ex

@@ -47,6 +47,9 @@ defmodule Astarte.Pairing.API.Credentials do
       {:error, :forbidden} ->
         {:error, :forbidden}
 
+      {:error, :device_not_found} ->
+        {:error, :device_not_found}
+
       {:error, _other} ->
         {:error, :rpc_error}
     end
@@ -76,6 +79,9 @@ defmodule Astarte.Pairing.API.Credentials do
       {:error, :forbidden} ->
         {:error, :forbidden}
 
+      {:error, :device_not_found} ->
+        {:error, :device_not_found}
+
       {:error, _reason} ->
         {:error, :rpc_error}
     end

apps/astarte_pairing_api/lib/astarte_pairing_api/rpc/pairing.ex

@@ -209,12 +209,6 @@ defmodule Astarte.Pairing.API.RPC.Pairing do
   end
 
   defp extract_reply({:generic_error_reply, %GenericErrorReply{error_name: "device_not_found"}}) do
-    {:error, :forbidden}
-  end
-
-  defp extract_reply(
-         {:generic_error_reply, %GenericErrorReply{error_name: "device_not_registered"}}
-       ) do
     {:error, :device_not_found}
   end
 

apps/astarte_pairing_api/test/astarte_pairing_api/agent/agent_test.exs

@@ -157,14 +157,14 @@ defmodule Astarte.Pairing.API.AgentTest do
                                    reply: {:generic_ok_reply, %GenericOkReply{}}
                                  }
                                  |> Reply.encode()
-    @encoded_device_not_registered_response %Reply{
-                                              reply:
-                                                {:generic_error_reply,
-                                                 %GenericErrorReply{
-                                                   error_name: "device_not_registered"
-                                                 }}
-                                            }
-                                            |> Reply.encode()
+    @encoded_device_not_found_response %Reply{
+                                         reply:
+                                           {:generic_error_reply,
+                                            %GenericErrorReply{
+                                              error_name: "device_not_found"
+                                            }}
+                                       }
+                                       |> Reply.encode()
     @encoded_realm_not_found_response %Reply{
                                         reply:
                                           {:generic_error_reply,
@@ -205,7 +205,7 @@ defmodule Astarte.Pairing.API.AgentTest do
                  device_id: @test_device_id
                } = unregister_call
 
-        {:ok, @encoded_device_not_registered_response}
+        {:ok, @encoded_device_not_found_response}
       end)
 
       assert {:error, :device_not_found} = Agent.unregister_device(@test_realm, @test_device_id)

apps/astarte_pairing_api/test/astarte_pairing_api/credentials/credentials_test.exs

@@ -189,13 +189,13 @@ defmodule Astarte.Pairing.API.CredentialsTest do
                )
     end
 
-    test "returns forbidden with device not found" do
+    test "returns device_not_found with device not found" do
       MockRPCClient
       |> expect(:rpc_call, fn _serialized_call, @rpc_destination, @timeout ->
         {:ok, @encoded_device_not_found_response}
       end)
 
-      assert {:error, :forbidden} =
+      assert {:error, :device_not_found} =
                Credentials.get_astarte_mqtt_v1(
                  @realm,
                  @unexisting_hw_id,
@@ -336,13 +336,13 @@ defmodule Astarte.Pairing.API.CredentialsTest do
                )
     end
 
-    test "returns forbidden with device not found" do
+    test "returns device_not_found with device not found" do
       MockRPCClient
       |> expect(:rpc_call, fn _serialized_call, @rpc_destination, @timeout ->
         {:ok, @encoded_device_not_found_response}
       end)
 
-      assert {:error, :forbidden} =
+      assert {:error, :device_not_found} =
                Credentials.verify_astarte_mqtt_v1(
                  @realm,
                  @unexisting_hw_id,

apps/astarte_pairing_api/test/astarte_pairing_api_web/controllers/agent_controller_test.exs

@@ -212,14 +212,14 @@ defmodule Astarte.Pairing.APIWeb.AgentControllerTest do
                                    reply: {:generic_ok_reply, %GenericOkReply{}}
                                  }
                                  |> Reply.encode()
-    @encoded_device_not_registered_response %Reply{
-                                              reply:
-                                                {:generic_error_reply,
-                                                 %GenericErrorReply{
-                                                   error_name: "device_not_registered"
-                                                 }}
-                                            }
-                                            |> Reply.encode()
+    @encoded_device_not_found_response %Reply{
+                                         reply:
+                                           {:generic_error_reply,
+                                            %GenericErrorReply{
+                                              error_name: "device_not_found"
+                                            }}
+                                       }
+                                       |> Reply.encode()
 
     test "successful call", %{conn: conn} do
       MockRPCClient
@@ -234,13 +234,13 @@ defmodule Astarte.Pairing.APIWeb.AgentControllerTest do
       assert response(conn, 204) == ""
     end
 
-    test "renders errors when device is not registered", %{conn: conn} do
+    test "renders errors when device does not exist", %{conn: conn} do
       MockRPCClient
       |> expect(:rpc_call, fn _serialized_call, @rpc_destination, @timeout ->
         {:ok, @encoded_pubkey_response}
       end)
       |> expect(:rpc_call, fn _serialized_call, @rpc_destination, @timeout ->
-        {:ok, @encoded_device_not_registered_response}
+        {:ok, @encoded_device_not_found_response}
       end)
 
       conn = delete(conn, agent_path(conn, :delete, @realm, @device_id))