From 1d0ccec6dd8375b751846f69bb170ebd11e9a391 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 22 Nov 2016 14:23:53 +0530 Subject: Add a `scopes` column to the `personal_access_tokens` table --- app/models/personal_access_token.rb | 2 ++ ..._add_column_scopes_to_personal_access_tokens.rb | 36 ++++++++++++++++++++ ...al_access_tokens_default_back_to_empty_array.rb | 39 ++++++++++++++++++++++ db/schema.rb | 1 + spec/factories/personal_access_tokens.rb | 1 + 5 files changed, 79 insertions(+) create mode 100644 db/migrate/20160823083941_add_column_scopes_to_personal_access_tokens.rb create mode 100644 db/migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb diff --git a/app/models/personal_access_token.rb b/app/models/personal_access_token.rb index c4b095e0c04..10a34c42fd8 100644 --- a/app/models/personal_access_token.rb +++ b/app/models/personal_access_token.rb @@ -2,6 +2,8 @@ class PersonalAccessToken < ActiveRecord::Base include TokenAuthenticatable add_authentication_token_field :token + serialize :scopes, Array + belongs_to :user scope :active, -> { where(revoked: false).where("expires_at >= NOW() OR expires_at IS NULL") } diff --git a/db/migrate/20160823083941_add_column_scopes_to_personal_access_tokens.rb b/db/migrate/20160823083941_add_column_scopes_to_personal_access_tokens.rb new file mode 100644 index 00000000000..ab7f0365603 --- /dev/null +++ b/db/migrate/20160823083941_add_column_scopes_to_personal_access_tokens.rb @@ -0,0 +1,36 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddColumnScopesToPersonalAccessTokens < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + # When a migration requires downtime you **must** uncomment the following + # constant and define a short and easy to understand explanation as to why the + # migration requires downtime. + # DOWNTIME_REASON = '' + + # When using the methods "add_concurrent_index" or "add_column_with_default" + # you must disable the use of transactions as these methods can not run in an + # existing transaction. When using "add_concurrent_index" make sure that this + # method is the _only_ method called in the migration, any other changes + # should go in a separate migration. This ensures that upon failure _only_ the + # index creation fails and can be retried or reverted easily. + # + # To disable transactions uncomment the following line and remove these + # comments: + disable_ddl_transaction! + + def up + # The default needs to be `[]`, but all existing access tokens need to have `scopes` set to `['api']`. + # It's easier to achieve this by adding the column with the `['api']` default, and then changing the default to + # `[]`. + add_column_with_default :personal_access_tokens, :scopes, :string, default: ['api'].to_yaml + end + + def down + remove_column :personal_access_tokens, :scopes + end +end diff --git a/db/migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb b/db/migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb new file mode 100644 index 00000000000..018cc3d4747 --- /dev/null +++ b/db/migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb @@ -0,0 +1,39 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class ChangePersonalAccessTokensDefaultBackToEmptyArray < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + # When a migration requires downtime you **must** uncomment the following + # constant and define a short and easy to understand explanation as to why the + # migration requires downtime. + # DOWNTIME_REASON = '' + + # When using the methods "add_concurrent_index" or "add_column_with_default" + # you must disable the use of transactions as these methods can not run in an + # existing transaction. When using "add_concurrent_index" make sure that this + # method is the _only_ method called in the migration, any other changes + # should go in a separate migration. This ensures that upon failure _only_ the + # index creation fails and can be retried or reverted easily. + # + # To disable transactions uncomment the following line and remove these + # comments: + # disable_ddl_transaction! + + def up + # The default needs to be `[]`, but all existing access tokens need to have `scopes` set to `['api']`. + # It's easier to achieve this by adding the column with the `['api']` default, and then changing the default to + # `[]`. + change_column_default :personal_access_tokens, :scopes, [].to_yaml + end + + def down + # The default needs to be `[]`, but all existing access tokens need to have `scopes` set to `['api']`. + # It's easier to achieve this by adding the column with the `['api']` default, and then changing the default to + # `[]`. + change_column_default :personal_access_tokens, :scopes, ['api'].to_yaml + end +end diff --git a/db/schema.rb b/db/schema.rb index 67ff83d96d9..a1a22df0d53 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -854,6 +854,7 @@ ActiveRecord::Schema.define(version: 20161212142807) do t.datetime "expires_at" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.string "scopes", default: "--- []\n", null: false end add_index "personal_access_tokens", ["token"], name: "index_personal_access_tokens_on_token", unique: true, using: :btree diff --git a/spec/factories/personal_access_tokens.rb b/spec/factories/personal_access_tokens.rb index da4c72bcb5b..811eab7e15b 100644 --- a/spec/factories/personal_access_tokens.rb +++ b/spec/factories/personal_access_tokens.rb @@ -5,5 +5,6 @@ FactoryGirl.define do name { FFaker::Product.brand } revoked false expires_at { 5.days.from_now } + scopes ['api'] end end -- cgit v1.2.1 From 6c809dfae84e702f7a49d3fac5725745264e0ff9 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 22 Nov 2016 14:27:31 +0530 Subject: Allow creating personal access tokens / OAuth applications with scopes. --- app/assets/stylesheets/pages/profile.scss | 10 +++++++++ app/controllers/admin/applications_controller.rb | 6 ++++- app/controllers/concerns/oauth_applications.rb | 14 ++++++++++++ app/controllers/oauth/applications_controller.rb | 6 +++++ .../profiles/personal_access_tokens_controller.rb | 12 +++++----- app/views/admin/applications/_form.html.haml | 10 +++++++++ app/views/admin/applications/show.html.haml | 15 +++++++++++-- app/views/doorkeeper/applications/_form.html.haml | 9 ++++++++ app/views/doorkeeper/applications/show.html.haml | 15 ++++++++++++- .../personal_access_tokens/_form.html.haml | 22 ++++++++++++++++++ .../personal_access_tokens/index.html.haml | 17 +++----------- .../profiles/personal_access_tokens_spec.rb | 26 ++++++++++++++++++++++ 12 files changed, 138 insertions(+), 24 deletions(-) create mode 100644 app/controllers/concerns/oauth_applications.rb create mode 100644 app/views/profiles/personal_access_tokens/_form.html.haml diff --git a/app/assets/stylesheets/pages/profile.scss b/app/assets/stylesheets/pages/profile.scss index 8a5b0e20a86..8b1976bd925 100644 --- a/app/assets/stylesheets/pages/profile.scss +++ b/app/assets/stylesheets/pages/profile.scss @@ -262,3 +262,13 @@ table.u2f-registrations { border-right: solid 1px transparent; } } + +.oauth-application-show { + .scope-name { + font-weight: 600; + } + + .scopes-list { + padding-left: 18px; + } +} \ No newline at end of file diff --git a/app/controllers/admin/applications_controller.rb b/app/controllers/admin/applications_controller.rb index 471d24934a0..759044910bb 100644 --- a/app/controllers/admin/applications_controller.rb +++ b/app/controllers/admin/applications_controller.rb @@ -1,4 +1,6 @@ class Admin::ApplicationsController < Admin::ApplicationController + include OauthApplications + before_action :set_application, only: [:show, :edit, :update, :destroy] def index @@ -10,9 +12,11 @@ class Admin::ApplicationsController < Admin::ApplicationController def new @application = Doorkeeper::Application.new + @scopes = Doorkeeper.configuration.scopes end def edit + @scopes = Doorkeeper.configuration.scopes end def create @@ -47,6 +51,6 @@ class Admin::ApplicationsController < Admin::ApplicationController # Only allow a trusted parameter "white list" through. def application_params - params[:doorkeeper_application].permit(:name, :redirect_uri) + params[:doorkeeper_application].permit(:name, :redirect_uri, :scopes) end end diff --git a/app/controllers/concerns/oauth_applications.rb b/app/controllers/concerns/oauth_applications.rb new file mode 100644 index 00000000000..34ad43ededd --- /dev/null +++ b/app/controllers/concerns/oauth_applications.rb @@ -0,0 +1,14 @@ +module OauthApplications + extend ActiveSupport::Concern + + included do + before_action :prepare_scopes, only: [:create, :update] + end + + def prepare_scopes + scopes = params.dig(:doorkeeper_application, :scopes) + if scopes + params[:doorkeeper_application][:scopes] = scopes.join(' ') + end + end +end diff --git a/app/controllers/oauth/applications_controller.rb b/app/controllers/oauth/applications_controller.rb index 0f54dfa4efc..b5449a6c30e 100644 --- a/app/controllers/oauth/applications_controller.rb +++ b/app/controllers/oauth/applications_controller.rb @@ -2,6 +2,7 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController include Gitlab::CurrentSettings include Gitlab::GonHelper include PageLayoutHelper + include OauthApplications before_action :verify_user_oauth_applications_enabled before_action :authenticate_user! @@ -13,6 +14,10 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController set_index_vars end + def edit + @scopes = Doorkeeper.configuration.scopes + end + def create @application = Doorkeeper::Application.new(application_params) @@ -40,6 +45,7 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController @authorized_tokens = current_user.oauth_authorized_tokens @authorized_anonymous_tokens = @authorized_tokens.reject(&:application) @authorized_apps = @authorized_tokens.map(&:application).uniq.reject(&:nil?) + @scopes = Doorkeeper.configuration.scopes # Don't overwrite a value possibly set by `create` @application ||= Doorkeeper::Application.new diff --git a/app/controllers/profiles/personal_access_tokens_controller.rb b/app/controllers/profiles/personal_access_tokens_controller.rb index 508b82a9a6c..6e007f17913 100644 --- a/app/controllers/profiles/personal_access_tokens_controller.rb +++ b/app/controllers/profiles/personal_access_tokens_controller.rb @@ -1,8 +1,6 @@ class Profiles::PersonalAccessTokensController < Profiles::ApplicationController - before_action :load_personal_access_tokens, only: :index - def index - @personal_access_token = current_user.personal_access_tokens.build + set_index_vars end def create @@ -12,7 +10,7 @@ class Profiles::PersonalAccessTokensController < Profiles::ApplicationController flash[:personal_access_token] = @personal_access_token.token redirect_to profile_personal_access_tokens_path, notice: "Your new personal access token has been created." else - load_personal_access_tokens + set_index_vars render :index end end @@ -32,10 +30,12 @@ class Profiles::PersonalAccessTokensController < Profiles::ApplicationController private def personal_access_token_params - params.require(:personal_access_token).permit(:name, :expires_at) + params.require(:personal_access_token).permit(:name, :expires_at, scopes: []) end - def load_personal_access_tokens + def set_index_vars + @personal_access_token ||= current_user.personal_access_tokens.build + @scopes = Gitlab::Auth::SCOPES @active_personal_access_tokens = current_user.personal_access_tokens.active.order(:expires_at) @inactive_personal_access_tokens = current_user.personal_access_tokens.inactive end diff --git a/app/views/admin/applications/_form.html.haml b/app/views/admin/applications/_form.html.haml index 4aacbb8cd77..36d2f415a05 100644 --- a/app/views/admin/applications/_form.html.haml +++ b/app/views/admin/applications/_form.html.haml @@ -18,6 +18,16 @@ Use %code= Doorkeeper.configuration.native_redirect_uri for local tests + + .form-group + = f.label :scopes, class: 'col-sm-2 control-label' + .col-sm-10 + - @scopes.each do |scope| + %fieldset + = check_box_tag 'doorkeeper_application[scopes][]', scope, application.scopes.include?(scope), id: "doorkeeper_application_scopes_#{scope}" + = label_tag "doorkeeper_application_scopes_#{scope}", scope + %span= "(#{t(scope, scope: [:doorkeeper, :scopes])})" + .form-actions = f.submit 'Submit', class: "btn btn-save wide" = link_to "Cancel", admin_applications_path, class: "btn btn-default" diff --git a/app/views/admin/applications/show.html.haml b/app/views/admin/applications/show.html.haml index 3eb9d61972b..3418dc96496 100644 --- a/app/views/admin/applications/show.html.haml +++ b/app/views/admin/applications/show.html.haml @@ -2,8 +2,7 @@ %h3.page-title Application: #{@application.name} - -.table-holder +.table-holder.oauth-application-show %table.table %tr %td @@ -23,6 +22,18 @@ - @application.redirect_uri.split.each do |uri| %div %span.monospace= uri + + - if @application.scopes.present? + %tr + %td + Scopes + %td + %ul.scopes-list.append-bottom-0 + - @application.scopes.each do |scope| + %li + %span.scope-name= scope + = "(#{t(scope, scope: [:doorkeeper, :scopes])})" + .form-actions = link_to 'Edit', edit_admin_application_path(@application), class: 'btn btn-primary wide pull-left' = render 'delete_form', application: @application, submit_btn_css: 'btn btn-danger prepend-left-10' diff --git a/app/views/doorkeeper/applications/_form.html.haml b/app/views/doorkeeper/applications/_form.html.haml index 5c98265727a..6fdb04077b6 100644 --- a/app/views/doorkeeper/applications/_form.html.haml +++ b/app/views/doorkeeper/applications/_form.html.haml @@ -17,5 +17,14 @@ %code= Doorkeeper.configuration.native_redirect_uri for local tests + .form-group + = f.label :scopes, class: 'label-light' + - @scopes.each do |scope| + %fieldset + = check_box_tag 'doorkeeper_application[scopes][]', scope, application.scopes.include?(scope), id: "doorkeeper_application_scopes_#{scope}" + = label_tag "doorkeeper_application_scopes_#{scope}", scope + %span= "(#{t(scope, scope: [:doorkeeper, :scopes])})" + + .prepend-top-default = f.submit 'Save application', class: "btn btn-create" diff --git a/app/views/doorkeeper/applications/show.html.haml b/app/views/doorkeeper/applications/show.html.haml index 47442b78d48..a18e133c8de 100644 --- a/app/views/doorkeeper/applications/show.html.haml +++ b/app/views/doorkeeper/applications/show.html.haml @@ -2,7 +2,7 @@ %h3.page-title Application: #{@application.name} -.table-holder +.table-holder.oauth-application-show %table.table %tr %td @@ -22,6 +22,19 @@ - @application.redirect_uri.split.each do |uri| %div %span.monospace= uri + + - if @application.scopes.present? + %tr + %td + Scopes + %td + %ul.scopes-list.append-bottom-0 + - @application.scopes.each do |scope| + %li + %span.scope-name= scope + = "(#{t(scope, scope: [:doorkeeper, :scopes])})" + + .form-actions = link_to 'Edit', edit_oauth_application_path(@application), class: 'btn btn-primary wide pull-left' = render 'delete_form', application: @application, submit_btn_css: 'btn btn-danger prepend-left-10' diff --git a/app/views/profiles/personal_access_tokens/_form.html.haml b/app/views/profiles/personal_access_tokens/_form.html.haml new file mode 100644 index 00000000000..6083fdaa31d --- /dev/null +++ b/app/views/profiles/personal_access_tokens/_form.html.haml @@ -0,0 +1,22 @@ += form_for [:profile, @personal_access_token], method: :post, html: { class: 'js-requires-input' } do |f| + + = form_errors(@personal_access_token) + + .form-group + = f.label :name, class: 'label-light' + = f.text_field :name, class: "form-control", required: true + + .form-group + = f.label :expires_at, class: 'label-light' + = f.text_field :expires_at, class: "datepicker form-control", required: false + + .form-group + = f.label :scopes, class: 'label-light' + - @scopes.each do |scope| + %fieldset + = check_box_tag 'personal_access_token[scopes][]', scope, @personal_access_token.scopes.include?(scope), id: "personal_access_token_scopes_#{scope}" + = label_tag "personal_access_token_scopes_#{scope}", scope + %span= "(#{t(scope, scope: [:doorkeeper, :scopes])})" + + .prepend-top-default + = f.submit 'Create Personal Access Token', class: "btn btn-create" diff --git a/app/views/profiles/personal_access_tokens/index.html.haml b/app/views/profiles/personal_access_tokens/index.html.haml index 05a2ea67aa2..39eef0f6baf 100644 --- a/app/views/profiles/personal_access_tokens/index.html.haml +++ b/app/views/profiles/personal_access_tokens/index.html.haml @@ -28,21 +28,8 @@ Add a Personal Access Token %p.profile-settings-content Pick a name for the application, and we'll give you a unique token. - = form_for [:profile, @personal_access_token], - method: :post, html: { class: 'js-requires-input' } do |f| - = form_errors(@personal_access_token) - - .form-group - = f.label :name, class: 'label-light' - = f.text_field :name, class: "form-control", required: true - - .form-group - = f.label :expires_at, class: 'label-light' - = f.text_field :expires_at, class: "datepicker form-control", required: false - - .prepend-top-default - = f.submit 'Create Personal Access Token', class: "btn btn-create" + = render "form" %hr @@ -56,6 +43,7 @@ %th Name %th Created %th Expires + %th Scopes %th %tbody - @active_personal_access_tokens.each do |token| @@ -67,6 +55,7 @@ = token.expires_at.to_date.to_s(:medium) - else %span.personal-access-tokens-never-expires-label Never + %td= token.scopes.present? ? token.scopes.join(", ") : "" %td= link_to "Revoke", revoke_profile_personal_access_token_path(token), method: :put, class: "btn btn-danger pull-right", data: { confirm: "Are you sure you want to revoke this token? This action cannot be undone." } - else diff --git a/spec/features/profiles/personal_access_tokens_spec.rb b/spec/features/profiles/personal_access_tokens_spec.rb index a85930c7543..0ffeeff0921 100644 --- a/spec/features/profiles/personal_access_tokens_spec.rb +++ b/spec/features/profiles/personal_access_tokens_spec.rb @@ -51,6 +51,32 @@ describe 'Profile > Personal Access Tokens', feature: true, js: true do expect(active_personal_access_tokens).to have_text(Date.today.next_month.at_beginning_of_month.to_s(:medium)) end + context "scopes" do + it "allows creation of a token with scopes" do + visit profile_personal_access_tokens_path + fill_in "Name", with: FFaker::Product.brand + + check "api" + check "read_user" + + expect {click_on "Create Personal Access Token"}.to change { PersonalAccessToken.count }.by(1) + expect(created_personal_access_token).to eq(PersonalAccessToken.last.token) + expect(PersonalAccessToken.last.scopes).to match_array(['api', 'read_user']) + expect(active_personal_access_tokens).to have_text('api') + expect(active_personal_access_tokens).to have_text('read_user') + end + + it "allows creation of a token with no scopes" do + visit profile_personal_access_tokens_path + fill_in "Name", with: FFaker::Product.brand + + expect {click_on "Create Personal Access Token"}.to change { PersonalAccessToken.count }.by(1) + expect(created_personal_access_token).to eq(PersonalAccessToken.last.token) + expect(PersonalAccessToken.last.scopes).to eq([]) + expect(active_personal_access_tokens).to have_text('no scopes') + end + end + context "when creation fails" do it "displays an error message" do disallow_personal_access_token_saves! -- cgit v1.2.1 From 7fa06ed55d18af4d055041eb27d38fecf9b5548f Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 22 Nov 2016 14:34:23 +0530 Subject: Calls to the API are checked for scope. - Move the `Oauth2::AccessTokenValidationService` class to `AccessTokenValidationService`, since it is now being used for personal access token validation as well. - Each API endpoint declares the scopes it accepts (if any). Currently, the top level API module declares the `api` scope, and the `Users` API module declares the `read_user` scope (for GET requests). - Move the `find_user_by_private_token` from the API `Helpers` module to the `APIGuard` module, to avoid littering `Helpers` with more auth-related methods to support `find_user_by_private_token` --- app/services/access_token_validation_service.rb | 34 ++++++++++++ .../oauth2/access_token_validation_service.rb | 42 --------------- config/initializers/doorkeeper.rb | 4 +- config/locales/doorkeeper.en.yml | 1 + lib/api/api.rb | 2 + lib/api/api_guard.rb | 62 ++++++++++++++++------ lib/api/helpers.rb | 15 ++---- lib/api/users.rb | 5 +- lib/gitlab/auth.rb | 4 ++ spec/requests/api/doorkeeper_access_spec.rb | 2 +- spec/requests/api/helpers_spec.rb | 43 +++++++++------ .../access_token_validation_service_spec.rb | 42 +++++++++++++++ 12 files changed, 164 insertions(+), 92 deletions(-) create mode 100644 app/services/access_token_validation_service.rb delete mode 100644 app/services/oauth2/access_token_validation_service.rb create mode 100644 spec/services/access_token_validation_service_spec.rb diff --git a/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb new file mode 100644 index 00000000000..69449f3a445 --- /dev/null +++ b/app/services/access_token_validation_service.rb @@ -0,0 +1,34 @@ +module AccessTokenValidationService + # Results: + VALID = :valid + EXPIRED = :expired + REVOKED = :revoked + INSUFFICIENT_SCOPE = :insufficient_scope + + class << self + def validate(token, scopes: []) + if token.expired? + return EXPIRED + + elsif token.revoked? + return REVOKED + + elsif !self.sufficient_scope?(token, scopes) + return INSUFFICIENT_SCOPE + + else + return VALID + end + end + + # True if the token's scope contains any of the required scopes. + def sufficient_scope?(token, required_scopes) + if required_scopes.blank? + true + else + # Check whether the token is allowed access to any of the required scopes. + Set.new(required_scopes).intersection(Set.new(token.scopes)).present? + end + end + end +end diff --git a/app/services/oauth2/access_token_validation_service.rb b/app/services/oauth2/access_token_validation_service.rb deleted file mode 100644 index 264fdccde8f..00000000000 --- a/app/services/oauth2/access_token_validation_service.rb +++ /dev/null @@ -1,42 +0,0 @@ -module Oauth2::AccessTokenValidationService - # Results: - VALID = :valid - EXPIRED = :expired - REVOKED = :revoked - INSUFFICIENT_SCOPE = :insufficient_scope - - class << self - def validate(token, scopes: []) - if token.expired? - return EXPIRED - - elsif token.revoked? - return REVOKED - - elsif !self.sufficient_scope?(token, scopes) - return INSUFFICIENT_SCOPE - - else - return VALID - end - end - - protected - - # True if the token's scope is a superset of required scopes, - # or the required scopes is empty. - def sufficient_scope?(token, scopes) - if scopes.blank? - # if no any scopes required, the scopes of token is sufficient. - return true - else - # If there are scopes required, then check whether - # the set of authorized scopes is a superset of the set of required scopes - required_scopes = Set.new(scopes) - authorized_scopes = Set.new(token.scopes) - - return authorized_scopes >= required_scopes - end - end - end -end diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index fc4b0a72add..88cd0f5f652 100644 --- a/config/initializers/doorkeeper.rb +++ b/config/initializers/doorkeeper.rb @@ -52,8 +52,8 @@ Doorkeeper.configure do # Define access token scopes for your provider # For more information go to # https://github.com/doorkeeper-gem/doorkeeper/wiki/Using-Scopes - default_scopes :api - # optional_scopes :write, :update + default_scopes(*Gitlab::Auth::DEFAULT_SCOPES) + optional_scopes(*Gitlab::Auth::OPTIONAL_SCOPES) # Change the way client credentials are retrieved from the request object. # By default it retrieves first from the `HTTP_AUTHORIZATION` header, then diff --git a/config/locales/doorkeeper.en.yml b/config/locales/doorkeeper.en.yml index a4032a21420..1d728282d90 100644 --- a/config/locales/doorkeeper.en.yml +++ b/config/locales/doorkeeper.en.yml @@ -59,6 +59,7 @@ en: unknown: "The access token is invalid" scopes: api: Access your API + read_user: Read user information flash: applications: diff --git a/lib/api/api.rb b/lib/api/api.rb index cec2702e44d..9d5adffd8f4 100644 --- a/lib/api/api.rb +++ b/lib/api/api.rb @@ -3,6 +3,8 @@ module API include APIGuard version 'v3', using: :path + before { allow_access_with_scope :api } + rescue_from Gitlab::Access::AccessDeniedError do rack_response({ 'message' => '403 Forbidden' }.to_json, 403) end diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb index 8cc7a26f1fa..cd266669b1e 100644 --- a/lib/api/api_guard.rb +++ b/lib/api/api_guard.rb @@ -6,6 +6,9 @@ module API module APIGuard extend ActiveSupport::Concern + PRIVATE_TOKEN_HEADER = "HTTP_PRIVATE_TOKEN" + PRIVATE_TOKEN_PARAM = :private_token + included do |base| # OAuth2 Resource Server Authentication use Rack::OAuth2::Server::Resource::Bearer, 'The API' do |request| @@ -41,30 +44,59 @@ module API # Defaults to empty array. # def doorkeeper_guard(scopes: []) - access_token = find_access_token - return nil unless access_token - - case validate_access_token(access_token, scopes) - when Oauth2::AccessTokenValidationService::INSUFFICIENT_SCOPE - raise InsufficientScopeError.new(scopes) + if access_token = find_access_token + case AccessTokenValidationService.validate(access_token, scopes: scopes) + when AccessTokenValidationService::INSUFFICIENT_SCOPE + raise InsufficientScopeError.new(scopes) + when AccessTokenValidationService::EXPIRED + raise ExpiredError + when AccessTokenValidationService::REVOKED + raise RevokedError + when AccessTokenValidationService::VALID + @current_user = User.find(access_token.resource_owner_id) + end + end + end - when Oauth2::AccessTokenValidationService::EXPIRED - raise ExpiredError + def find_user_by_private_token(scopes: []) + token_string = (params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER]).to_s - when Oauth2::AccessTokenValidationService::REVOKED - raise RevokedError + return nil unless token_string.present? - when Oauth2::AccessTokenValidationService::VALID - @current_user = User.find(access_token.resource_owner_id) - end + find_user_by_authentication_token(token_string) || find_user_by_personal_access_token(token_string, scopes) end def current_user @current_user end + # Set the authorization scope(s) allowed for the current request. + # + # Note: A call to this method adds to any previous scopes in place. This is done because + # `Grape` callbacks run from the outside-in: the top-level callback (API::API) runs first, then + # the next-level callback (API::API::Users, for example) runs. All these scopes are valid for the + # given endpoint (GET `/api/users` is accessible by the `api` and `read_user` scopes), and so they + # need to be stored. + def allow_access_with_scope(*scopes) + @scopes ||= [] + @scopes.concat(scopes.map(&:to_s)) + end + private + def find_user_by_authentication_token(token_string) + User.find_by_authentication_token(token_string) + end + + def find_user_by_personal_access_token(token_string, scopes) + access_token = PersonalAccessToken.active.find_by_token(token_string) + return unless access_token + + if AccessTokenValidationService.sufficient_scope?(access_token, scopes) + User.find(access_token.user_id) + end + end + def find_access_token @access_token ||= Doorkeeper.authenticate(doorkeeper_request, Doorkeeper.configuration.access_token_methods) end @@ -72,10 +104,6 @@ module API def doorkeeper_request @doorkeeper_request ||= ActionDispatch::Request.new(env) end - - def validate_access_token(access_token, scopes) - Oauth2::AccessTokenValidationService.validate(access_token, scopes: scopes) - end end module ClassMethods diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 746849ef4c0..4be659fc20b 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -2,8 +2,6 @@ module API module Helpers include Gitlab::Utils - PRIVATE_TOKEN_HEADER = "HTTP_PRIVATE_TOKEN" - PRIVATE_TOKEN_PARAM = :private_token SUDO_HEADER = "HTTP_SUDO" SUDO_PARAM = :sudo @@ -308,7 +306,7 @@ module API private def private_token - params[PRIVATE_TOKEN_PARAM] || env[PRIVATE_TOKEN_HEADER] + params[APIGuard::PRIVATE_TOKEN_PARAM] || env[APIGuard::PRIVATE_TOKEN_HEADER] end def warden @@ -323,18 +321,11 @@ module API warden.try(:authenticate) if %w[GET HEAD].include?(env['REQUEST_METHOD']) end - def find_user_by_private_token - token = private_token - return nil unless token.present? - - User.find_by_authentication_token(token) || User.find_by_personal_access_token(token) - end - def initial_current_user return @initial_current_user if defined?(@initial_current_user) - @initial_current_user ||= find_user_by_private_token - @initial_current_user ||= doorkeeper_guard + @initial_current_user ||= find_user_by_private_token(scopes: @scopes) + @initial_current_user ||= doorkeeper_guard(scopes: @scopes) @initial_current_user ||= find_user_from_warden unless @initial_current_user && Gitlab::UserAccess.new(@initial_current_user).allowed? diff --git a/lib/api/users.rb b/lib/api/users.rb index c7db2d71017..0842c3874c5 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -2,7 +2,10 @@ module API class Users < Grape::API include PaginationParams - before { authenticate! } + before do + allow_access_with_scope :read_user if request.get? + authenticate! + end resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do helpers do diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index aca5d0020cf..c3c464248ef 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -2,6 +2,10 @@ module Gitlab module Auth class MissingPersonalTokenError < StandardError; end + SCOPES = [:api, :read_user] + DEFAULT_SCOPES = [:api] + OPTIONAL_SCOPES = SCOPES - DEFAULT_SCOPES + class << self def find_for_git_client(login, password, project:, ip:) raise "Must provide an IP for rate limiting" if ip.nil? diff --git a/spec/requests/api/doorkeeper_access_spec.rb b/spec/requests/api/doorkeeper_access_spec.rb index 5262a623761..bd9ecaf2685 100644 --- a/spec/requests/api/doorkeeper_access_spec.rb +++ b/spec/requests/api/doorkeeper_access_spec.rb @@ -5,7 +5,7 @@ describe API::API, api: true do let!(:user) { create(:user) } let!(:application) { Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user) } - let!(:token) { Doorkeeper::AccessToken.create! application_id: application.id, resource_owner_id: user.id } + let!(:token) { Doorkeeper::AccessToken.create! application_id: application.id, resource_owner_id: user.id, scopes: "api" } describe "when unauthenticated" do it "returns authentication success" do diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb index 4035fd97af5..15b93118ee4 100644 --- a/spec/requests/api/helpers_spec.rb +++ b/spec/requests/api/helpers_spec.rb @@ -1,6 +1,7 @@ require 'spec_helper' describe API::Helpers, api: true do + include API::APIGuard::HelperMethods include API::Helpers include SentryHelper @@ -15,24 +16,24 @@ describe API::Helpers, api: true do def set_env(user_or_token, identifier) clear_env clear_param - env[API::Helpers::PRIVATE_TOKEN_HEADER] = user_or_token.respond_to?(:private_token) ? user_or_token.private_token : user_or_token + env[API::APIGuard::PRIVATE_TOKEN_HEADER] = user_or_token.respond_to?(:private_token) ? user_or_token.private_token : user_or_token env[API::Helpers::SUDO_HEADER] = identifier.to_s end def set_param(user_or_token, identifier) clear_env clear_param - params[API::Helpers::PRIVATE_TOKEN_PARAM] = user_or_token.respond_to?(:private_token) ? user_or_token.private_token : user_or_token + params[API::APIGuard::PRIVATE_TOKEN_PARAM] = user_or_token.respond_to?(:private_token) ? user_or_token.private_token : user_or_token params[API::Helpers::SUDO_PARAM] = identifier.to_s end def clear_env - env.delete(API::Helpers::PRIVATE_TOKEN_HEADER) + env.delete(API::APIGuard::PRIVATE_TOKEN_HEADER) env.delete(API::Helpers::SUDO_HEADER) end def clear_param - params.delete(API::Helpers::PRIVATE_TOKEN_PARAM) + params.delete(API::APIGuard::PRIVATE_TOKEN_PARAM) params.delete(API::Helpers::SUDO_PARAM) end @@ -94,22 +95,22 @@ describe API::Helpers, api: true do describe "when authenticating using a user's private token" do it "returns nil for an invalid token" do - env[API::Helpers::PRIVATE_TOKEN_HEADER] = 'invalid token' + env[API::APIGuard::PRIVATE_TOKEN_HEADER] = 'invalid token' allow_any_instance_of(self.class).to receive(:doorkeeper_guard){ false } expect(current_user).to be_nil end it "returns nil for a user without access" do - env[API::Helpers::PRIVATE_TOKEN_HEADER] = user.private_token + env[API::APIGuard::PRIVATE_TOKEN_HEADER] = user.private_token allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false) expect(current_user).to be_nil end it "leaves user as is when sudo not specified" do - env[API::Helpers::PRIVATE_TOKEN_HEADER] = user.private_token + env[API::APIGuard::PRIVATE_TOKEN_HEADER] = user.private_token expect(current_user).to eq(user) clear_env - params[API::Helpers::PRIVATE_TOKEN_PARAM] = user.private_token + params[API::APIGuard::PRIVATE_TOKEN_PARAM] = user.private_token expect(current_user).to eq(user) end end @@ -117,37 +118,45 @@ describe API::Helpers, api: true do describe "when authenticating using a user's personal access tokens" do let(:personal_access_token) { create(:personal_access_token, user: user) } + before do + allow_any_instance_of(self.class).to receive(:doorkeeper_guard) { false } + end + it "returns nil for an invalid token" do - env[API::Helpers::PRIVATE_TOKEN_HEADER] = 'invalid token' - allow_any_instance_of(self.class).to receive(:doorkeeper_guard){ false } + env[API::APIGuard::PRIVATE_TOKEN_HEADER] = 'invalid token' expect(current_user).to be_nil end it "returns nil for a user without access" do - env[API::Helpers::PRIVATE_TOKEN_HEADER] = personal_access_token.token + env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false) expect(current_user).to be_nil end + it "returns nil for a token without the appropriate scope" do + personal_access_token = create(:personal_access_token, user: user, scopes: ['read_user']) + env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token + allow_access_with_scope('write_user') + expect(current_user).to be_nil + end + it "leaves user as is when sudo not specified" do - env[API::Helpers::PRIVATE_TOKEN_HEADER] = personal_access_token.token + env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token expect(current_user).to eq(user) clear_env - params[API::Helpers::PRIVATE_TOKEN_PARAM] = personal_access_token.token + params[API::APIGuard::PRIVATE_TOKEN_PARAM] = personal_access_token.token expect(current_user).to eq(user) end it 'does not allow revoked tokens' do personal_access_token.revoke! - env[API::Helpers::PRIVATE_TOKEN_HEADER] = personal_access_token.token - allow_any_instance_of(self.class).to receive(:doorkeeper_guard){ false } + env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token expect(current_user).to be_nil end it 'does not allow expired tokens' do personal_access_token.update_attributes!(expires_at: 1.day.ago) - env[API::Helpers::PRIVATE_TOKEN_HEADER] = personal_access_token.token - allow_any_instance_of(self.class).to receive(:doorkeeper_guard){ false } + env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token expect(current_user).to be_nil end end diff --git a/spec/services/access_token_validation_service_spec.rb b/spec/services/access_token_validation_service_spec.rb new file mode 100644 index 00000000000..8808934fa24 --- /dev/null +++ b/spec/services/access_token_validation_service_spec.rb @@ -0,0 +1,42 @@ +require 'spec_helper' + +describe AccessTokenValidationService, services: true do + + describe ".sufficient_scope?" do + it "returns true if the required scope is present in the token's scopes" do + token = double("token", scopes: [:api, :read_user]) + + expect(described_class.sufficient_scope?(token, [:api])).to be(true) + end + + it "returns true if more than one of the required scopes is present in the token's scopes" do + token = double("token", scopes: [:api, :read_user, :other_scope]) + + expect(described_class.sufficient_scope?(token, [:api, :other_scope])).to be(true) + end + + it "returns true if the list of required scopes is an exact match for the token's scopes" do + token = double("token", scopes: [:api, :read_user, :other_scope]) + + expect(described_class.sufficient_scope?(token, [:api, :read_user, :other_scope])).to be(true) + end + + it "returns true if the list of required scopes contains all of the token's scopes, in addition to others" do + token = double("token", scopes: [:api, :read_user]) + + expect(described_class.sufficient_scope?(token, [:api, :read_user, :other_scope])).to be(true) + end + + it 'returns true if the list of required scopes is blank' do + token = double("token", scopes: []) + + expect(described_class.sufficient_scope?(token, [])).to be(true) + end + + it "returns false if there are no scopes in common between the required scopes and the token scopes" do + token = double("token", scopes: [:api, :read_user]) + + expect(described_class.sufficient_scope?(token, [:other_scope])).to be(false) + end + end +end -- cgit v1.2.1 From 36b3210b9ec4fffd9fa5a73626907e8a6a59f435 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 22 Nov 2016 14:43:37 +0530 Subject: Validate access token scopes in `Gitlab::Auth` - This module is used for git-over-http, as well as JWT. - The only valid scope here is `api`, currently. --- lib/gitlab/auth.rb | 14 ++++++++++--- spec/lib/gitlab/auth_spec.rb | 46 +++++++++++++++++++++++++++++++++++------- spec/requests/git_http_spec.rb | 2 +- 3 files changed, 51 insertions(+), 11 deletions(-) diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index c3c464248ef..c6a23aa2bdf 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -92,7 +92,7 @@ module Gitlab def oauth_access_token_check(login, password) if login == "oauth2" && password.present? token = Doorkeeper::AccessToken.by_token(password) - if token && token.accessible? + if token && token.accessible? && token_has_scope?(token) user = User.find_by(id: token.resource_owner_id) Gitlab::Auth::Result.new(user, nil, :oauth, read_authentication_abilities) end @@ -101,12 +101,20 @@ module Gitlab def personal_access_token_check(login, password) if login && password - user = User.find_by_personal_access_token(password) + token = PersonalAccessToken.active.find_by_token(password) validation = User.by_login(login) - Gitlab::Auth::Result.new(user, nil, :personal_token, full_authentication_abilities) if user.present? && user == validation + + if token && token.user == validation && token_has_scope?(token) + Gitlab::Auth::Result.new(validation, nil, :personal_token, full_authentication_abilities) + end + end end + def token_has_scope?(token) + AccessTokenValidationService.sufficient_scope?(token, ['api']) + end + def lfs_token_check(login, password) deploy_key_matches = login.match(/\Alfs\+deploy-key-(\d+)\z/) diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index c9d64e99f88..b64413cda12 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -79,14 +79,46 @@ describe Gitlab::Auth, lib: true do expect(gl_auth.find_for_git_client("lfs+deploy-key-#{key.id}", token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(key, nil, :lfs_deploy_token, read_authentication_abilities)) end - it 'recognizes OAuth tokens' do - user = create(:user) - application = Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user) - token = Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id) - ip = 'ip' + context "while using OAuth tokens as passwords" do + it 'succeeds for OAuth tokens with the `api` scope' do + user = create(:user) + application = Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user) + token = Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id, scopes: "api") + ip = 'ip' + + expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: 'oauth2') + expect(gl_auth.find_for_git_client("oauth2", token.token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, nil, :oauth, read_authentication_abilities)) + end + + it 'fails for OAuth tokens with other scopes' do + user = create(:user) + application = Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user) + token = Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id, scopes: "read_user") + ip = 'ip' + + expect(gl_auth).to receive(:rate_limit!).with(ip, success: false, login: 'oauth2') + expect(gl_auth.find_for_git_client("oauth2", token.token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(nil, nil)) + end + end + + context "while using personal access tokens as passwords" do + it 'succeeds for personal access tokens with the `api` scope' do + user = create(:user) + personal_access_token = create(:personal_access_token, user: user, scopes: ['api']) + ip = 'ip' + + expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: user.email) + expect(gl_auth.find_for_git_client(user.email, personal_access_token.token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, nil, :personal_token, full_authentication_abilities)) + end + + it 'fails for personal access tokens with other scopes' do + user = create(:user) + personal_access_token = create(:personal_access_token, user: user, scopes: ['read_user']) + ip = 'ip' - expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: 'oauth2') - expect(gl_auth.find_for_git_client("oauth2", token.token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, nil, :oauth, read_authentication_abilities)) + expect(gl_auth).to receive(:rate_limit!).with(ip, success: false, login: user.email) + expect(gl_auth.find_for_git_client(user.email, personal_access_token.token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(nil, nil)) + end end it 'returns double nil for invalid credentials' do diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb index f1728d61def..d71bb08c218 100644 --- a/spec/requests/git_http_spec.rb +++ b/spec/requests/git_http_spec.rb @@ -230,7 +230,7 @@ describe 'Git HTTP requests', lib: true do context "when an oauth token is provided" do before do application = Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user) - @token = Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id) + @token = Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id, scopes: "api") end it "downloads get status 200" do -- cgit v1.2.1 From ac9835c602f1c9b5a35ef40df079faf1d4b91f7b Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 22 Nov 2016 16:20:21 +0530 Subject: Update CHANGELOG --- changelogs/unreleased/20492-access-token-scopes.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/20492-access-token-scopes.yml diff --git a/changelogs/unreleased/20492-access-token-scopes.yml b/changelogs/unreleased/20492-access-token-scopes.yml new file mode 100644 index 00000000000..a9424ded662 --- /dev/null +++ b/changelogs/unreleased/20492-access-token-scopes.yml @@ -0,0 +1,4 @@ +--- +title: Add scopes for personal access tokens and OAuth tokens +merge_request: 5951 +author: -- cgit v1.2.1 From 4d6da770de94f4bf140507cdf43461b67269ce28 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 24 Nov 2016 13:07:22 +0530 Subject: Implement minor changes from @dbalexandre's review. - Mainly whitespace changes. - Require the migration adding the `scope` column to the `personal_access_tokens` table to have downtime, since API calls will fail if the new code is in place, but the migration hasn't run. - Minor refactoring - load `@scopes` in a `before_action`, since we're doing it in three different places. --- app/controllers/admin/applications_controller.rb | 3 +-- app/controllers/concerns/oauth_applications.rb | 5 ++++ app/controllers/oauth/applications_controller.rb | 6 +---- app/views/doorkeeper/applications/_form.html.haml | 1 - app/views/doorkeeper/applications/show.html.haml | 1 - ..._add_column_scopes_to_personal_access_tokens.rb | 23 +++--------------- ...al_access_tokens_default_back_to_empty_array.rb | 28 +++------------------- lib/api/api_guard.rb | 26 +++++++++++--------- lib/gitlab/auth.rb | 1 - .../access_token_validation_service_spec.rb | 1 - 10 files changed, 28 insertions(+), 67 deletions(-) diff --git a/app/controllers/admin/applications_controller.rb b/app/controllers/admin/applications_controller.rb index 759044910bb..62f62e99a97 100644 --- a/app/controllers/admin/applications_controller.rb +++ b/app/controllers/admin/applications_controller.rb @@ -2,6 +2,7 @@ class Admin::ApplicationsController < Admin::ApplicationController include OauthApplications before_action :set_application, only: [:show, :edit, :update, :destroy] + before_action :load_scopes, only: [:new, :edit] def index @applications = Doorkeeper::Application.where("owner_id IS NULL") @@ -12,11 +13,9 @@ class Admin::ApplicationsController < Admin::ApplicationController def new @application = Doorkeeper::Application.new - @scopes = Doorkeeper.configuration.scopes end def edit - @scopes = Doorkeeper.configuration.scopes end def create diff --git a/app/controllers/concerns/oauth_applications.rb b/app/controllers/concerns/oauth_applications.rb index 34ad43ededd..7210ed3eb32 100644 --- a/app/controllers/concerns/oauth_applications.rb +++ b/app/controllers/concerns/oauth_applications.rb @@ -7,8 +7,13 @@ module OauthApplications def prepare_scopes scopes = params.dig(:doorkeeper_application, :scopes) + if scopes params[:doorkeeper_application][:scopes] = scopes.join(' ') end end + + def load_scopes + @scopes = Doorkeeper.configuration.scopes + end end diff --git a/app/controllers/oauth/applications_controller.rb b/app/controllers/oauth/applications_controller.rb index b5449a6c30e..2ae4785b12c 100644 --- a/app/controllers/oauth/applications_controller.rb +++ b/app/controllers/oauth/applications_controller.rb @@ -7,6 +7,7 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController before_action :verify_user_oauth_applications_enabled before_action :authenticate_user! before_action :add_gon_variables + before_action :load_scopes, only: [:index, :create, :edit] layout 'profile' @@ -14,10 +15,6 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController set_index_vars end - def edit - @scopes = Doorkeeper.configuration.scopes - end - def create @application = Doorkeeper::Application.new(application_params) @@ -45,7 +42,6 @@ class Oauth::ApplicationsController < Doorkeeper::ApplicationsController @authorized_tokens = current_user.oauth_authorized_tokens @authorized_anonymous_tokens = @authorized_tokens.reject(&:application) @authorized_apps = @authorized_tokens.map(&:application).uniq.reject(&:nil?) - @scopes = Doorkeeper.configuration.scopes # Don't overwrite a value possibly set by `create` @application ||= Doorkeeper::Application.new diff --git a/app/views/doorkeeper/applications/_form.html.haml b/app/views/doorkeeper/applications/_form.html.haml index 6fdb04077b6..96677dc1a4d 100644 --- a/app/views/doorkeeper/applications/_form.html.haml +++ b/app/views/doorkeeper/applications/_form.html.haml @@ -25,6 +25,5 @@ = label_tag "doorkeeper_application_scopes_#{scope}", scope %span= "(#{t(scope, scope: [:doorkeeper, :scopes])})" - .prepend-top-default = f.submit 'Save application', class: "btn btn-create" diff --git a/app/views/doorkeeper/applications/show.html.haml b/app/views/doorkeeper/applications/show.html.haml index a18e133c8de..5473a8e0ddc 100644 --- a/app/views/doorkeeper/applications/show.html.haml +++ b/app/views/doorkeeper/applications/show.html.haml @@ -34,7 +34,6 @@ %span.scope-name= scope = "(#{t(scope, scope: [:doorkeeper, :scopes])})" - .form-actions = link_to 'Edit', edit_oauth_application_path(@application), class: 'btn btn-primary wide pull-left' = render 'delete_form', application: @application, submit_btn_css: 'btn btn-danger prepend-left-10' diff --git a/db/migrate/20160823083941_add_column_scopes_to_personal_access_tokens.rb b/db/migrate/20160823083941_add_column_scopes_to_personal_access_tokens.rb index ab7f0365603..91479de840b 100644 --- a/db/migrate/20160823083941_add_column_scopes_to_personal_access_tokens.rb +++ b/db/migrate/20160823083941_add_column_scopes_to_personal_access_tokens.rb @@ -1,32 +1,15 @@ -# See http://doc.gitlab.com/ce/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. +# The default needs to be `[]`, but all existing access tokens need to have `scopes` set to `['api']`. +# It's easier to achieve this by adding the column with the `['api']` default, and then changing the default to +# `[]`. class AddColumnScopesToPersonalAccessTokens < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers - # Set this constant to true if this migration requires downtime. DOWNTIME = false - # When a migration requires downtime you **must** uncomment the following - # constant and define a short and easy to understand explanation as to why the - # migration requires downtime. - # DOWNTIME_REASON = '' - - # When using the methods "add_concurrent_index" or "add_column_with_default" - # you must disable the use of transactions as these methods can not run in an - # existing transaction. When using "add_concurrent_index" make sure that this - # method is the _only_ method called in the migration, any other changes - # should go in a separate migration. This ensures that upon failure _only_ the - # index creation fails and can be retried or reverted easily. - # - # To disable transactions uncomment the following line and remove these - # comments: disable_ddl_transaction! def up - # The default needs to be `[]`, but all existing access tokens need to have `scopes` set to `['api']`. - # It's easier to achieve this by adding the column with the `['api']` default, and then changing the default to - # `[]`. add_column_with_default :personal_access_tokens, :scopes, :string, default: ['api'].to_yaml end diff --git a/db/migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb b/db/migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb index 018cc3d4747..c8ceb116b8a 100644 --- a/db/migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb +++ b/db/migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb @@ -1,39 +1,17 @@ -# See http://doc.gitlab.com/ce/development/migration_style_guide.html -# for more information on how to write migrations for GitLab. +# The default needs to be `[]`, but all existing access tokens need to have `scopes` set to `['api']`. +# It's easier to achieve this by adding the column with the `['api']` default, and then changing the default to +# `[]`. class ChangePersonalAccessTokensDefaultBackToEmptyArray < ActiveRecord::Migration include Gitlab::Database::MigrationHelpers - # Set this constant to true if this migration requires downtime. DOWNTIME = false - # When a migration requires downtime you **must** uncomment the following - # constant and define a short and easy to understand explanation as to why the - # migration requires downtime. - # DOWNTIME_REASON = '' - - # When using the methods "add_concurrent_index" or "add_column_with_default" - # you must disable the use of transactions as these methods can not run in an - # existing transaction. When using "add_concurrent_index" make sure that this - # method is the _only_ method called in the migration, any other changes - # should go in a separate migration. This ensures that upon failure _only_ the - # index creation fails and can be retried or reverted easily. - # - # To disable transactions uncomment the following line and remove these - # comments: - # disable_ddl_transaction! - def up - # The default needs to be `[]`, but all existing access tokens need to have `scopes` set to `['api']`. - # It's easier to achieve this by adding the column with the `['api']` default, and then changing the default to - # `[]`. change_column_default :personal_access_tokens, :scopes, [].to_yaml end def down - # The default needs to be `[]`, but all existing access tokens need to have `scopes` set to `['api']`. - # It's easier to achieve this by adding the column with the `['api']` default, and then changing the default to - # `[]`. change_column_default :personal_access_tokens, :scopes, ['api'].to_yaml end end diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb index cd266669b1e..563224a580f 100644 --- a/lib/api/api_guard.rb +++ b/lib/api/api_guard.rb @@ -44,17 +44,21 @@ module API # Defaults to empty array. # def doorkeeper_guard(scopes: []) - if access_token = find_access_token - case AccessTokenValidationService.validate(access_token, scopes: scopes) - when AccessTokenValidationService::INSUFFICIENT_SCOPE - raise InsufficientScopeError.new(scopes) - when AccessTokenValidationService::EXPIRED - raise ExpiredError - when AccessTokenValidationService::REVOKED - raise RevokedError - when AccessTokenValidationService::VALID - @current_user = User.find(access_token.resource_owner_id) - end + access_token = find_access_token + return nil unless access_token + + case AccessTokenValidationService.validate(access_token, scopes: scopes) + when AccessTokenValidationService::INSUFFICIENT_SCOPE + raise InsufficientScopeError.new(scopes) + + when AccessTokenValidationService::EXPIRED + raise ExpiredError + + when AccessTokenValidationService::REVOKED + raise RevokedError + + when AccessTokenValidationService::VALID + @current_user = User.find(access_token.resource_owner_id) end end diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index c6a23aa2bdf..c425702fd75 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -107,7 +107,6 @@ module Gitlab if token && token.user == validation && token_has_scope?(token) Gitlab::Auth::Result.new(validation, nil, :personal_token, full_authentication_abilities) end - end end diff --git a/spec/services/access_token_validation_service_spec.rb b/spec/services/access_token_validation_service_spec.rb index 8808934fa24..332e745aa36 100644 --- a/spec/services/access_token_validation_service_spec.rb +++ b/spec/services/access_token_validation_service_spec.rb @@ -1,7 +1,6 @@ require 'spec_helper' describe AccessTokenValidationService, services: true do - describe ".sufficient_scope?" do it "returns true if the required scope is present in the token's scopes" do token = double("token", scopes: [:api, :read_user]) -- cgit v1.2.1 From 990ae6b8e5f2797a6c168f9c16a725a159570058 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 24 Nov 2016 14:22:03 +0530 Subject: Move the scopes form/list view into a partial. - The list of scopes that's displayed while creating a personal access token is identical to the list that's displayed while creating an OAuth application. Extract these into a partial. - The list of scopes that's displayed while in the show page for an OAuth token in the profile settings and admin settings are identical. Extract these into a partial. --- app/views/admin/applications/show.html.haml | 11 +---------- app/views/doorkeeper/applications/_form.html.haml | 6 +----- app/views/doorkeeper/applications/show.html.haml | 11 +---------- app/views/profiles/personal_access_tokens/_form.html.haml | 6 +----- app/views/shared/tokens/_scopes_form.html.haml | 5 +++++ app/views/shared/tokens/_scopes_list.html.haml | 10 ++++++++++ 6 files changed, 19 insertions(+), 30 deletions(-) create mode 100644 app/views/shared/tokens/_scopes_form.html.haml create mode 100644 app/views/shared/tokens/_scopes_list.html.haml diff --git a/app/views/admin/applications/show.html.haml b/app/views/admin/applications/show.html.haml index 3418dc96496..6e7b7003ac5 100644 --- a/app/views/admin/applications/show.html.haml +++ b/app/views/admin/applications/show.html.haml @@ -23,16 +23,7 @@ %div %span.monospace= uri - - if @application.scopes.present? - %tr - %td - Scopes - %td - %ul.scopes-list.append-bottom-0 - - @application.scopes.each do |scope| - %li - %span.scope-name= scope - = "(#{t(scope, scope: [:doorkeeper, :scopes])})" + = render partial: "shared/tokens/scopes_list" .form-actions = link_to 'Edit', edit_admin_application_path(@application), class: 'btn btn-primary wide pull-left' diff --git a/app/views/doorkeeper/applications/_form.html.haml b/app/views/doorkeeper/applications/_form.html.haml index 96677dc1a4d..a6ad0bb8d1b 100644 --- a/app/views/doorkeeper/applications/_form.html.haml +++ b/app/views/doorkeeper/applications/_form.html.haml @@ -19,11 +19,7 @@ .form-group = f.label :scopes, class: 'label-light' - - @scopes.each do |scope| - %fieldset - = check_box_tag 'doorkeeper_application[scopes][]', scope, application.scopes.include?(scope), id: "doorkeeper_application_scopes_#{scope}" - = label_tag "doorkeeper_application_scopes_#{scope}", scope - %span= "(#{t(scope, scope: [:doorkeeper, :scopes])})" + = render partial: 'shared/tokens/scopes_form', locals: { prefix: 'doorkeeper_application', token: application } .prepend-top-default = f.submit 'Save application', class: "btn btn-create" diff --git a/app/views/doorkeeper/applications/show.html.haml b/app/views/doorkeeper/applications/show.html.haml index 5473a8e0ddc..e528cb825f5 100644 --- a/app/views/doorkeeper/applications/show.html.haml +++ b/app/views/doorkeeper/applications/show.html.haml @@ -23,16 +23,7 @@ %div %span.monospace= uri - - if @application.scopes.present? - %tr - %td - Scopes - %td - %ul.scopes-list.append-bottom-0 - - @application.scopes.each do |scope| - %li - %span.scope-name= scope - = "(#{t(scope, scope: [:doorkeeper, :scopes])})" + = render partial: "shared/tokens/scopes_list" .form-actions = link_to 'Edit', edit_oauth_application_path(@application), class: 'btn btn-primary wide pull-left' diff --git a/app/views/profiles/personal_access_tokens/_form.html.haml b/app/views/profiles/personal_access_tokens/_form.html.haml index 6083fdaa31d..5651b242129 100644 --- a/app/views/profiles/personal_access_tokens/_form.html.haml +++ b/app/views/profiles/personal_access_tokens/_form.html.haml @@ -12,11 +12,7 @@ .form-group = f.label :scopes, class: 'label-light' - - @scopes.each do |scope| - %fieldset - = check_box_tag 'personal_access_token[scopes][]', scope, @personal_access_token.scopes.include?(scope), id: "personal_access_token_scopes_#{scope}" - = label_tag "personal_access_token_scopes_#{scope}", scope - %span= "(#{t(scope, scope: [:doorkeeper, :scopes])})" + = render partial: 'shared/tokens/scopes_form', locals: { prefix: 'personal_access_token', token: @personal_access_token } .prepend-top-default = f.submit 'Create Personal Access Token', class: "btn btn-create" diff --git a/app/views/shared/tokens/_scopes_form.html.haml b/app/views/shared/tokens/_scopes_form.html.haml new file mode 100644 index 00000000000..5dbbd9e4808 --- /dev/null +++ b/app/views/shared/tokens/_scopes_form.html.haml @@ -0,0 +1,5 @@ +- @scopes.each do |scope| + %fieldset + = check_box_tag "#{prefix}[scopes][]", scope, token.scopes.include?(scope), id: "#{prefix}_scopes_#{scope}" + = label_tag "#{prefix}_scopes_#{scope}", scope + %span= "(#{t(scope, scope: [:doorkeeper, :scopes])})" diff --git a/app/views/shared/tokens/_scopes_list.html.haml b/app/views/shared/tokens/_scopes_list.html.haml new file mode 100644 index 00000000000..9e3b562f0f5 --- /dev/null +++ b/app/views/shared/tokens/_scopes_list.html.haml @@ -0,0 +1,10 @@ +- if @application.scopes.present? + %tr + %td + Scopes + %td + %ul.scopes-list.append-bottom-0 + - @application.scopes.each do |scope| + %li + %span.scope-name= scope + = "(#{t(scope, scope: [:doorkeeper, :scopes])})" -- cgit v1.2.1 From dc95bcbb165289d9754e6bf66288c8d4350f6e57 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 24 Nov 2016 14:39:12 +0530 Subject: Refactor access token validation in `Gitlab::Auth` - Based on @dbalexandre's review - Extract token validity conditions into two separate methods, for personal access tokens and OAuth tokens. --- lib/gitlab/auth.rb | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index c425702fd75..c21afaa1551 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -92,7 +92,7 @@ module Gitlab def oauth_access_token_check(login, password) if login == "oauth2" && password.present? token = Doorkeeper::AccessToken.by_token(password) - if token && token.accessible? && token_has_scope?(token) + if valid_oauth_token?(token) user = User.find_by(id: token.resource_owner_id) Gitlab::Auth::Result.new(user, nil, :oauth, read_authentication_abilities) end @@ -104,12 +104,20 @@ module Gitlab token = PersonalAccessToken.active.find_by_token(password) validation = User.by_login(login) - if token && token.user == validation && token_has_scope?(token) + if valid_personal_access_token?(token, validation) Gitlab::Auth::Result.new(validation, nil, :personal_token, full_authentication_abilities) end end end + def valid_oauth_token?(token) + token && token.accessible? && token_has_scope?(token) + end + + def valid_personal_access_token?(token, user) + token && token.user == user && token_has_scope?(token) + end + def token_has_scope?(token) AccessTokenValidationService.sufficient_scope?(token, ['api']) end -- cgit v1.2.1 From fc7a5a3806c7c7317731ea305715fe0573b90d88 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 28 Nov 2016 12:30:41 +0530 Subject: Modify `ApiHelpers` spec to adhere to the Four-Phase test style. - Use whitespace to separate the setup, expectation and teardown phases. --- spec/requests/api/helpers_spec.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb index 15b93118ee4..c3d7ac3eef8 100644 --- a/spec/requests/api/helpers_spec.rb +++ b/spec/requests/api/helpers_spec.rb @@ -97,20 +97,26 @@ describe API::Helpers, api: true do it "returns nil for an invalid token" do env[API::APIGuard::PRIVATE_TOKEN_HEADER] = 'invalid token' allow_any_instance_of(self.class).to receive(:doorkeeper_guard){ false } + expect(current_user).to be_nil end it "returns nil for a user without access" do env[API::APIGuard::PRIVATE_TOKEN_HEADER] = user.private_token allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false) + expect(current_user).to be_nil end it "leaves user as is when sudo not specified" do env[API::APIGuard::PRIVATE_TOKEN_HEADER] = user.private_token + expect(current_user).to eq(user) + clear_env + params[API::APIGuard::PRIVATE_TOKEN_PARAM] = user.private_token + expect(current_user).to eq(user) end end @@ -124,12 +130,14 @@ describe API::Helpers, api: true do it "returns nil for an invalid token" do env[API::APIGuard::PRIVATE_TOKEN_HEADER] = 'invalid token' + expect(current_user).to be_nil end it "returns nil for a user without access" do env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token allow_any_instance_of(Gitlab::UserAccess).to receive(:allowed?).and_return(false) + expect(current_user).to be_nil end @@ -137,6 +145,7 @@ describe API::Helpers, api: true do personal_access_token = create(:personal_access_token, user: user, scopes: ['read_user']) env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token allow_access_with_scope('write_user') + expect(current_user).to be_nil end @@ -145,18 +154,21 @@ describe API::Helpers, api: true do expect(current_user).to eq(user) clear_env params[API::APIGuard::PRIVATE_TOKEN_PARAM] = personal_access_token.token + expect(current_user).to eq(user) end it 'does not allow revoked tokens' do personal_access_token.revoke! env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token + expect(current_user).to be_nil end it 'does not allow expired tokens' do personal_access_token.update_attributes!(expires_at: 1.day.ago) env[API::APIGuard::PRIVATE_TOKEN_HEADER] = personal_access_token.token + expect(current_user).to be_nil end end -- cgit v1.2.1 From f14d423dc7c9ec2d97c83f0c8893661922df4360 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 28 Nov 2016 13:13:53 +0530 Subject: Add a controller spec for personal access tokens. Split the existing feature spec into both feature and controller specs. Feature specs assert on browser DOM, and controller specs assert on database state. --- .../profiles/personal_access_tokens_spec.rb | 49 +++++++++++++++++++++ .../profiles/personal_access_tokens_spec.rb | 51 +++++----------------- 2 files changed, 60 insertions(+), 40 deletions(-) create mode 100644 spec/controllers/profiles/personal_access_tokens_spec.rb diff --git a/spec/controllers/profiles/personal_access_tokens_spec.rb b/spec/controllers/profiles/personal_access_tokens_spec.rb new file mode 100644 index 00000000000..45534a3a587 --- /dev/null +++ b/spec/controllers/profiles/personal_access_tokens_spec.rb @@ -0,0 +1,49 @@ +require 'spec_helper' + +describe Profiles::PersonalAccessTokensController do + let(:user) { create(:user) } + + describe '#create' do + def created_token + PersonalAccessToken.order(:created_at).last + end + + before { sign_in(user) } + + it "allows creation of a token" do + name = FFaker::Product.brand + + post :create, personal_access_token: { name: name } + + expect(created_token).not_to be_nil + expect(created_token.name).to eq(name) + expect(created_token.expires_at).to be_nil + expect(PersonalAccessToken.active).to include(created_token) + end + + it "allows creation of a token with an expiry date" do + expires_at = 5.days.from_now + + post :create, personal_access_token: { name: FFaker::Product.brand, expires_at: expires_at } + + expect(created_token).not_to be_nil + expect(created_token.expires_at.to_i).to eq(expires_at.to_i) + end + + context "scopes" do + it "allows creation of a token with scopes" do + post :create, personal_access_token: { name: FFaker::Product.brand, scopes: ['api', 'read_user'] } + + expect(created_token).not_to be_nil + expect(created_token.scopes).to eq(['api', 'read_user']) + end + + it "allows creation of a token with no scopes" do + post :create, personal_access_token: { name: FFaker::Product.brand, scopes: [] } + + expect(created_token).not_to be_nil + expect(created_token.scopes).to eq([]) + end + end + end +end diff --git a/spec/features/profiles/personal_access_tokens_spec.rb b/spec/features/profiles/personal_access_tokens_spec.rb index 0ffeeff0921..55a01057c83 100644 --- a/spec/features/profiles/personal_access_tokens_spec.rb +++ b/spec/features/profiles/personal_access_tokens_spec.rb @@ -27,54 +27,25 @@ describe 'Profile > Personal Access Tokens', feature: true, js: true do describe "token creation" do it "allows creation of a token" do - visit profile_personal_access_tokens_path - fill_in "Name", with: FFaker::Product.brand - - expect {click_on "Create Personal Access Token"}.to change { PersonalAccessToken.count }.by(1) - expect(created_personal_access_token).to eq(PersonalAccessToken.last.token) - expect(active_personal_access_tokens).to have_text(PersonalAccessToken.last.name) - expect(active_personal_access_tokens).to have_text("Never") - end + name = FFaker::Product.brand - it "allows creation of a token with an expiry date" do visit profile_personal_access_tokens_path - fill_in "Name", with: FFaker::Product.brand + fill_in "Name", with: name # Set date to 1st of next month find_field("Expires at").trigger('focus') find("a[title='Next']").click click_on "1" - expect {click_on "Create Personal Access Token"}.to change { PersonalAccessToken.count }.by(1) - expect(created_personal_access_token).to eq(PersonalAccessToken.last.token) - expect(active_personal_access_tokens).to have_text(PersonalAccessToken.last.name) - expect(active_personal_access_tokens).to have_text(Date.today.next_month.at_beginning_of_month.to_s(:medium)) - end - - context "scopes" do - it "allows creation of a token with scopes" do - visit profile_personal_access_tokens_path - fill_in "Name", with: FFaker::Product.brand - - check "api" - check "read_user" - - expect {click_on "Create Personal Access Token"}.to change { PersonalAccessToken.count }.by(1) - expect(created_personal_access_token).to eq(PersonalAccessToken.last.token) - expect(PersonalAccessToken.last.scopes).to match_array(['api', 'read_user']) - expect(active_personal_access_tokens).to have_text('api') - expect(active_personal_access_tokens).to have_text('read_user') - end - - it "allows creation of a token with no scopes" do - visit profile_personal_access_tokens_path - fill_in "Name", with: FFaker::Product.brand + # Scopes + check "api" + check "read_user" - expect {click_on "Create Personal Access Token"}.to change { PersonalAccessToken.count }.by(1) - expect(created_personal_access_token).to eq(PersonalAccessToken.last.token) - expect(PersonalAccessToken.last.scopes).to eq([]) - expect(active_personal_access_tokens).to have_text('no scopes') - end + click_on "Create Personal Access Token" + expect(active_personal_access_tokens).to have_text(name) + expect(active_personal_access_tokens).to have_text(Date.today.next_month.at_beginning_of_month.to_s(:medium)) + expect(active_personal_access_tokens).to have_text('api') + expect(active_personal_access_tokens).to have_text('read_user') end context "when creation fails" do @@ -111,7 +82,7 @@ describe 'Profile > Personal Access Tokens', feature: true, js: true do disallow_personal_access_token_saves! visit profile_personal_access_tokens_path - expect { click_on "Revoke" }.not_to change { PersonalAccessToken.inactive.count } + click_on "Revoke" expect(active_personal_access_tokens).to have_text(personal_access_token.name) expect(page).to have_content("Could not revoke") end -- cgit v1.2.1 From f706a973c26f9de9a1f1599d532b33e9e66a80bb Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 5 Dec 2016 09:49:51 +0530 Subject: View-related (and other minor) changes to !5951 based on @rymai's review. - The `scopes_form` partial can be used in the `admin/applications` view as well - Don't allow partials to access instance variables directly. Instead, pass in the instance variables as local variables, and use `local_assigns.fetch` to assert that the variables are passed in as expected. - Change a few instances of `render :partial` to `render` - Remove an instance of `required: false` in a view, since this is the default - Inline many instances of a local variable (`ip = 'ip'`) in `auth_spec` --- app/views/admin/applications/_form.html.haml | 6 +-- app/views/admin/applications/show.html.haml | 2 +- app/views/doorkeeper/applications/_form.html.haml | 2 +- app/views/doorkeeper/applications/show.html.haml | 2 +- .../personal_access_tokens/_form.html.haml | 11 ++++-- .../personal_access_tokens/index.html.haml | 2 +- app/views/shared/tokens/_scopes_form.html.haml | 6 ++- app/views/shared/tokens/_scopes_list.html.haml | 23 ++++++----- spec/lib/gitlab/auth_spec.rb | 46 +++++++++------------- 9 files changed, 48 insertions(+), 52 deletions(-) diff --git a/app/views/admin/applications/_form.html.haml b/app/views/admin/applications/_form.html.haml index 36d2f415a05..c689b26d6e6 100644 --- a/app/views/admin/applications/_form.html.haml +++ b/app/views/admin/applications/_form.html.haml @@ -22,11 +22,7 @@ .form-group = f.label :scopes, class: 'col-sm-2 control-label' .col-sm-10 - - @scopes.each do |scope| - %fieldset - = check_box_tag 'doorkeeper_application[scopes][]', scope, application.scopes.include?(scope), id: "doorkeeper_application_scopes_#{scope}" - = label_tag "doorkeeper_application_scopes_#{scope}", scope - %span= "(#{t(scope, scope: [:doorkeeper, :scopes])})" + = render 'shared/tokens/scopes_form', prefix: 'doorkeeper_application', token: application, scopes: @scopes .form-actions = f.submit 'Submit', class: "btn btn-save wide" diff --git a/app/views/admin/applications/show.html.haml b/app/views/admin/applications/show.html.haml index 6e7b7003ac5..14683cc66e9 100644 --- a/app/views/admin/applications/show.html.haml +++ b/app/views/admin/applications/show.html.haml @@ -23,7 +23,7 @@ %div %span.monospace= uri - = render partial: "shared/tokens/scopes_list" + = render "shared/tokens/scopes_list", token: @application .form-actions = link_to 'Edit', edit_admin_application_path(@application), class: 'btn btn-primary wide pull-left' diff --git a/app/views/doorkeeper/applications/_form.html.haml b/app/views/doorkeeper/applications/_form.html.haml index a6ad0bb8d1b..b3313c7c985 100644 --- a/app/views/doorkeeper/applications/_form.html.haml +++ b/app/views/doorkeeper/applications/_form.html.haml @@ -19,7 +19,7 @@ .form-group = f.label :scopes, class: 'label-light' - = render partial: 'shared/tokens/scopes_form', locals: { prefix: 'doorkeeper_application', token: application } + = render 'shared/tokens/scopes_form', prefix: 'doorkeeper_application', token: application, scopes: @scopes .prepend-top-default = f.submit 'Save application', class: "btn btn-create" diff --git a/app/views/doorkeeper/applications/show.html.haml b/app/views/doorkeeper/applications/show.html.haml index e528cb825f5..559de63d96d 100644 --- a/app/views/doorkeeper/applications/show.html.haml +++ b/app/views/doorkeeper/applications/show.html.haml @@ -23,7 +23,7 @@ %div %span.monospace= uri - = render partial: "shared/tokens/scopes_list" + = render "shared/tokens/scopes_list", token: @application .form-actions = link_to 'Edit', edit_oauth_application_path(@application), class: 'btn btn-primary wide pull-left' diff --git a/app/views/profiles/personal_access_tokens/_form.html.haml b/app/views/profiles/personal_access_tokens/_form.html.haml index 5651b242129..3f6efa33953 100644 --- a/app/views/profiles/personal_access_tokens/_form.html.haml +++ b/app/views/profiles/personal_access_tokens/_form.html.haml @@ -1,6 +1,9 @@ -= form_for [:profile, @personal_access_token], method: :post, html: { class: 'js-requires-input' } do |f| +- personal_access_token = local_assigns.fetch(:personal_access_token) +- scopes = local_assigns.fetch(:scopes) - = form_errors(@personal_access_token) += form_for [:profile, personal_access_token], method: :post, html: { class: 'js-requires-input' } do |f| + + = form_errors(personal_access_token) .form-group = f.label :name, class: 'label-light' @@ -8,11 +11,11 @@ .form-group = f.label :expires_at, class: 'label-light' - = f.text_field :expires_at, class: "datepicker form-control", required: false + = f.text_field :expires_at, class: "datepicker form-control" .form-group = f.label :scopes, class: 'label-light' - = render partial: 'shared/tokens/scopes_form', locals: { prefix: 'personal_access_token', token: @personal_access_token } + = render 'shared/tokens/scopes_form', prefix: 'personal_access_token', token: personal_access_token, scopes: scopes .prepend-top-default = f.submit 'Create Personal Access Token', class: "btn btn-create" diff --git a/app/views/profiles/personal_access_tokens/index.html.haml b/app/views/profiles/personal_access_tokens/index.html.haml index 39eef0f6baf..bb4effeeeb1 100644 --- a/app/views/profiles/personal_access_tokens/index.html.haml +++ b/app/views/profiles/personal_access_tokens/index.html.haml @@ -29,7 +29,7 @@ %p.profile-settings-content Pick a name for the application, and we'll give you a unique token. - = render "form" + = render "form", personal_access_token: @personal_access_token, scopes: @scopes %hr diff --git a/app/views/shared/tokens/_scopes_form.html.haml b/app/views/shared/tokens/_scopes_form.html.haml index 5dbbd9e4808..5074afb63a1 100644 --- a/app/views/shared/tokens/_scopes_form.html.haml +++ b/app/views/shared/tokens/_scopes_form.html.haml @@ -1,4 +1,8 @@ -- @scopes.each do |scope| +- scopes = local_assigns.fetch(:scopes) +- prefix = local_assigns.fetch(:prefix) +- token = local_assigns.fetch(:token) + +- scopes.each do |scope| %fieldset = check_box_tag "#{prefix}[scopes][]", scope, token.scopes.include?(scope), id: "#{prefix}_scopes_#{scope}" = label_tag "#{prefix}_scopes_#{scope}", scope diff --git a/app/views/shared/tokens/_scopes_list.html.haml b/app/views/shared/tokens/_scopes_list.html.haml index 9e3b562f0f5..f99e905e95c 100644 --- a/app/views/shared/tokens/_scopes_list.html.haml +++ b/app/views/shared/tokens/_scopes_list.html.haml @@ -1,10 +1,13 @@ -- if @application.scopes.present? - %tr - %td - Scopes - %td - %ul.scopes-list.append-bottom-0 - - @application.scopes.each do |scope| - %li - %span.scope-name= scope - = "(#{t(scope, scope: [:doorkeeper, :scopes])})" +- token = local_assigns.fetch(:token) + +- return unless token.scopes.present? + +%tr + %td + Scopes + %td + %ul.scopes-list.append-bottom-0 + - token.scopes.each do |scope| + %li + %span.scope-name= scope + = "(#{t(scope, scope: [:doorkeeper, :scopes])})" diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index b64413cda12..f251c0dd25a 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -47,36 +47,31 @@ describe Gitlab::Auth, lib: true do project.create_drone_ci_service(active: true) project.drone_ci_service.update(token: 'token') - ip = 'ip' - - expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: 'drone-ci-token') - expect(gl_auth.find_for_git_client('drone-ci-token', 'token', project: project, ip: ip)).to eq(Gitlab::Auth::Result.new(nil, project, :ci, build_authentication_abilities)) + expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: 'drone-ci-token') + expect(gl_auth.find_for_git_client('drone-ci-token', 'token', project: project, ip: 'ip')).to eq(Gitlab::Auth::Result.new(nil, project, :ci, build_authentication_abilities)) end it 'recognizes master passwords' do user = create(:user, password: 'password') - ip = 'ip' - expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: user.username) - expect(gl_auth.find_for_git_client(user.username, 'password', project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, nil, :gitlab_or_ldap, full_authentication_abilities)) + expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: user.username) + expect(gl_auth.find_for_git_client(user.username, 'password', project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(user, nil, :gitlab_or_ldap, full_authentication_abilities)) end it 'recognizes user lfs tokens' do user = create(:user) - ip = 'ip' token = Gitlab::LfsToken.new(user).token - expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: user.username) - expect(gl_auth.find_for_git_client(user.username, token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, nil, :lfs_token, full_authentication_abilities)) + expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: user.username) + expect(gl_auth.find_for_git_client(user.username, token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(user, nil, :lfs_token, full_authentication_abilities)) end it 'recognizes deploy key lfs tokens' do key = create(:deploy_key) - ip = 'ip' token = Gitlab::LfsToken.new(key).token - expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: "lfs+deploy-key-#{key.id}") - expect(gl_auth.find_for_git_client("lfs+deploy-key-#{key.id}", token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(key, nil, :lfs_deploy_token, read_authentication_abilities)) + expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: "lfs+deploy-key-#{key.id}") + expect(gl_auth.find_for_git_client("lfs+deploy-key-#{key.id}", token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(key, nil, :lfs_deploy_token, read_authentication_abilities)) end context "while using OAuth tokens as passwords" do @@ -84,20 +79,18 @@ describe Gitlab::Auth, lib: true do user = create(:user) application = Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user) token = Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id, scopes: "api") - ip = 'ip' - expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: 'oauth2') - expect(gl_auth.find_for_git_client("oauth2", token.token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, nil, :oauth, read_authentication_abilities)) + expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: 'oauth2') + expect(gl_auth.find_for_git_client("oauth2", token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(user, nil, :oauth, read_authentication_abilities)) end it 'fails for OAuth tokens with other scopes' do user = create(:user) application = Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user) token = Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id, scopes: "read_user") - ip = 'ip' - expect(gl_auth).to receive(:rate_limit!).with(ip, success: false, login: 'oauth2') - expect(gl_auth.find_for_git_client("oauth2", token.token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(nil, nil)) + expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: 'oauth2') + expect(gl_auth.find_for_git_client("oauth2", token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(nil, nil)) end end @@ -105,28 +98,25 @@ describe Gitlab::Auth, lib: true do it 'succeeds for personal access tokens with the `api` scope' do user = create(:user) personal_access_token = create(:personal_access_token, user: user, scopes: ['api']) - ip = 'ip' - expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: user.email) - expect(gl_auth.find_for_git_client(user.email, personal_access_token.token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, nil, :personal_token, full_authentication_abilities)) + expect(gl_auth).to receive(:rate_limit!).with('ip', success: true, login: user.email) + expect(gl_auth.find_for_git_client(user.email, personal_access_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(user, nil, :personal_token, full_authentication_abilities)) end it 'fails for personal access tokens with other scopes' do user = create(:user) personal_access_token = create(:personal_access_token, user: user, scopes: ['read_user']) - ip = 'ip' - expect(gl_auth).to receive(:rate_limit!).with(ip, success: false, login: user.email) - expect(gl_auth.find_for_git_client(user.email, personal_access_token.token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(nil, nil)) + expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: user.email) + expect(gl_auth.find_for_git_client(user.email, personal_access_token.token, project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new(nil, nil)) end end it 'returns double nil for invalid credentials' do login = 'foo' - ip = 'ip' - expect(gl_auth).to receive(:rate_limit!).with(ip, success: false, login: login) - expect(gl_auth.find_for_git_client(login, 'bar', project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new) + expect(gl_auth).to receive(:rate_limit!).with('ip', success: false, login: login) + expect(gl_auth.find_for_git_client(login, 'bar', project: nil, ip: 'ip')).to eq(Gitlab::Auth::Result.new) end end -- cgit v1.2.1 From b303948ff549ce57d3b6985c2c366dfcdc5a2ca3 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 5 Dec 2016 22:55:53 +0530 Subject: Convert AccessTokenValidationService into a class. - Previously, AccessTokenValidationService was a module, and all its public methods accepted a token. It makes sense to convert it to a class which accepts a token during initialization. - Also rename the `sufficient_scope?` method to `include_any_scope?` - Based on feedback from @rymai --- app/services/access_token_validation_service.rb | 38 ++++++++++------------ lib/api/api_guard.rb | 4 +-- lib/gitlab/auth.rb | 2 +- .../access_token_validation_service_spec.rb | 14 ++++---- 4 files changed, 28 insertions(+), 30 deletions(-) diff --git a/app/services/access_token_validation_service.rb b/app/services/access_token_validation_service.rb index 69449f3a445..ddaaed90e5b 100644 --- a/app/services/access_token_validation_service.rb +++ b/app/services/access_token_validation_service.rb @@ -1,34 +1,32 @@ -module AccessTokenValidationService +AccessTokenValidationService = Struct.new(:token) do # Results: VALID = :valid EXPIRED = :expired REVOKED = :revoked INSUFFICIENT_SCOPE = :insufficient_scope - class << self - def validate(token, scopes: []) - if token.expired? - return EXPIRED + def validate(scopes: []) + if token.expired? + return EXPIRED - elsif token.revoked? - return REVOKED + elsif token.revoked? + return REVOKED - elsif !self.sufficient_scope?(token, scopes) - return INSUFFICIENT_SCOPE + elsif !self.include_any_scope?(scopes) + return INSUFFICIENT_SCOPE - else - return VALID - end + else + return VALID end + end - # True if the token's scope contains any of the required scopes. - def sufficient_scope?(token, required_scopes) - if required_scopes.blank? - true - else - # Check whether the token is allowed access to any of the required scopes. - Set.new(required_scopes).intersection(Set.new(token.scopes)).present? - end + # True if the token's scope contains any of the passed scopes. + def include_any_scope?(scopes) + if scopes.blank? + true + else + # Check whether the token is allowed access to any of the required scopes. + Set.new(scopes).intersection(Set.new(token.scopes)).present? end end end diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb index 563224a580f..df6db140d0e 100644 --- a/lib/api/api_guard.rb +++ b/lib/api/api_guard.rb @@ -47,7 +47,7 @@ module API access_token = find_access_token return nil unless access_token - case AccessTokenValidationService.validate(access_token, scopes: scopes) + case AccessTokenValidationService.new(access_token).validate(scopes: scopes) when AccessTokenValidationService::INSUFFICIENT_SCOPE raise InsufficientScopeError.new(scopes) @@ -96,7 +96,7 @@ module API access_token = PersonalAccessToken.active.find_by_token(token_string) return unless access_token - if AccessTokenValidationService.sufficient_scope?(access_token, scopes) + if AccessTokenValidationService.new(access_token).include_any_scope?(scopes) User.find(access_token.user_id) end end diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index c21afaa1551..2879a4d2f5d 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -119,7 +119,7 @@ module Gitlab end def token_has_scope?(token) - AccessTokenValidationService.sufficient_scope?(token, ['api']) + AccessTokenValidationService.new(token).include_any_scope?(['api']) end def lfs_token_check(login, password) diff --git a/spec/services/access_token_validation_service_spec.rb b/spec/services/access_token_validation_service_spec.rb index 332e745aa36..87f093ee8ce 100644 --- a/spec/services/access_token_validation_service_spec.rb +++ b/spec/services/access_token_validation_service_spec.rb @@ -1,41 +1,41 @@ require 'spec_helper' describe AccessTokenValidationService, services: true do - describe ".sufficient_scope?" do + describe ".include_any_scope?" do it "returns true if the required scope is present in the token's scopes" do token = double("token", scopes: [:api, :read_user]) - expect(described_class.sufficient_scope?(token, [:api])).to be(true) + expect(described_class.new(token).include_any_scope?([:api])).to be(true) end it "returns true if more than one of the required scopes is present in the token's scopes" do token = double("token", scopes: [:api, :read_user, :other_scope]) - expect(described_class.sufficient_scope?(token, [:api, :other_scope])).to be(true) + expect(described_class.new(token).include_any_scope?([:api, :other_scope])).to be(true) end it "returns true if the list of required scopes is an exact match for the token's scopes" do token = double("token", scopes: [:api, :read_user, :other_scope]) - expect(described_class.sufficient_scope?(token, [:api, :read_user, :other_scope])).to be(true) + expect(described_class.new(token).include_any_scope?([:api, :read_user, :other_scope])).to be(true) end it "returns true if the list of required scopes contains all of the token's scopes, in addition to others" do token = double("token", scopes: [:api, :read_user]) - expect(described_class.sufficient_scope?(token, [:api, :read_user, :other_scope])).to be(true) + expect(described_class.new(token).include_any_scope?([:api, :read_user, :other_scope])).to be(true) end it 'returns true if the list of required scopes is blank' do token = double("token", scopes: []) - expect(described_class.sufficient_scope?(token, [])).to be(true) + expect(described_class.new(token).include_any_scope?([])).to be(true) end it "returns false if there are no scopes in common between the required scopes and the token scopes" do token = double("token", scopes: [:api, :read_user]) - expect(described_class.sufficient_scope?(token, [:other_scope])).to be(false) + expect(described_class.new(token).include_any_scope?([:other_scope])).to be(false) end end end -- cgit v1.2.1 From 5becbe2495850923604c71b4c807666ea94819b3 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 5 Dec 2016 22:58:19 +0530 Subject: Rename the `token_has_scope?` method. `valid_api_token?` is a better name. Scopes are just (potentially) one facet of a "valid" token. --- lib/gitlab/auth.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 2879a4d2f5d..8dda65c71ef 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -111,14 +111,14 @@ module Gitlab end def valid_oauth_token?(token) - token && token.accessible? && token_has_scope?(token) + token && token.accessible? && valid_api_token?(token) end def valid_personal_access_token?(token, user) - token && token.user == user && token_has_scope?(token) + token && token.user == user && valid_api_token?(token) end - def token_has_scope?(token) + def valid_api_token?(token) AccessTokenValidationService.new(token).include_any_scope?(['api']) end -- cgit v1.2.1 From eb434b15ebbc7d0b7ed79bb2daa45601e3c918ca Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Fri, 16 Dec 2016 14:57:09 +0530 Subject: Make `ChangePersonalAccessTokensDefaultBackToEmptyArray` a "post" migration. If we leave this as a regular migration, we could have the following flow: 1. Application knows nothing about scopes. 2. First migration runs, all existing personal access tokens have `api` scope 3. Application still knows nothing about scopes. 4. Second migration runs, all tokens created after this point have no scope 5. Application still knows nothing about scopes. 6. Tokens created at this time _should have the API scope, but instead have no scope_ 7. Application code is reloaded, application knows about scopes 8. Tokens created after this point only have no scope if the user deliberately chooses to have no scopes. Point #6 is the problem here. To avoid this, we move the second migration to a "post" migration, which runs after the application code is deployed/reloaded. --- ...sonal_access_tokens_default_back_to_empty_array.rb | 17 ----------------- ...sonal_access_tokens_default_back_to_empty_array.rb | 19 +++++++++++++++++++ 2 files changed, 19 insertions(+), 17 deletions(-) delete mode 100644 db/migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb create mode 100644 db/post_migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb diff --git a/db/migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb b/db/migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb deleted file mode 100644 index c8ceb116b8a..00000000000 --- a/db/migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb +++ /dev/null @@ -1,17 +0,0 @@ -# The default needs to be `[]`, but all existing access tokens need to have `scopes` set to `['api']`. -# It's easier to achieve this by adding the column with the `['api']` default, and then changing the default to -# `[]`. - -class ChangePersonalAccessTokensDefaultBackToEmptyArray < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - DOWNTIME = false - - def up - change_column_default :personal_access_tokens, :scopes, [].to_yaml - end - - def down - change_column_default :personal_access_tokens, :scopes, ['api'].to_yaml - end -end diff --git a/db/post_migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb b/db/post_migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb new file mode 100644 index 00000000000..7df561d82dd --- /dev/null +++ b/db/post_migrate/20160824121037_change_personal_access_tokens_default_back_to_empty_array.rb @@ -0,0 +1,19 @@ +# The default needs to be `[]`, but all existing access tokens need to have `scopes` set to `['api']`. +# It's easier to achieve this by adding the column with the `['api']` default (regular migration), and +# then changing the default to `[]` (in this post-migration). +# +# Details: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5951#note_19721973 + +class ChangePersonalAccessTokensDefaultBackToEmptyArray < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + def up + change_column_default :personal_access_tokens, :scopes, [].to_yaml + end + + def down + change_column_default :personal_access_tokens, :scopes, ['api'].to_yaml + end +end -- cgit v1.2.1