fix(guard): fix gitlab caching null credentials (#69) (#77)

Author: forestileao

guard/config/config.exs

@@ -111,7 +111,8 @@ if System.get_env("AMQP_URL") != nil do
       authorization: [connection: :amqp],
       user: [connection: :amqp],
       organization: [connection: :amqp],
-      project: [connection: :amqp]
+      project: [connection: :amqp],
+      instance_config: [connection: :amqp]
     ]
 end
 

guard/config/runtime.exs

@@ -179,6 +179,7 @@ if System.get_env("AMQP_URL") != nil do
       authorization: [connection: :amqp],
       user: [connection: :amqp],
       organization: [connection: :amqp],
-      project: [connection: :amqp]
+      project: [connection: :amqp],
+      instance_config: [connection: :amqp]
     ]
 end

guard/lib/guard/application.ex

@@ -24,7 +24,6 @@ defmodule Guard.Application do
     services = [Guard.Repo, Guard.FrontRepo] ++ start_instance_config()
 
     children = add_api_servers(services)
-    children = children ++ workers()
     children = children ++ init_feature_provider()
 
     children = children ++ caches()
@@ -66,15 +65,6 @@ defmodule Guard.Application do
     end
   end
 
-  defp workers do
-    select_active([
-      %{
-        worker: {Guard.Workers.SyncOidcUsers, []},
-        active: System.get_env("START_SYNC_OIDC_USERS_WORKER") == "true"
-      }
-    ])
-  end
-
   defp add_api_servers(services) do
     grpc = System.get_env("GRPC_API") || "true"
     rabbit = System.get_env("RABBIT_CONSUMER") || "true"
@@ -112,7 +102,11 @@ defmodule Guard.Application do
   defp add_test_grpc_service(services, _, _), do: services
 
   defp add_id_service(services, "true") do
-    services ++ [{Plug.Cowboy, scheme: :http, plug: Guard.Id.Api, options: [port: 4003]}]
+    services ++
+      [
+        {Plug.Cowboy, scheme: :http, plug: Guard.Id.Api, options: [port: 4003]},
+        {Services.InstanceConfigInvalidatorWorker, []}
+      ]
   end
 
   defp add_id_service(services, _), do: services

guard/lib/guard/events/config_modified.ex

@@ -0,0 +1,27 @@
+defmodule Guard.Events.ConfigModified do
+  @moduledoc """
+  Event emitted when an instance config is modified.
+  """
+
+  @spec publish(InternalApi.InstanceConfig.ConfigType.t()) :: :ok
+  def publish(type) do
+    event =
+      InternalApi.InstanceConfig.ConfigModified.new(
+        type: type,
+        timestamp:
+          Google.Protobuf.Timestamp.new(seconds: DateTime.utc_now() |> DateTime.to_unix(:second))
+      )
+
+    message = InternalApi.InstanceConfig.ConfigModified.encode(event)
+
+    exchange_name = "instance_config_exchange"
+    routing_key = "modified"
+
+    {:ok, channel} = AMQP.Application.get_channel(:instance_config)
+    Tackle.Exchange.create(channel, exchange_name)
+
+    :ok = Tackle.Exchange.publish(channel, exchange_name, message, routing_key)
+
+    :ok
+  end
+end

guard/lib/guard/git_provider_credentials.ex

@@ -47,7 +47,8 @@ defmodule Guard.GitProviderCredentials do
     cache_key = "#{provider}_credentials"
 
     case Cachex.get(:config_cache, cache_key) do
-      {:ok, credentials} when not is_nil(credentials) ->
+      {:ok, %{client_id: client_id, client_secret: client_secret} = credentials}
+      when not is_nil(client_id) and not is_nil(client_secret) ->
         {:ok, credentials}
 
       _ ->

guard/lib/guard/grpc_servers/instance_config_server.ex

@@ -34,9 +34,11 @@ defmodule Guard.GrpcServers.InstanceConfigServer do
 
   def modify_config(request, _stream) do
     Watchman.benchmark("modify_config.duration", fn ->
-      request = request |> request_atom_type()
+      parsed_request = request |> request_atom_type()
+      response = modify_config_(parsed_request.config)
 
-      modify_config_(request.config)
+      Guard.Events.ConfigModified.publish(request.config.type)
+      response
     end)
   end
 

guard/lib/guard/services/instance_config_invalidator_worker.ex

