refactors documentation and personal access tokens form to not allow admins to...

refactors documentation and personal access tokens form to not allow admins to generate non impersionation tokens

app/controllers/admin/personal_access_tokens_controller.rb

@@ -6,7 +6,8 @@ class Admin::PersonalAccessTokensController < Admin::ApplicationController
   end
 
   def create
-    @personal_access_token = user.personal_access_tokens.generate(personal_access_token_params)
+    # We never want to non-impersonate a user
+    @personal_access_token = user.personal_access_tokens.generate(personal_access_token_params.merge(impersonation: true))
 
     if @personal_access_token.save
       flash[:personal_access_token] = @personal_access_token.token

app/views/admin/personal_access_tokens/_form.html.haml

-- personal_access_token = local_assigns.fetch(:personal_access_token)
-- scopes = local_assigns.fetch(:scopes)
-
-= form_for [:admin_user, 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"
-
-  .form-group
-    = f.label :scopes, class: 'label-light'
-    = render 'shared/tokens/scopes_form', prefix: 'personal_access_token', token: personal_access_token, scopes: scopes
-
-  .form-group
-    = f.label :impersonation, class: 'label-light'
-    %fieldset
-      = f.check_box :impersonation
-      = f.label 'impersonation', 'You can impersonate the user'
-      %span= "(Normal users will not see this type of token)"
-
-  .prepend-top-default
-    = f.submit 'Create Personal Access Token', class: "btn btn-create"

app/views/admin/personal_access_tokens/index.html.haml

@@ -3,18 +3,15 @@
 
 .row.prepend-top-default
   .col-lg-12
-
     %h5.prepend-top-0
       Add a Personal Access Token
     %p.profile-settings-content
       Pick a name for the application, and we'll give you a unique token.
-
-    = render "form", personal_access_token: @personal_access_token, scopes: @scopes
+    = render "profiles/personal_access_tokens/form", user: :admin_user, personal_access_token: @personal_access_token, scopes: @scopes
 
     %hr
 
     %h5 Active Personal Access Tokens (#{@active_personal_access_tokens.length})
-
     - if @active_personal_access_tokens.present?
       .table-responsive
         %table.table.active-personal-access-tokens
@@ -44,7 +41,6 @@
                   = clipboard_button(clipboard_text: personal_access_token.token)
                 %td= personal_access_token.impersonation
                 %td= link_to "Revoke", revoke_admin_user_personal_access_token_path(id: personal_access_token.id, user_id: personal_access_token.user.username), 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
       .settings-message.text-center
         This user has no active tokens.
@@ -52,7 +48,6 @@
     %hr
 
     %h5 Inactive Personal Access Tokens (#{@inactive_personal_access_tokens.length})
-
     - if @inactive_personal_access_tokens.present?
       .table-responsive
         %table.table.inactive-personal-access-tokens
@@ -65,16 +60,20 @@
               %tr
                 %td= token.name
                 %td= token.created_at.to_date.to_s(:medium)
-
     - else
       .settings-message.text-center
         This user has no inactive tokens.
 
-
 :javascript
-  var date = $('#personal_access_token_expires_at').val();
+  var $dateField = $('#personal_access_token_expires_at');
+  var date = $dateField.val();
 
-  var datepicker = $(".datepicker").datepicker({
-    dateFormat: "yy-mm-dd",
-    minDate: 0
+  new Pikaday({
+    field: $dateField.get(0),
+    theme: 'gitlab-theme',
+    format: 'YYYY-MM-DD',
+    minDate: new Date(),
+    onSelect: function(dateText) {
+      $dateField.val(dateFormat(new Date(dateText), 'yyyy-mm-dd'));
+    }
   });

app/views/profiles/personal_access_tokens/_form.html.haml

 - personal_access_token = local_assigns.fetch(:personal_access_token)
 - scopes = local_assigns.fetch(:scopes)
 
-= form_for [:profile, personal_access_token], method: :post, html: { class: 'js-requires-input' } do |f|
+= form_for [user, personal_access_token], method: :post, html: { class: 'js-requires-input' } do |f|
 
   = form_errors(personal_access_token)
 

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", personal_access_token: @personal_access_token, scopes: @scopes
+    = render "form", user: :profile, personal_access_token: @personal_access_token, scopes: @scopes
 
     %hr
 

doc/api/README.md

@@ -8,7 +8,7 @@ under [`/lib/api`](https://gitlab.com/gitlab-org/gitlab-ce/tree/master/lib/api).
 Documentation for various API resources can be found separately in the
 following locations:
 
-- [Access Tokens](personal_access_tokens.md)
+- [Personal Access Tokens](personal_access_tokens.md)
 - [Award Emoji](award_emoji.md)
 - [Branches](branches.md)
 - [Broadcast Messages](broadcast_messages.md)

doc/api/personal_access_tokens.md

@@ -14,13 +14,13 @@ An example:
     "name": "mytoken",
     "revoked": false,
     "expires_at": "2017-01-04",
-    "scopes": ['api'],
+    "scopes": ["api"],
     "active": true
   }
 ]
 ```
 
-In addition, you can filter users based on state: `all`, `active` and `inactive`
+In addition, you can filter tokens based on state: `all`, `active` and `inactive`
 
 ```
 GET /personal_access_tokens?state=all
@@ -34,6 +34,18 @@ GET /personal_access_tokens?state=active
 GET /personal_access_tokens?state=inactive
 ```
 
+## Show
+
+```
+GET /personal_access_tokens/:personal_access_token_id
+```
+
+Parameters:
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `personal_access_token_id` | integer | yes | The ID of the personal access token |
+
 ## Create
 
 ```

doc/api/users.md

@@ -858,7 +858,7 @@ An example:
 ]
 ```
 
-In addition, you can filter users based on state: `all`, `active` and `inactive`
+In addition, you can filter tokens based on state: `all`, `active` and `inactive`
 
 ```
 GET /users/:user_id/personal_access_tokens?state=all
@@ -878,12 +878,27 @@ Finally, you can filter based on impersonation: `true` or `false`.
 GET /users/:user_id/personal_access_tokens?impersonation=true
 ```
 
+## Show a user personal access token
+
+It shows a user's personal access token. Note that only administrators can do this.
+
+```
+GET /users/:user_id/personal_access_tokens/:personal_access_token_id
+```
+
+Parameters:
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `user_id` | integer | yes | The ID of the user |
+| `personal_access_token_id` | integer | yes | The ID of the personal access token |
+
 ## Create a personal access token
 
 It creates a new personal access token. Note that only administrators can do this.
-If you set the impersonation flag to true, you can impersonate the user and
-performing both API calls and Git reads and writes. The user will not see these
-tokens in his profile settings.
+You are only able to create impersonation tokens to impersonate the user and perform
+both API calls and Git reads and writes. The user will not see these tokens in his profile
+settings page.
 
 ```
 POST /users/:user_id/personal_access_tokens

lib/api/personal_access_tokens.rb

@@ -3,7 +3,10 @@ module API
     before { authenticate! }
 
     resource :personal_access_tokens do
-      desc 'Retrieve personal access tokens'
+      desc 'Retrieve personal access tokens' do
+        detail 'This feature was introduced in GitLab 9.0'
+        success Entities::BasicPersonalAccessToken
+      end
       params do
         optional :state, type: String, default: 'all', values: %w[all active inactive], desc: 'Filters (all|active|inactive) personal_access_tokens'
       end
@@ -20,7 +23,24 @@ module API
         present personal_access_tokens, with: Entities::BasicPersonalAccessToken
       end
 
-      desc 'Create a personal access token'
+      desc 'Retrieve personal access token' do
+        detail 'This feature was introduced in GitLab 9.0'
+        success Entities::BasicPersonalAccessToken
+      end
+      params do
+        requires :personal_access_token_id, type: Integer, desc: 'The ID of the personal access token'
+      end
+      get ':personal_access_token_id' do
+        personal_access_token = PersonalAccessToken.find_by(id: params[:personal_access_token_id], user_id: current_user.id)
+        not_found!('PersonalAccessToken') unless personal_access_token
+
+        present personal_access_token, with: Entities::BasicPersonalAccessToken
+      end
+
+      desc 'Create a personal access token' do
+        detail 'This feature was introduced in GitLab 9.0'
+        success Entities::BasicPersonalAccessToken
+      end
       params do
         requires :name, type: String, desc: 'The name of the personal access token'
         optional :expires_at, type: Date, desc: 'The expiration date in the format YEAR-MONTH-DAY of the personal access token'
@@ -39,7 +59,10 @@ module API
         end
       end
 
-      desc 'Revoke a personal access token'
+      desc 'Revoke a personal access token' do
+        detail 'This feature was introduced in GitLab 9.0'
+        success Entities::BasicPersonalAccessToken
+      end
       params do
         requires :personal_access_token_id, type: Integer, desc: 'The ID of the personal access token'
       end
@@ -49,7 +72,7 @@ module API
 
         personal_access_token.revoke!
 
-        present personal_access_token, with: Entities::BasicPersonalAccessToken
+        no_content!
       end
     end
   end

lib/api/users.rb

@@ -363,71 +363,97 @@ module API
         present paginate(events), with: Entities::Event
       end
 
-      desc 'Retrieve personal access tokens. Available only for admins.'
       params do
-        requires :user_id, type: Integer
-        optional :state, type: String, default: 'all', values: %w[all active inactive], desc: 'Filters (all|active|inactive) personal_access_tokens'
-        optional :impersonation, type: Boolean, default: false, desc: 'Filters only impersonation personal_access_token'
+        requires :user_id, type: Integer, desc: 'The ID of the user'
       end
-      get ':user_id/personal_access_tokens' do
-        authenticated_as_admin!
+      segment ':user_id' do
+        resource :personal_access_tokens do
+          before { authenticated_as_admin! }
 
-        user = User.find_by(id: params[:user_id])
-        not_found!('User') unless user
+          desc 'Retrieve personal access tokens. Available only for admins.' do
+            detail 'This feature was introduced in GitLab 9.0'
+            success Entities::PersonalAccessToken
+          end
+          params do
+            optional :state, type: String, default: 'all', values: %w[all active inactive], desc: 'Filters (all|active|inactive) personal_access_tokens'
+            optional :impersonation, type: Boolean, default: false, desc: 'Filters only impersonation personal_access_tokens'
+          end
+          get do
+            user = User.find_by(id: params[:user_id])
+            not_found!('User') unless user
 
-        personal_access_tokens = PersonalAccessToken.and_impersonation_tokens.where(user_id: user.id)
-        personal_access_tokens = personal_access_tokens.impersonation if params[:impersonation]
+            personal_access_tokens = PersonalAccessToken.and_impersonation_tokens.where(user_id: user.id)
+            personal_access_tokens = personal_access_tokens.impersonation if params[:impersonation]
 
-        case params[:state]
-        when "active"
-          personal_access_tokens = personal_access_tokens.active
-        when "inactive"
-          personal_access_tokens = personal_access_tokens.inactive
-        end
+            case params[:state]
+            when "active"
+              personal_access_tokens = personal_access_tokens.active
+            when "inactive"
+              personal_access_tokens = personal_access_tokens.inactive
+            end
 
-        present personal_access_tokens, with: Entities::PersonalAccessToken
-      end
+            present personal_access_tokens, with: Entities::PersonalAccessToken
+          end
 
-      desc 'Create a personal access token. Available only for admins.'
-      params do
-        requires :user_id, type: Integer, desc: 'The ID of the user'
-        requires :name, type: String, desc: 'The name of the personal access token'
-        optional :expires_at, type: Date, desc: 'The expiration date in the format YEAR-MONTH-DAY of the personal access token'
-        optional :scopes, type: Array, desc: 'The array of scopes of the personal access token'
-        optional :impersonation, type: Boolean, default: false, desc: 'The impersonation flag of the personal access token'
-      end
-      post ':user_id/personal_access_tokens' do
-        authenticated_as_admin!
+          desc 'Create a personal access token. Available only for admins.' do
+            detail 'This feature was introduced in GitLab 9.0'
+            success Entities::PersonalAccessToken
+          end
+          params do
+            requires :name, type: String, desc: 'The name of the personal access token'
+            optional :expires_at, type: Date, desc: 'The expiration date in the format YEAR-MONTH-DAY of the personal access token'
+            optional :scopes, type: Array, desc: 'The array of scopes of the personal access token'
+            optional :impersonation, type: Boolean, default: false, desc: 'The impersonation flag of the personal access token'
+          end
+          post do
+            user = User.find_by(id: params[:user_id])
+            not_found!('User') unless user
 
-        user = User.find_by(id: params[:user_id])
-        not_found!('User') unless user
+            personal_access_token = PersonalAccessToken.generate(declared_params(include_missing: false, include_parent_namespaces: true))
+
+            if personal_access_token.save
+              present personal_access_token, with: Entities::PersonalAccessToken
+            else
+              render_validation_error!(personal_access_token)
+            end
+          end
 
-        personal_access_token = PersonalAccessToken.generate(declared_params(include_missing: false))
+          desc 'Retrieve personal access token. Available only for admins.' do
+            detail 'This feature was introduced in GitLab 9.0'
+            success Entities::PersonalAccessToken
+          end
+          params do
+            requires :personal_access_token_id, type: Integer, desc: 'The ID of the personal access token'
+          end
+          get '/:personal_access_token_id' do
+            user = User.find_by(id: params[:user_id])
+            not_found!('User') unless user
 
-        if personal_access_token.save
-          present personal_access_token, with: Entities::PersonalAccessToken
-        else
-          render_validation_error!(personal_access_token)
-        end
-      end
+            personal_access_token = PersonalAccessToken.and_impersonation_tokens.find_by(user_id: user.id, id: params[:personal_access_token_id])
+            not_found!('PersonalAccessToken') unless personal_access_token
 
-      desc 'Revoke a personal access token. Available only for admins.'
-      params do
-        requires :user_id, type: Integer, desc: 'The ID of the user'
-        requires :personal_access_token_id, type: Integer, desc: 'The ID of the personal access token'
-      end
-      delete ':user_id/personal_access_tokens/:personal_access_token_id' do
-        authenticated_as_admin!
+            present personal_access_token, with: Entities::PersonalAccessToken
+          end
 
-        user = User.find_by(id: params[:user_id])
-        not_found!('User') unless user
+          desc 'Revoke a personal access token. Available only for admins.' do
+            detail 'This feature was introduced in GitLab 9.0'
+            success Entities::PersonalAccessToken
+          end
+          params do
+            requires :personal_access_token_id, type: Integer, desc: 'The ID of the personal access token'
+          end
+          delete '/:personal_access_token_id' do
+            user = User.find_by(id: params[:user_id])
+            not_found!('User') unless user
 
-        personal_access_token = PersonalAccessToken.and_impersonation_tokens.find_by(user_id: user.id, id: params[:personal_access_token_id])
-        not_found!('PersonalAccessToken') unless personal_access_token
+            personal_access_token = PersonalAccessToken.and_impersonation_tokens.find_by(user_id: user.id, id: params[:personal_access_token_id])
+            not_found!('PersonalAccessToken') unless personal_access_token
 
-        personal_access_token.revoke!
+            personal_access_token.revoke!
 
-        present personal_access_token, with: Entities::PersonalAccessToken
+            no_content!
+          end
+        end
       end
     end
 

spec/controllers/profiles/personal_access_tokens_spec.rb

@@ -52,21 +52,17 @@ describe Profiles::PersonalAccessTokensController do
     let!(:inactive_personal_access_token) { create(:revoked_personal_access_token, user: user) }
     let!(:impersonation_personal_access_token) { create(:impersonation_personal_access_token, user: user) }
 
-    it "retrieves active personal access tokens" do
-      get :index
+    before { get :index }
 
+    it "retrieves active personal access tokens" do
       expect(assigns(:active_personal_access_tokens)).to include(active_personal_access_token)
     end
 
     it "retrieves inactive personal access tokens" do
-      get :index
-
       expect(assigns(:inactive_personal_access_tokens)).to include(inactive_personal_access_token)
     end
 
     it "does not retrieve impersonation personal access tokens" do
-      get :index
-
       expect(assigns(:active_personal_access_tokens)).not_to include(impersonation_personal_access_token)
       expect(assigns(:inactive_personal_access_tokens)).not_to include(impersonation_personal_access_token)
     end

spec/features/admin/admin_users_personal_access_tokens_spec.rb

@@ -22,9 +22,7 @@ describe 'Admin > Users > Personal Access Tokens', feature: true, js: true do
     allow_any_instance_of(PersonalAccessToken).to receive(:errors).and_return(errors)
   end
 
-  before do
-    login_as(admin)
-  end
+  before { login_as(admin) }
 
   describe "token creation" do
     it "allows creation of a token" do
@@ -35,15 +33,13 @@ describe 'Admin > Users > Personal Access Tokens', feature: true, js: true do
 
       # Set date to 1st of next month
       find_field("Expires at").trigger('focus')
-      find("a[title='Next']").click
+      find(".pika-next").click
       click_on "1"
 
       # Scopes
       check "api"
       check "read_user"
 
-      check "You can impersonate the user"
-
       click_on "Create Personal Access Token"
       expect(active_personal_access_tokens).to have_text(name)
       expect(active_personal_access_tokens).to have_text('In')

spec/requests/api/personal_access_tokens_spec.rb

@@ -4,6 +4,7 @@ describe API::PersonalAccessTokens, api: true  do
   include ApiHelpers
 
   let(:user)  { create(:user) }
+  let(:not_found_token) { (PersonalAccessToken.maximum('id') || 0) + 10 }
 
   describe "GET /personal_access_tokens" do
     let!(:active_personal_access_token) { create(:personal_access_token, user: user) }
@@ -76,12 +77,38 @@ describe API::PersonalAccessTokens, api: true  do
     end
   end
 
+  describe 'GET /personal_access_tokens/:personal_access_token_id' do
+    let!(:personal_access_token) { create(:personal_access_token, user: user, revoked: false) }
+    let!(:personal_access_token_of_another_user) { create(:personal_access_token, revoked: false) }
+
+    it 'returns a 404 error if personal access token not found' do
+      get api("/personal_access_tokens/#{not_found_token}", user)
+
+      expect(response).to have_http_status(404)
+      expect(json_response['message']).to eq('404 PersonalAccessToken Not Found')
+    end
+
+    it 'returns a 404 error if personal access token exists but it is a personal access tokens of another user' do
+      get api("/personal_access_tokens/#{personal_access_token_of_another_user.id}", user)
+
+      expect(response).to have_http_status(404)
+      expect(json_response['message']).to eq('404 PersonalAccessToken Not Found')
+    end
+
+    it 'returns a personal access token and does not expose token in the json response' do
+      get api("/personal_access_tokens/#{personal_access_token.id}", user)
+
+      expect(response).to have_http_status(200)
+      expect(json_response['token']).not_to be_present
+    end
+  end
+
   describe 'DELETE /personal_access_tokens/:personal_access_token_id' do
     let!(:personal_access_token) { create(:personal_access_token, user: user, revoked: false) }
     let!(:personal_access_token_of_another_user) { create(:personal_access_token, revoked: false) }
 
     it 'returns a 404 error if personal access token not found' do
-      delete api("/personal_access_tokens/42", user)
+      delete api("/personal_access_tokens/#{not_found_token}", user)
 
       expect(response).to have_http_status(404)
       expect(json_response['message']).to eq('404 PersonalAccessToken Not Found')
@@ -97,11 +124,9 @@ describe API::PersonalAccessTokens, api: true  do
     it 'revokes a personal access token and does not expose token in the json response' do
       delete api("/personal_access_tokens/#{personal_access_token.id}", user)
 
-      expect(response).to have_http_status(200)
+      expect(response).to have_http_status(204)
       expect(personal_access_token.revoked).to eq(false)
       expect(personal_access_token.reload.revoked).to eq(true)
-      expect(json_response['revoked']).to eq(true)
-      expect(json_response['token']).not_to be_present
     end
   end
 end

spec/requests/api/users_spec.rb

@@ -11,6 +11,7 @@ describe API::Users, api: true  do
   let(:ldap_user) { create(:omniauth_user, provider: 'ldapmain') }
   let(:ldap_blocked_user) { create(:omniauth_user, provider: 'ldapmain', state: 'ldap_blocked') }
   let(:not_existing_user_id) { (User.maximum('id') || 0 ) + 10 }
+  let(:not_existing_pat_id) { (PersonalAccessToken.maximum('id') || 0 ) + 10 }
 
   describe "GET /users" do
     context "when unauthenticated" do
@@ -1268,7 +1269,47 @@ describe API::Users, api: true  do
     end
   end
 
-  describe 'DELETE /users/:id/personal_access_tokens/:personal_access_token_id' do
+  describe 'GET /users/:user_id/personal_access_tokens/:personal_access_token_id' do
+    let!(:personal_access_token) { create(:personal_access_token, user: user, revoked: false) }
+    let!(:impersonation_token) { create(:impersonation_personal_access_token, user: user, revoked: false) }
+
+    it 'returns 404 error if user not found' do
+      get api("/users/#{not_existing_user_id}/personal_access_tokens/1", admin)
+
+      expect(response).to have_http_status(404)
+      expect(json_response['message']).to eq('404 User Not Found')
+    end
+
+    it 'returns a 404 error if personal access token not found' do
+      get api("/users/#{user.id}/personal_access_tokens/#{not_existing_pat_id}", admin)
+
+      expect(response).to have_http_status(404)
+      expect(json_response['message']).to eq('404 PersonalAccessToken Not Found')
+    end
+
+    it 'returns a 403 error when authenticated as normal user' do
+      get api("/users/#{user.id}/personal_access_tokens/#{personal_access_token.id}", user)
+
+      expect(response).to have_http_status(403)
+      expect(json_response['message']).to eq('403 Forbidden')
+    end
+
+    it 'returns a personal access token' do
+      get api("/users/#{user.id}/personal_access_tokens/#{personal_access_token.id}", admin)
+
+      expect(response).to have_http_status(200)
+      expect(json_response['token']).to be_present
+    end
+
+    it 'returns an impersonation token' do
+      get api("/users/#{user.id}/personal_access_tokens/#{impersonation_token.id}", admin)
+
+      expect(response).to have_http_status(200)
+      expect(json_response['impersonation']).to eq(true)
+    end
+  end
+
+  describe 'DELETE /users/:user_id/personal_access_tokens/:personal_access_token_id' do
     let!(:personal_access_token) { create(:personal_access_token, user: user, revoked: false) }
     let!(:impersonation_token) { create(:impersonation_personal_access_token, user: user, revoked: false) }
 
@@ -1280,7 +1321,7 @@ describe API::Users, api: true  do
     end
 
     it 'returns a 404 error if personal access token not found' do
-      delete api("/users/#{user.id}/personal_access_tokens/42", admin)
+      delete api("/users/#{user.id}/personal_access_tokens/#{not_existing_pat_id}", admin)
 
       expect(response).to have_http_status(404)
       expect(json_response['message']).to eq('404 PersonalAccessToken Not Found')
@@ -1296,20 +1337,17 @@ describe API::Users, api: true  do
     it 'revokes a personal access token' do
       delete api("/users/#{user.id}/personal_access_tokens/#{personal_access_token.id}", admin)
 
-      expect(response).to have_http_status(200)
+      expect(response).to have_http_status(204)
       expect(personal_access_token.revoked).to eq(false)
       expect(personal_access_token.reload.revoked).to eq(true)
-      expect(json_response['revoked']).to eq(true)
-      expect(json_response['token']).to be_present
     end
 
     it 'revokes an impersonation token' do
       delete api("/users/#{user.id}/personal_access_tokens/#{impersonation_token.id}", admin)
 
-      expect(response).to have_http_status(200)
+      expect(response).to have_http_status(204)
       expect(impersonation_token.revoked).to eq(false)
       expect(impersonation_token.reload.revoked).to eq(true)
-      expect(json_response['revoked']).to eq(true)
     end
   end
 end