fix(guard): remove token cache from repo host account (#153)

forestileao · web-flow · commit 179f067a9c52 · 2025-03-24T13:39:10.000-03:00
diff --git a/ee/rbac/lib/rbac/front_repo/repo_host_account.ex b/ee/rbac/lib/rbac/front_repo/repo_host_account.ex
@@ -46,6 +46,7 @@ defmodule Rbac.FrontRepo.RepoHostAccount do
     field(:permission_scope, :string)
     field(:token, :string)
     field(:refresh_token, :string)
+    field(:token_expires_at, :utc_datetime)
     field(:revoked, :boolean, default: false)
 
     timestamps(inserted_at: :created_at, updated_at: :updated_at, type: :utc_datetime)
diff --git a/ee/rbac/priv/front_repo/migrations/20220314144013_create_repo_host_accounts.exs b/ee/rbac/priv/front_repo/migrations/20220314144013_create_repo_host_accounts.exs
@@ -13,6 +13,7 @@ defmodule Rbac.FrontRepo.Migrations.CreateRepoHostAccounts do
       add :permission_scope, :string
       add :revoked, :boolean, default: false, null: false
       add :refresh_token, :string
+      add :token_expires_at, :utc_datetime
       add :created_at, :utc_datetime
       add :updated_at, :utc_datetime
     end
diff --git a/github_hooks/Gemfile b/github_hooks/Gemfile
@@ -32,7 +32,7 @@ gem "sentry-rails"
 gem "sentry-sidekiq"
 
 gem "sprockets", "< 4"
-gem "nokogiri", "~> 1.18.3"
+gem "nokogiri", "~> 1.18.4"
 
 gem "rt-watchman", :require => "watchman", :github => "renderedtext/watchman", :ref => "74530687a232aea678b6738114c82dfc163657cd"
 gem "rt-logman", :require => "logman"
diff --git a/github_hooks/Gemfile.lock b/github_hooks/Gemfile.lock
@@ -221,7 +221,7 @@ GEM
     net-smtp (0.5.0)
       net-protocol
     nio4r (2.7.4)
-    nokogiri (1.18.3)
+    nokogiri (1.18.5)
       mini_portile2 (~> 2.8.2)
       racc (~> 1.4)
     octokit (4.20.0)
@@ -466,7 +466,7 @@ DEPENDENCIES
   kaminari (~> 1.2.2)
   lograge
   net-http
-  nokogiri (~> 1.18.3)
+  nokogiri (~> 1.18.4)
   octokit
   pg (~> 1.1)
   pry-byebug (~> 3.10, >= 3.10.1)
diff --git a/github_hooks/db/migrate/20250321112218_add_token_expires_at_to_repo_host_accounts.rb b/github_hooks/db/migrate/20250321112218_add_token_expires_at_to_repo_host_accounts.rb
@@ -0,0 +1,5 @@
+class AddTokenExpiresAtToRepoHostAccounts < ActiveRecord::Migration[5.1]
+  def change
+    add_column :repo_host_accounts, :token_expires_at, :datetime, null: true, default: nil
+  end
+end
diff --git a/guard/lib/guard/api/bitbucket.ex b/guard/lib/guard/api/bitbucket.ex
@@ -35,18 +35,10 @@ defmodule Guard.Api.Bitbucket do
   Fetch or refresh access token
   """
   def user_token(repo_host_account) do
-    cache_key = OAuth.token_cache_key(repo_host_account)
-
-    case Cachex.get(:token_cache, cache_key) do
-      {:ok, {token, expires_at}} when not is_nil(token) and token != "" ->
-        if OAuth.valid_token?(expires_at) do
-          {:ok, {token, expires_at}}
-        else
-          fetch_and_cache_token(repo_host_account)
-        end
-
-      _ ->
-        fetch_and_cache_token(repo_host_account)
+    if OAuth.valid_token?(repo_host_account.token_expires_at, nil_valid: false) do
+      {:ok, {repo_host_account.token, repo_host_account.token_expires_at}}
+    else
+      fetch_token(repo_host_account)
     end
   end
 
@@ -63,7 +55,12 @@ defmodule Guard.Api.Bitbucket do
     end
   end
 
-  defp fetch_and_cache_token(repo_host_account) do
+  defp fetch_token(%{refresh_token: refresh_token}) when refresh_token in [nil, ""] do
+    Logger.warning("No refresh token found for Bitbucket repo host account, account is revoked")
+    {:error, :revoked}
+  end
+
+  defp fetch_token(repo_host_account) do
     body_params = %{
       "grant_type" => "refresh_token",
       "refresh_token" => repo_host_account.refresh_token
diff --git a/guard/lib/guard/api/github.ex b/guard/lib/guard/api/github.ex
@@ -36,6 +36,11 @@ defmodule Guard.Api.Github do
 
   @doc """
   Fetch or refresh access token
+
+  # Important Considerations:
+  - By default, GitHub tokens do not expire unless the optional setting in the GitHub app is changed.
+  - If altered, the expires_in and refresh_token will not be null.
+  - That's why we have refresh token logic for GitHub, even if it's not typically used.
   """
   def user_token(repo_host_account) do
     case validate_token(repo_host_account.token) do
@@ -44,6 +49,11 @@ defmodule Guard.Api.Github do
     end
   end
 
+  defp handle_fetch_token(%{refresh_token: refresh_token}) when refresh_token in [nil, ""] do
+    Logger.warning("No refresh token found for GitHub repo host account, account is revoked")
+    {:error, :revoked}
+  end
+
   defp handle_fetch_token(repo_host_account) do
     {:ok, {client_id, client_secret}} = Guard.GitProviderCredentials.get(:github)
 
@@ -58,7 +68,7 @@ defmodule Guard.Api.Github do
 
     case Tesla.post(client, @oauth_path, nil, query: query_params) do
       {:ok, %Tesla.Env{status: status, body: body}} when status in 200..299 ->
-        OAuth.handle_ok_token_response(repo_host_account, body, cache: false)
+        OAuth.handle_ok_token_response(repo_host_account, body)
 
       {:ok, %Tesla.Env{status: status}} when status in 400..499 ->
         Logger.warning("Failed to refresh github token, account might be revoked")
diff --git a/guard/lib/guard/api/gitlab.ex b/guard/lib/guard/api/gitlab.ex
@@ -11,18 +11,10 @@ defmodule Guard.Api.Gitlab do
   Fetch or refresh access token
   """
   def user_token(repo_host_account) do
-    cache_key = OAuth.token_cache_key(repo_host_account)
-
-    case Cachex.get(:token_cache, cache_key) do
-      {:ok, {token, expires_at}} when not is_nil(token) and token != "" ->
-        if OAuth.valid_token?(expires_at) do
-          {:ok, {token, expires_at}}
-        else
-          handle_fetch_and_cache_token(repo_host_account)
-        end
-
-      _ ->
-        handle_fetch_and_cache_token(repo_host_account)
+    if OAuth.valid_token?(repo_host_account.token_expires_at, nil_valid: false) do
+      {:ok, {repo_host_account.token, repo_host_account.token_expires_at}}
+    else
+      handle_fetch_token(repo_host_account)
     end
   end
 
@@ -32,15 +24,20 @@ defmodule Guard.Api.Gitlab do
     case Tesla.get(client, @oauth_token_info_path) do
       {:ok, res} ->
         expires_at = OAuth.calc_expires_at(res.body["expires_in"])
-        {:ok, res.status in 200..299 && OAuth.valid_token?(expires_at)}
+        {:ok, res.status in 200..299 && OAuth.valid_token?(expires_at, nil_valid: false)}
 
       {:error, error} ->
         Logger.error("Error validating token: #{inspect(error)}")
         {:error, :network_error}
     end
   end
 
-  defp handle_fetch_and_cache_token(repo_host_account) do
+  defp handle_fetch_token(%{refresh_token: refresh_token}) when refresh_token in [nil, ""] do
+    Logger.warning("No refresh token found for GitLab repo host account, account is revoked")
+    {:error, :revoked}
+  end
+
+  defp handle_fetch_token(repo_host_account) do
     body_params = %{
       "grant_type" => "refresh_token",
       "refresh_token" => repo_host_account.refresh_token,
diff --git a/guard/lib/guard/application.ex b/guard/lib/guard/application.ex
@@ -144,10 +144,6 @@ defmodule Guard.Application do
         worker: Supervisor.child_spec({Cachex, :ppl_cache}, id: :ppl_cache),
         active: true
       },
-      %{
-        worker: Supervisor.child_spec({Cachex, :token_cache}, id: :token_cache),
-        active: true
-      },
       %{
         worker:
           Supervisor.child_spec({Cachex, :feature_provider_cache}, id: :feature_provider_cache),
diff --git a/guard/lib/guard/front_repo/repo_host_account.ex b/guard/lib/guard/front_repo/repo_host_account.ex
@@ -46,6 +46,7 @@ defmodule Guard.FrontRepo.RepoHostAccount do
     field(:permission_scope, :string)
     field(:token, :string)
     field(:refresh_token, :string)
+    field(:token_expires_at, :utc_datetime)
     field(:revoked, :boolean, default: false)
 
     timestamps(inserted_at: :created_at, updated_at: :updated_at, type: :utc_datetime)
@@ -184,8 +185,14 @@ defmodule Guard.FrontRepo.RepoHostAccount do
     end
   end
 
-  def update_token_pair(rha, token, refresh_token) do
-    update_account(%{token: token, refresh_token: refresh_token}, rha)
+  def update_token(rha, token, refresh_token, expires_at) do
+    params = %{
+      token: token,
+      refresh_token: refresh_token,
+      token_expires_at: expires_at
+    }
+
+    update_account(params, rha)
   end
 
   @spec get_uid_by_login(String.t(), String.t()) :: {:ok, String.t()} | {:error, :not_found}
@@ -314,6 +321,7 @@ defmodule Guard.FrontRepo.RepoHostAccount do
           :revoked,
           :token,
           :refresh_token,
+          :token_expires_at,
           :permission_scope
         ]
       )