@@ -0,0 +1,38 @@
+defmodule Guard.Services.InstanceConfigInvalidatorWorker do
+  require Logger
+
+  @doc """
+  This module consumes RabbitMQ instance config change events
+  and invalidates instance config caches.
+  """
+
+  use Tackle.Consumer,
+    url: Application.get_env(:guard, :amqp_url),
+    exchange: "instance_config_exchange",
+    routing_key: "modified",
+    service: "guard",
+    queue: :dynamic,
+    queue_opts: [
+      durable: false,
+      auto_delete: true,
+      exclusive: true
+    ]
+
+  def handle_message(message) do
+    event = InternalApi.InstanceConfig.ConfigModified.decode(message)
+
+    cache_key =
+      InternalApi.InstanceConfig.ConfigType.key(event.type)
+      |> cache_key()
+
+    if cache_key, do: Cachex.del(:config_cache, cache_key)
+    Logger.info("Invalidated Instance Config Cache with key: #{inspect(cache_key)}")
+
+    :ok
+  end
+
+  defp cache_key(:CONFIG_TYPE_GITHUB_APP), do: "github_credentials"
+  defp cache_key(:CONFIG_TYPE_BITBUCKET_APP), do: "bitbucket_credentials"
+  defp cache_key(:CONFIG_TYPE_GITLAB_APP), do: "gitlab_credentials"
+  defp cache_key(_), do: nil
+end

guard/lib/internal_api/instance_config.pb.ex

@@ -101,6 +101,20 @@ defmodule InternalApi.InstanceConfig.ConfigType do
   field(:CONFIG_TYPE_GITLAB_APP, 4)
 end
 
+defmodule InternalApi.InstanceConfig.ConfigModified do
+  @moduledoc false
+  use Protobuf, syntax: :proto3
+
+  @type t :: %__MODULE__{
+          type: integer,
+          timestamp: Google.Protobuf.Timestamp.t() | nil
+        }
+
+  defstruct [:type, :timestamp]
+  field(:type, 1, type: InternalApi.InstanceConfig.ConfigType, enum: true)
+  field(:timestamp, 2, type: Google.Protobuf.Timestamp)
+end
+
 defmodule InternalApi.InstanceConfig.InstanceConfigService.Service do
   @moduledoc false
   use GRPC.Service, name: "InternalApi.InstanceConfig.InstanceConfigService"

guard/test/guard/git_provider_credentials_test.exs

