Set the request IP before rate limiting resolves the user

What does this MR do and why?

Gitlab::Middleware::IpAddress was mounted with use inside the Grape app at lib/api/api.rb, so it ran after Gitlab::Middleware::LabkitRackRateLimit had already resolved the authenticated user. That resolution calls PersonalAccessTokens::LastUsedService, which reads Gitlab::IpAddressState.current and finds it nil, so the token's last-used IP is never recorded.

This mounts it in the outer Rails stack instead, directly above the rate limiter, and drops the now-redundant Grape-level mount. Resulting order: Warden::Manager 53, IpAddress 54, LabkitRackRateLimit 55, Rack::Attack 56.

config/application.rb loads before Zeitwerk, so the insert needs the accompanying require_dependency. Without it the application fails to boot with uninitialized constant Gitlab::Middleware::IpAddress.

Two bugs, one of them visible today

Feed paths, reproducible on master. authenticate_sessionless_user!(:rss) is a prepend_before_action, so it runs ahead of ApplicationController's around_action :set_current_ip_address. A feed request authenticated with a personal access token therefore records no IP at all. spec/requests/rack_middlewares/ip_address_spec.rb covers this: it fails on master and passes here.

API paths, only visible once the cohort flags are gone. The rate limiter currently runs behind the six labkit rack cohort flags, which the test suite stubs off, so this cannot be reproduced in CI on master. On the flag removal branch in !253596 (merged), spec/requests/api/personal_access_tokens_spec.rb:420 and :632 fail with actual collection contained: [] without this change and pass with it, leaving PersonalAccessTokens::LastUsedService untouched.

Scope of the change

Gitlab::IpAddressState is now set from the middleware rather than from inside Grape and ApplicationController, so it is available earlier. To check what that actually affects, every read of IpAddressState.current during one authenticated API request was instrumented across four configurations:

Base Change Readers that saw a value
master none LastUsedService only, IP present
master this MR identical
flag removal branch none LastUsedService only, some reads nil
flag removal branch this MR LastUsedService only, IP present

PersonalAccessTokens::LastUsedService is the only reader that appears. The other readers of IpAddressState.current are unaffected on this path: lib/api/internal/base.rb:133 because the middleware skips /api/v\d+/internal/ and internal API sets its own IP explicitly, and the EE readers because they run inside a controller, policy or service that already had one.

Test results

Run locally against a real Postgres and Redis.

  • 716 examples, 0 failures, 8 pending across rack_middlewares/ip_address_spec.rb, api/internal/base_spec.rb, api/personal_access_tokens_spec.rb, auth/auth_finders_spec.rb, personal_access_tokens/last_used_service_spec.rb, application_controller_spec.rb and api/helpers/authentication_spec.rb.
  • The IpAddressState readers and writers, sidekiq_middleware/set_ip_address, action_cable/ip_address_state_callback, application_worker and ee/gitlab/workhorse: 125 examples, 0 failures.
  • On the flag removal branch with this change: 422 examples, 0 failures, 8 pending, and the two failing token examples go from 2 failures to 0.
  • Middleware order and boot confirmed with rails runner. Combined with !254069 (merged) the order is Warden::Manager 53, IpAddress 54, LoadBalancing::RackMiddleware 55, LabkitRackRateLimit 56, so the two changes compose.
  • RuboCop on all three files: no offences.

Not verified: production behaviour, and the reader census covers GET /projects rather than every request shape.

For the authentication group

@ifarkas this is the change @reprazent raised with you. The question was whether widening where Gitlab::IpAddressState is set is acceptable. The census above is the evidence on that: on the API path it changes exactly one consumer, the one being fixed.

References

Screenshots or screen recordings

Not applicable, no user interface change.

How to set up and validate locally

  1. On master, confirm the feed bug: bundle exec rspec spec/requests/rack_middlewares/ip_address_spec.rb fails.

  2. On this branch, the same command passes.

  3. The API side needs the flag removal branch, since the suite stubs the cohort flags off:

    git switch hgala/29663-remove-labkit-rack-cohort-flags
    bundle exec rspec spec/requests/api/personal_access_tokens_spec.rb:420 spec/requests/api/personal_access_tokens_spec.rb:632

    Both fail. Apply this branch's config/application.rb and lib/api/api.rb on top and both pass.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Merge request reports

Loading
Loading