diff --git a/guard/lib/guard/grpc_servers/user_server.ex b/guard/lib/guard/grpc_servers/user_server.ex
@@ -384,11 +384,6 @@ defmodule Guard.GrpcServers.UserServer do
           "Token for #{user.id} is #{if is_valid, do: "valid", else: "invalid"}. Updating revoke status."
         )
 
-        unless is_valid do
-          cache_key = Guard.Utils.OAuth.token_cache_key(repo_account)
-          Cachex.del(:token_cache, cache_key)
-        end
-
         FrontRepo.RepoHostAccount.update_revoke_status(repo_account, not is_valid)
 
       {:error, _} ->
diff --git a/guard/lib/guard/utils.ex b/guard/lib/guard/utils.ex
@@ -45,7 +45,7 @@ defmodule Guard.Utils do
 end
 
 defmodule Guard.Utils.OAuth do
-  def handle_ok_token_response(repo_host_account, body, opts \\ [cache: true]) do
+  def handle_ok_token_response(repo_host_account, body) do
     body =
       if is_binary(body) do
         Jason.decode!(body)
@@ -59,19 +59,25 @@ defmodule Guard.Utils.OAuth do
 
     expires_at = calc_expires_at(expires_in)
 
-    if valid_token?(expires_at) do
-      update_token_pair(repo_host_account, token, refresh_token)
-    end
+    # By default, GitHub don't expires, so the expires_at is nil
+    nil_valid = repo_host_account.repo_host == "github"
 
