fix(guard): differentiate between various types of GRPC errors (#522)

Author: skipi

guard/lib/guard/front_repo/user.ex

@@ -94,6 +94,8 @@ defmodule Guard.FrontRepo.User do
                u.authentication_token == ^token and is_nil(u.blocked_at) and
                  (is_nil(u.deactivated) or u.deactivated == false)
            )
+           # Preload service account to avoid N+1 queries when checking if user is a service account
+           |> preload(:service_account)
          ) do
       nil -> {:error, :not_found}
       user -> {:ok, user}

guard/lib/guard/grpc_servers/auth_server.ex

@@ -11,7 +11,9 @@ defmodule Guard.GrpcServers.AuthServer do
     observe("grpc.authentication.authenticate", fn ->
       case find_user_by_token(token) do
         {:ok, user} ->
-          respond_with_user(user, "API_TOKEN", "", "")
+          user
+          |> tap(&log_service_account_access(&1))
+          |> respond_with_user("API_TOKEN", "", "")
 
         {:error, :user, :not_found} ->
           respond_false()
@@ -217,4 +219,11 @@ defmodule Guard.GrpcServers.AuthServer do
       end
     end)
   end
+
+  defp log_service_account_access(%{service_account: nil} = _user), do: :ok
+
+  defp log_service_account_access(%{service_account: _} = user) do
+    Watchman.increment({"service_account.access", [user.org_id]})
+    :ok
+  end
 end

guard/lib/guard/grpc_servers/utils.ex

@@ -18,6 +18,16 @@ defmodule Guard.GrpcServers.Utils do
         Watchman.increment({name, ["OK"]})
         result
       rescue
+        # Some GRPC errors are expected and we don't need to do anything with them(validation, not found, etc).
+        e in GRPC.RPCError ->
+          Logger.error(
+            "Service #{name} - request: #{inspect(request)} - Exited with a failure: #{inspect(e)}"
+          )
+
+          error_category = categorize_grpc_error(e.status)
+          Watchman.increment({name, [error_category]})
+          reraise e, __STACKTRACE__
+
         e ->
           Logger.error(
             "Service #{name} - request: #{inspect(request)} - Exited with an error: #{inspect(e)}"
@@ -28,4 +38,19 @@ defmodule Guard.GrpcServers.Utils do
       end
     end)
   end
+
+  defp categorize_grpc_error(status) do
+    case status do
+      # Client errors (expected)
+      status when status in [3, :invalid_argument] -> "CLIENT_ERROR"
+      status when status in [5, :not_found] -> "CLIENT_ERROR"
+      status when status in [6, :already_exists] -> "CLIENT_ERROR"
+      status when status in [7, :permission_denied] -> "CLIENT_ERROR"
+      status when status in [8, :resource_exhausted] -> "CLIENT_ERROR"
+      status when status in [9, :failed_precondition] -> "CLIENT_ERROR"
+      status when status in [11, :out_of_range] -> "CLIENT_ERROR"
+      # Server errors (unexpected)
+      _ -> "SERVER_ERROR"
+    end
+  end
 end

guard/test/guard/api/bitbucket_test.exs

@@ -1,5 +1,5 @@
 defmodule Guard.Api.BitbucketTest do
-  use Guard.RepoCase
+  use Guard.RepoCase, async: false
 
   alias Guard.Api.Bitbucket
 

guard/test/guard/api/github_test.exs

@@ -1,5 +1,5 @@
 defmodule Guard.Api.GithubTest do
-  use Guard.RepoCase
+  use Guard.RepoCase, async: false
 
   alias Guard.Api.Github
 

guard/test/guard/api/gitlab_test.exs

@@ -1,5 +1,5 @@
 defmodule Guard.Api.GitlabTest do
-  use Guard.RepoCase
+  use Guard.RepoCase, async: false
 
   alias Guard.Api.Gitlab
 

guard/test/guard/grpc_servers/auth_server_test.exs

@@ -1,6 +1,8 @@
 defmodule Guard.GrpcServers.AuthServerTest do
   use Guard.RepoCase, async: false
 
+  import Mock
+
   alias InternalApi.Auth
   alias InternalApi.Auth.Authentication.Stub
   alias InternalApi.Auth.AuthenticateRequest
@@ -190,6 +192,47 @@ defmodule Guard.GrpcServers.AuthServerTest do
 
       assert_response(response, user)
     end
+
+    test "logs service account access metric for service account authentication" do
+      org_id = Ecto.UUID.generate()
+
+      {:ok, %{user: sa_user}} = Support.Factories.ServiceAccountFactory.insert(org_id: org_id)
+
+      token = Guard.AuthenticationToken.new()
+      token_hash = Guard.AuthenticationToken.hash_token(token)
+
+      sa_user
+      |> Ecto.Changeset.change(authentication_token: token_hash)
+      |> Guard.FrontRepo.update()
+
+      with_mock Watchman, [:passthrough], increment: fn _ -> :ok end do
+        # Authenticate with service account token
+        request = AuthenticateRequest.new(token: token)
+        {:ok, channel} = GRPC.Stub.connect("localhost:50051")
+        {:ok, response} = channel |> Stub.authenticate(request)
+
+        assert response.authenticated == true
+        assert response.user_id == sa_user.id
+
+        # Verify service account access metric was logged
+        assert called(Watchman.increment({"service_account.access", [org_id]}))
+      end
+    end
+
+    test "does not log service account metric for regular users", %{token: token, user: user} do
+      with_mock Watchman, [:passthrough], increment: fn _ -> :ok end do
+        # Authenticate with regular user token
+        request = AuthenticateRequest.new(token: token)
+        {:ok, channel} = GRPC.Stub.connect("localhost:50051")
+        {:ok, response} = channel |> Stub.authenticate(request)
+
+        assert response.authenticated == true
+        assert response.user_id == user.id
+
+        # Verify no service account metric was logged
+        assert_not_called(Watchman.increment({"service_account.access", :_}))
+      end
+    end
   end
 
   describe "authenticate_with_cookie" do