<!--IssueSummary start--> <details> <summary> Everyone can contribute. [Help move this issue forward](https://handbook.gitlab.com/handbook/marketing/developer-relations/engineering/community-contributors-workflows/#contributor-links) while earning points, leveling up and collecting rewards. </summary> - [Label this issue](https://contributors.gitlab.com/manage-issue?action=label&projectId=278964&issueIid=633608) </details> <!--IssueSummary end--> ## Summary Follow-up to !261675, agreed in review there. The lookups that authenticate a deploy token by its value go through the `DeployToken.active` scope ([`app/models/deploy_token.rb:61`](https://gitlab.com/gitlab-org/gitlab/-/blob/da5834e6773a752b9954e0d9bf9f624164a1ba68/app/models/deploy_token.rb#L61)): ```ruby scope :active, -> { where("revoked = false AND expires_at >= NOW()") } ``` That includes [`Gitlab::Auth::AuthFinders#deploy_token_from_request`](https://gitlab.com/gitlab-org/gitlab/-/blob/da5834e6773a752b9954e0d9bf9f624164a1ba68/lib/gitlab/auth/auth_finders.rb#L176), [`deploy_token_check` in `lib/gitlab/auth.rb`](https://gitlab.com/gitlab-org/gitlab/-/blob/da5834e6773a752b9954e0d9bf9f624164a1ba68/lib/gitlab/auth.rb#L362), [`Gitlab::APIAuthentication::TokenResolver`](https://gitlab.com/gitlab-org/gitlab/-/blob/da5834e6773a752b9954e0d9bf9f624164a1ba68/lib/gitlab/api_authentication/token_resolver.rb#L184), [`DependencyProxy::AuthTokenService`](https://gitlab.com/gitlab-org/gitlab/-/blob/da5834e6773a752b9954e0d9bf9f624164a1ba68/app/services/dependency_proxy/auth_token_service.rb#L25), the [packages manager clients helpers](https://gitlab.com/gitlab-org/gitlab/-/blob/da5834e6773a752b9954e0d9bf9f624164a1ba68/lib/api/helpers/packages_manager_clients_helpers.rb#L26) and the [Conan helpers](https://gitlab.com/gitlab-org/gitlab/-/blob/da5834e6773a752b9954e0d9bf9f624164a1ba68/lib/api/helpers/packages/conan/api_helpers.rb#L309). `DeployToken.gitlab_deploy_token` and the deploy token lists of the project and group repository settings use it too. The token the lookup returns is then checked again in Ruby, through [`DeployToken#active?`](https://gitlab.com/gitlab-org/gitlab/-/blob/da5834e6773a752b9954e0d9bf9f624164a1ba68/app/models/deploy_token.rb#L109-115) (`!revoked && !expired?`): by `has_access_to?`, `has_access_to_group?` and `valid_for_dependency_proxy?`, and by the `deactivated` condition of [`BasePolicy`](https://gitlab.com/gitlab-org/gitlab/-/blob/da5834e6773a752b9954e0d9bf9f624164a1ba68/app/policies/base_policy.rb#L32), which [`GlobalPolicy`](https://gitlab.com/gitlab-org/gitlab/-/blob/da5834e6773a752b9954e0d9bf9f624164a1ba68/app/policies/global_policy.rb#L111-116) uses to prevent `access_api`. On `master`, `DeployToken` decides expiry with a [private override](https://gitlab.com/gitlab-org/gitlab/-/blob/da5834e6773a752b9954e0d9bf9f624164a1ba68/app/models/deploy_token.rb#L206-210) of `expired?` that compares `expires_at` with `Date.today`. !261675 removes it, so `Expirable#expired?` applies ([`app/models/concerns/expirable.rb:13-15`](https://gitlab.com/gitlab-org/gitlab/-/blob/da5834e6773a752b9954e0d9bf9f624164a1ba68/app/models/concerns/expirable.rb#L13-15)): ```ruby def expired? expires? && expires_at <= Time.current end ``` With that, the scope and `expired?` still disagree in two ways: 1. **At the instant in `expires_at`.** The scope finds a token whose `expires_at` equals `NOW()` (`>=`), while `expired?` reports a token whose `expires_at` equals `Time.current` as expired (`<=`). `Expirable`'s own scopes draw the line where `expired?` does: `expired` is `expires_at <= Time.current` and `not_expired` is `expires_at > Time.current` ([`expirable.rb:9-10`](https://gitlab.com/gitlab-org/gitlab/-/blob/da5834e6773a752b9954e0d9bf9f624164a1ba68/app/models/concerns/expirable.rb#L9-10)). 1. **On two clocks.** The scope compares with the database's clock (`NOW()`) and `expired?` with the application's (`Time.current`). When the application host's clock is ahead of the database host's, a token whose `expires_at` falls in that difference is found by the lookup and then reported as expired. Apart from both, the lookup and the checks after it run at different moments of the same request, so a token whose `expires_at` falls between them is also found and then reported as expired. In each case, on the API, the `access_api` check refuses the token. With the first commit of !261675 that answer is `403` with `This deploy token has expired.`, instead of the `500` reported in #600629. A request made a moment later is not authenticated as the token at all, because the lookup no longer finds it. Where the token is used through `has_access_to?`, `has_access_to_group?` or `valid_for_dependency_proxy?`, `active?` is checked before anything is granted, so there too the disagreement decides how a token at its expiry is refused, not whether it is. I have not followed every caller of the scope that far. ## Improvements Two changes to the scope, each with a different reach: - **Compare with `>`** (`expires_at > NOW()`). The scope then draws the line where `expired?` and `Expirable.not_expired` do, which closes item 1. Item 2 and the interval inside a request remain. - **Compare with the application's clock**, as `Expirable.not_expired` does (`arel_table[:expires_at].gt(Time.current)`). Both checks then read the same clock, which closes items 1 and 2. `not_expired` also matches `expires_at IS NULL`, which `deploy_tokens.expires_at` cannot hold: the column is `NOT NULL`, and a token that never expires stores `Forever.date`. So `revoked = false` combined with `not_expired` would select the rows the scope selects today, apart from the instant and the clock. Neither change closes the interval between the lookup and the later checks inside one request. ## Risks The scope backs every lookup listed above, so either change moves the moment at which a deploy token stops authenticating, for every token and every path: by one instant with `>`, and by the difference between the application's and the database's clocks with `Time.current`. The `expired` field of the deploy tokens API reads `expired?`, not the scope, so it does not change. ## Involved components - `app/models/deploy_token.rb`: the `active` scope. - `spec/models/deploy_token_spec.rb`: a spec for the scope. ## Optional: Missing test coverage No model spec covers `DeployToken.active`. The closest are the controller shared examples that count `DeployToken.active` ([`deploy_token_shared_examples.rb:18`](https://gitlab.com/gitlab-org/gitlab/-/blob/da5834e6773a752b9954e0d9bf9f624164a1ba68/spec/support/shared_examples/controllers/deploy_token_shared_examples.rb#L18)). `travel_to` does not move `NOW()`, so a spec of the boundary has to place `expires_at` relative to the database's time, or, with the second change, can use `travel_to` like the specs of `Expirable`.