-    if opts[:cache] do
-      Cachex.put(:token_cache, token_cache_key(repo_host_account), {token, expires_at})
+    if valid_token?(expires_at, nil_valid: nil_valid) do
+      update_token(repo_host_account, token, refresh_token, expires_at)
     end
 
     {:ok, {token, expires_at}}
   end
 
-  defp update_token_pair(repo_host_account, token, refresh_token) do
-    Guard.FrontRepo.RepoHostAccount.update_token_pair(repo_host_account, token, refresh_token)
+  defp update_token(repo_host_account, token, refresh_token, expires_at) do
+    {:ok, parsed_expires_at} = expires_at |> DateTime.from_unix(:second)
+
+    Guard.FrontRepo.RepoHostAccount.update_token(
+      repo_host_account,
+      token,
+      refresh_token,
+      parsed_expires_at
+    )
   end
 
   def calc_expires_at(nil), do: nil
@@ -81,16 +87,24 @@ defmodule Guard.Utils.OAuth do
     current_time + expires_in
   end
 
-  # Case where the token never expires
-  def valid_token?(nil), do: true
+  @doc """
+  Validate token
+
+  ## Options
+    - nil_valid: In case expires_at is nil, set token is valid
+  """
+  def valid_token?(expires_at, opts \\ [])
+  def valid_token?(nil, opts), do: opts[:nil_valid] == true
+
+  def valid_token?(%DateTime{} = expires_at, _opts) do
+    expires_at |> DateTime.to_unix() |> valid_token?()
+  end
 
-  def valid_token?(expires_at) do
+  def valid_token?(expires_at, _opts) do
     current_time = DateTime.utc_now() |> DateTime.to_unix()
     # 5 minutes before expiration
     expires_at - 300 > current_time
   end
-
-  def token_cache_key(%{repo_host: repo_host, id: id}), do: "#{repo_host}_token_#{id}"
 end
 
 defmodule Guard.Utils.Http do
