• Timothy Andrew's avatar
    Squashed commit representing changes in gitlab-ce!12300. · 6d0dad64
    Timothy Andrew authored
    - There were conflicting changes in `master` that were fixed in
      94258a65. This made rebasing the commits from gitlab-ce!12300
      problematic, due to conflicts.
    
    - Instead, I squashed all !12300 commits into a single commit, and
      cherry-picked that onto 33580-fix-api-scoping-ee, which resulted
      in this commit.
    
    Original commit messages below
    ==============================
    
    Initial attempt at refactoring API scope declarations.
    
    - Declaring an endpoint's scopes in a `before` block has proved to be
      unreliable. For example, if we're accessing the `API::Users` endpoint - code
      in a `before` block in `API::API` wouldn't be able to see the scopes set in
      `API::Users` since the `API::API` `before` block runs first.
    
    - This commit moves these declarations to the class level, since they don't need
      to change once set.
    
    Allow API scope declarations to be applied conditionally.
    
    - Scope declarations of the form:
    
        allow_access_with_scope :read_user, if: -> (request) { request.get? }
    
      will only apply for `GET` requests
    
    - Add a negative test to a `POST` endpoint in the `users` API to test this. Also
      test for this case in the `AccessTokenValidationService` unit tests.
    
    Test `/users` endpoints for the `read_user` scope.
    
    - Test `GET` endpoints to check that the scope is allowed.
    - Test `POST` endpoints to check that the scope is disallowed.
    - Test both `v3` and `v4` endpoints.
    
    When verifying scopes, manually include scopes from `API::API`.
    
    - They are not included automatically since `API::Users` does not inherit from
      `API::API`, as I initially assumed.
    
    - Scopes declared in `API::API` are considered global (to the API), and need to
      be included in all cases.
    
    Test OAuth token scope verification in the `API::Users` endpoint
    
    Add CHANGELOG entry for CE MR 12300
    
    Fix remaining spec failures for !12300.
    
    1. Get the spec for `lib/gitlab/auth.rb` passing.
    
      - Make the `request` argument to `AccessTokenValidationService` optional -
      `auth.rb` doesn't need to pass in a request.
    
      - Pass in scopes in the format `[{ name: 'api' }]` rather than `['api']`, which
      is what `AccessTokenValidationService` now expects.
    
    2. Get the spec for `API::V3::Users` passing
    
    2. Get the spec for `AccessTokenValidationService` passing
    
    Implement review comments from @dbalexandre for !12300.
    
    Implement review comments from @DouweM for !12300.
    
    - Use a struct for scopes, so we can call `scope.if` instead of `scope[:if]`
    
    - Refactor the "remove scopes whose :if condition returns false" logic to use a
      `select` rather than a `reject`.
    
    Extract a `Gitlab::Scope` class.
    
    - To represent an authorization scope, such as `api` or `read_user`
    - This is a better abstraction than the hash we were previously using.
    
    `AccessTokenValidationService` accepts `String` or `API::Scope` scopes.
    
    - There's no need to use `API::Scope` for scopes that don't have `if`
      conditions, such as in `lib/gitlab/auth.rb`.
    
    Fix build for !12300.
    
    - The `/users` and `/users/:id` APIs are now accessible without
      authentication (!12445), and so scopes are not relevant for these endpoints.
    
    - Previously, we were testing our scope declaration against these two methods.
      This commit moves these tests to other `GET` user endpoints which still
      require authentication.
    6d0dad64
auth.rb 7.6 KB