Skip to content

Resolve "Add a doorkeeper scope suitable for authentication"

username-removed-407765 requested to merge 20492-access-token-scopes into master

What does this MR do?

  • Add a single new scope (in addition to the api scope we've had) - read_user
  • Allow creating OAuth applications and Personal access tokens with a scope selected
  • Enforce scopes in the API

What are the relevant issue numbers?

  • Closes #20492 (closed)
  • EE counterpart for this MR: gitlab-org/gitlab-ee!946

Tasks

  • [#20492 (closed)/!5951 (merged)] Add a doorkeeper scope suitable for authentication
    • Implementation
      • Doorkeeper
        • Create application with scopes
        • Create token with scopes
        • Enforce scopes
        • Pretty names / explanations for scopes
      • Personal Access Tokens
        • Create tokens with scopes
        • Enforce scopes
        • Centralise canonical list of scopes
      • Admin::Application?
      • What happens when no scopes are selected while creating a token / application?
        • They have the API scope
      • Existing OAuth tokens should continue to have the api scope
        • Applications created without any scopes (as before) have scopes set to ''.
        • Any tokens created for these applications have scopes set to 'api'
      • Where is Gitlab::Auth used?
        • Used to authenticate Git pushes by using the token instead of a password
        • Check for the right scope during token validation
      • Existing Personal access tokens should continue to have the api scope
      • How does the sessions API affect this feature?
        • The sessions API allows clients to obtain a private token using the user's username and password
        • The API endpoint does not call authenticate!, so no scope checking occurs (as expected)
        • No changes are necessary heren
    • Test / refactor
      • Move Oauth2::AccessTokenValidationService to AccessTokenValidationService
      • Test on MySQL
      • Run all migrations on a fresh database
      • Write feature specs
      • Run feature specs on MySQL and Postgres
      • Fix build
    • Meta
      • API support added
      • Branch has no merge conflicts with master
      • Squashed related commits together
      • Added screenshots
      • CHANGELOG entry created
      • Created EE merge request
    • Follow-up
      • Verify that rebasing didn't break anything
      • Does find_user_from_warden impact this feature in any way?
      • Tests for sufficient_scope?
      • Tests for scope checks at the API level
      • Tests passing
    • Review
      • Endboss
        • What do you think about making AccessTokenValidationService a class
        • valid_api_token? (or even just api_token?) would be more explicit
        • Could we instead consider tokens with scopes == []
          • Check if migrations run before or after the new code is deployed
      • Miniboss
        • Extract the scope listing into a partial
        • What happens to existing doorkeeper tokens? Will they inherit the api scope?
        • Build passing
        • Extract partial from app/views/admin/applications/show.html.haml and app/views/doorkeeper/applications/show.html.html
        • Split feature / controller specs
    • Before Merge
      • Seed database before migrations, add applications / tokens, migrate, make sure everything is working
      • Check if we need to migrate all existing OAuth applications to use the api scope
      • Resolve conflicts
      • Recheck flows
        • API
          • Personal access token
          • OAuth application created per-user
          • OAuth application created by admin
        • Git over HTTP
          • Personal access token
          • OAuth application created per-user
          • OAuth application created by admin
      • EE MR
      • Recheck migrations on PG
      • Recheck migrations on MySQL
    • Wait for merge
    • After Merge
      • Create issue / MR to add documentation around scopes

Merge request reports