diff --git a/guard/priv/front_repo/migrations/20250321030710_add_token_expires_at_to_repo_host_accounts.exs b/guard/priv/front_repo/migrations/20250321030710_add_token_expires_at_to_repo_host_accounts.exs
@@ -0,0 +1,9 @@
+defmodule Guard.FrontRepo.Migrations.AddTokenExpiresAtToRepoHostAccounts do
+  use Ecto.Migration
+
+  def change do
+    alter table(:repo_host_accounts) do
+      add :token_expires_at, :utc_datetime
+    end
+  end
+end
diff --git a/guard/test/guard/api/bitbucket_test.exs b/guard/test/guard/api/bitbucket_test.exs
@@ -2,7 +2,6 @@ defmodule Guard.Api.BitbucketTest do
   use Guard.RepoCase
 
   alias Guard.Api.Bitbucket
-  alias Guard.Utils.OAuth
 
   setup do
     {:ok, user} = Support.Factories.RbacUser.insert()
@@ -23,6 +22,7 @@ defmodule Guard.Api.BitbucketTest do
         refresh_token: "example_refresh_token",
         user_id: user.id,
         token: "token",
+        token_expires_at: Support.Members.valid_expires_at(),
         revoked: false,
         permission_scope: "repo"
       )
@@ -31,14 +31,14 @@ defmodule Guard.Api.BitbucketTest do
   end
 
   describe "user_token/1" do
-    test "returns cached token when valid", %{repo_host_account: rha} do
-      cache_key = OAuth.token_cache_key(rha)
-      Cachex.put(:token_cache, cache_key, {"cached_token", valid_expires_at()})
-
-      assert {:ok, {"cached_token", _}} = Bitbucket.user_token(rha)
+    test "returns current token when valid", %{repo_host_account: rha} do
+      assert {:ok, {stored_token, _}} = Bitbucket.user_token(rha)
+      assert stored_token == rha.token
     end
 
-    test "refreshes token when cache is expired", %{repo_host_account: rha} do
+    test "refreshes token when current one is expired", %{repo_host_account: rha} do
+      rha = Map.put(rha, :token_expires_at, Support.Members.invalid_expires_at())
+
       Tesla.Mock.mock_global(fn
         %{method: :post, url: "https://bitbucket.org/site/oauth2/access_token"} ->
           {:ok,
@@ -60,8 +60,4 @@ defmodule Guard.Api.BitbucketTest do
       assert updated_rha.token == "new_token"
     end
   end
-
-  defp valid_expires_at do
-    (DateTime.utc_now() |> DateTime.to_unix()) + 3600
-  end
 end
diff --git a/guard/test/guard/api/gitlab_test.exs b/guard/test/guard/api/gitlab_test.exs
@@ -2,7 +2,6 @@ defmodule Guard.Api.GitlabTest do
   use Guard.RepoCase
 
   alias Guard.Api.Gitlab
-  alias Guard.Utils.OAuth
 
   setup do
     {:ok, user} = Support.Factories.RbacUser.insert()
@@ -23,6 +22,7 @@ defmodule Guard.Api.GitlabTest do
         refresh_token: "example_refresh_token",
         user_id: user.id,
         token: "token",
+        token_expires_at: Support.Members.valid_expires_at(),
         revoked: false,
         permission_scope: "repo"
       )
@@ -31,14 +31,14 @@ defmodule Guard.Api.GitlabTest do
   end
 
   describe "user_token/1" do
-    test "returns cached token when valid", %{repo_host_account: rha} do
-      cache_key = OAuth.token_cache_key(rha)
-      Cachex.put(:token_cache, cache_key, {"cached_token", valid_expires_at()})
-
-      assert {:ok, {"cached_token", _}} = Gitlab.user_token(rha)
+    test "returns current token when valid", %{repo_host_account: rha} do
+      assert {:ok, {token, _}} = Gitlab.user_token(rha)
+      assert token == rha.token
     end
 
-    test "refreshes token when cache is expired", %{repo_host_account: rha} do
+    test "refreshes token when current one is expired", %{repo_host_account: rha} do
+      rha = Map.put(rha, :token_expires_at, Support.Members.invalid_expires_at())
+
       Tesla.Mock.mock_global(fn
         %{method: :post, url: "https://gitlab.com/oauth/token"} ->
           {:ok,
@@ -57,8 +57,4 @@ defmodule Guard.Api.GitlabTest do
       assert updated_rha.token == "new_token"
     end
   end
-
-  defp valid_expires_at do
-    (DateTime.utc_now() |> DateTime.to_unix()) + 3600
-  end
 end
diff --git a/guard/test/support/members.ex b/guard/test/support/members.ex