@@ -0,0 +1,113 @@
+defmodule Guard.GitProviderCredentialsTest do
+  use Guard.RepoCase, async: false
+  alias Guard.InstanceConfig.Store
+
+  setup do
+    Application.put_env(:guard, :include_instance_config, true)
+    Guard.Mocks.GithubAppApi.github_app_api()
+    Guard.InstanceConfigRepo.delete_all(Guard.InstanceConfig.Models.Config)
+    Cachex.clear(:config_cache)
+
+    :ok
+  end
+
+  describe "get/1" do
+    test "should retrieve valid credentials from each provider when instance config is configured" do
+      setup_github_app_integration()
+      setup_gitlab_app_integration()
+      setup_bitbucket_app_integration()
+
+      assert {:ok, {"client_id", "client_secret"}} == Guard.GitProviderCredentials.get(:github)
+      assert {:ok, {"client_id", "client_secret"}} == Guard.GitProviderCredentials.get(:gitlab)
+      assert {:ok, {"client_id", "client_secret"}} == Guard.GitProviderCredentials.get(:bitbucket)
+    end
+
+    test "should return error when provider is not configured" do
+      assert {:error, :not_found} == Guard.GitProviderCredentials.get(:github)
+      assert {:error, :not_found} == Guard.GitProviderCredentials.get(:gitlab)
+      assert {:error, :not_found} == Guard.GitProviderCredentials.get(:bitbucket)
+    end
+
+    test "should refetch from instance config when cache is wrong" do
+      setup_github_app_integration()
+      setup_gitlab_app_integration()
+      setup_bitbucket_app_integration()
+
+      Cachex.put(:config_cache, "github_credentials", %{client_id: nil, client_secret: nil})
+      Cachex.put(:config_cache, "gitlab_credentials", %{client_id: nil, client_secret: nil})
+      Cachex.put(:config_cache, "bitbucket_credentials", %{client_id: nil, client_secret: nil})
+
+      assert {:ok, {"client_id", "client_secret"}} == Guard.GitProviderCredentials.get(:github)
+      assert {:ok, {"client_id", "client_secret"}} == Guard.GitProviderCredentials.get(:gitlab)
+      assert {:ok, {"client_id", "client_secret"}} == Guard.GitProviderCredentials.get(:bitbucket)
+    end
+
+    test "should get credentials from cache if it contains valid credentials" do
+      Cachex.put(:config_cache, "github_credentials", %{
+        client_id: "cached_client_id",
+        client_secret: "cached_secret"
+      })
+
+      Cachex.put(:config_cache, "gitlab_credentials", %{
+        client_id: "cached_client_id",
+        client_secret: "cached_secret"
+      })
+
+      Cachex.put(:config_cache, "bitbucket_credentials", %{
+        client_id: "cached_client_id",
+        client_secret: "cached_secret"
+      })
+
+      assert {:ok, {"cached_client_id", "cached_secret"}} ==
+               Guard.GitProviderCredentials.get(:github)
+
+      assert {:ok, {"cached_client_id", "cached_secret"}} ==
+               Guard.GitProviderCredentials.get(:gitlab)
+
+      assert {:ok, {"cached_client_id", "cached_secret"}} ==
+               Guard.GitProviderCredentials.get(:bitbucket)
+    end
+  end
+
+  defp setup_github_app_integration do
+    private_key = JOSE.JWK.generate_key({:rsa, 1024})
+    {_, pem_private_key} = JOSE.JWK.to_pem(private_key)
+
+    Guard.InstanceConfig.Models.Config.changeset(%Guard.InstanceConfig.Models.Config{}, %{
+      name: :CONFIG_TYPE_GITHUB_APP |> Atom.to_string(),
+      config: %{
+        app_id: "11111111111111111111111",
+        slug: "slug",
+        name: "name",
+        client_id: "client_id",
+        client_secret: "client_secret",
+        pem: pem_private_key,
+        html_url: "https://github.com",
+        webhook_secret: "webhook_secret"
+      }
+    })
+    |> Store.set()
+  end
+
+  defp setup_gitlab_app_integration do
+    Guard.InstanceConfig.Models.Config.changeset(%Guard.InstanceConfig.Models.Config{}, %{
+      name: :CONFIG_TYPE_GITLAB_APP |> Atom.to_string(),
+      config: %{
+        client_id: "client_id",
+        client_secret: "client_secret"
+      }
+    })
+    |> Store.set()
+  end
+
+  defp setup_bitbucket_app_integration do
+    Guard.InstanceConfig.Models.Config.changeset(%Guard.InstanceConfig.Models.Config{}, %{
+      name: :CONFIG_TYPE_BITBUCKET_APP |> Atom.to_string(),
+      config: %{
+        client_id: "client_id",
+        client_secret: "client_secret"
+      }
+    })
+    |> Store.set()
+  end
+end

guard/test/guard/services/instance_config_invalidator_worker_test.exs

@@ -0,0 +1,46 @@
+defmodule Guard.Services.InstanceConfigInvalidatorWorker.Test do
+  use Guard.RepoCase
+
+  setup do
+    Support.Guard.Store.clear!()
+
+    :ok
+  end
+
+  describe ".handle_message" do
+    test "Message processes and invalidates the cache" do
+      default_credentials = %{
+        client_id: "client_id",
+        client_secret: "client_secret"
+      }
+
+      Cachex.put(:config_cache, "github_credentials", default_credentials)
+      Cachex.put(:config_cache, "gitlab_credentials", default_credentials)
+      Cachex.put(:config_cache, "bitbucket_credentials", default_credentials)
+
+      [
+        :CONFIG_TYPE_GITHUB_APP,
+        :CONFIG_TYPE_BITBUCKET_APP,
+        :CONFIG_TYPE_GITLAB_APP
+      ]
+      |> Enum.each(fn type ->
+        InternalApi.InstanceConfig.ConfigType.value(type)
+        |> publish_event()
+      end)
+
+      :timer.sleep(1000)
+
+      assert {:ok, false} == Cachex.exists?(:config_cache, "github_credentials")
+      assert {:ok, false} == Cachex.exists?(:config_cache, "gitlab_credentials")
+      assert {:ok, false} == Cachex.exists?(:config_cache, "bitbucket_credentials")
+    end
+  end
+
+  #
+  # Helpers
+  #
+
+  defp publish_event(type) do
+    Guard.Events.ConfigModified.publish(type)
+  end
+end