Commit cf2206eb authored by Douwe Maan's avatar Douwe Maan

Merge branch '24537-reenable-private-token-with-sudo' into 'master'

Reenables /user API request to return private-token if user is admin and requested with sudo

## What does this MR do?

Reenables the API /users to return `private-token` when sudo is either a parameter or passed as a header and the user is admin.

## Screenshots (if relevant)

Without **sudo**:

![Screen_Shot_2016-11-21_at_11.44.49](/uploads/ebecf95dbadaf4a159b80c61c75771d9/Screen_Shot_2016-11-21_at_11.44.49.png)

With **sudo**:
![Screen_Shot_2016-11-21_at_11.45.52](/uploads/f25f9ddffcf2b921e9694e5a250191d3/Screen_Shot_2016-11-21_at_11.45.52.png)

## Does this MR meet the acceptance criteria?

- [x] [Changelog entry](https://docs.gitlab.com/ce/development/changelog.html) added
- [x] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- [x] API support added
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if it does - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?


Closes #24537

See merge request !7615
parents e7b045ea 3ed96afc
---
title: Reenables /user API request to return private-token if user is admin and request
is made with sudo
merge_request: 7615
author:
...@@ -291,7 +291,9 @@ Parameters: ...@@ -291,7 +291,9 @@ Parameters:
- `id` (required) - The ID of the user - `id` (required) - The ID of the user
## Current user ## User
### For normal users
Gets currently authenticated user. Gets currently authenticated user.
...@@ -335,6 +337,53 @@ GET /user ...@@ -335,6 +337,53 @@ GET /user
} }
``` ```
### For admins
Parameters:
- `sudo` (required) - the ID of a user
```
GET /user
```
```json
{
"id": 1,
"username": "john_smith",
"email": "john@example.com",
"name": "John Smith",
"state": "active",
"avatar_url": "http://localhost:3000/uploads/user/avatar/1/index.jpg",
"web_url": "http://localhost:3000/john_smith",
"created_at": "2012-05-23T08:00:58Z",
"is_admin": false,
"bio": null,
"location": null,
"skype": "",
"linkedin": "",
"twitter": "",
"website_url": "",
"organization": "",
"last_sign_in_at": "2012-06-01T11:41:01Z",
"confirmed_at": "2012-05-23T09:05:22Z",
"theme_id": 1,
"color_scheme_id": 2,
"projects_limit": 100,
"current_sign_in_at": "2012-06-02T06:36:55Z",
"identities": [
{"provider": "github", "extern_uid": "2435223452345"},
{"provider": "bitbucket", "extern_uid": "john_smith"},
{"provider": "google_oauth2", "extern_uid": "8776128412476123468721346"}
],
"can_create_group": true,
"can_create_project": true,
"two_factor_enabled": true,
"external": false,
"private_token": "dd34asd13as"
}
```
## List SSH keys ## List SSH keys
Get a list of currently authenticated user's SSH keys. Get a list of currently authenticated user's SSH keys.
......
...@@ -22,7 +22,7 @@ module API ...@@ -22,7 +22,7 @@ module API
expose :provider, :extern_uid expose :provider, :extern_uid
end end
class UserFull < User class UserPublic < User
expose :last_sign_in_at expose :last_sign_in_at
expose :confirmed_at expose :confirmed_at
expose :email expose :email
...@@ -34,7 +34,7 @@ module API ...@@ -34,7 +34,7 @@ module API
expose :external expose :external
end end
class UserLogin < UserFull class UserWithPrivateToken < UserPublic
expose :private_token expose :private_token
end end
...@@ -289,7 +289,7 @@ module API ...@@ -289,7 +289,7 @@ module API
end end
class SSHKeyWithUser < SSHKey class SSHKeyWithUser < SSHKey
expose :user, using: Entities::UserFull expose :user, using: Entities::UserPublic
end end
class Note < Grape::Entity class Note < Grape::Entity
......
...@@ -44,11 +44,14 @@ module API ...@@ -44,11 +44,14 @@ module API
return nil return nil
end end
identifier = sudo_identifier() identifier = sudo_identifier
# If the sudo is the current user do nothing if identifier
if identifier && !(@current_user.id == identifier || @current_user.username == identifier) # We check for private_token because we cannot allow PAT to be used
forbidden!('Must be admin to use sudo') unless @current_user.is_admin? forbidden!('Must be admin to use sudo') unless @current_user.is_admin?
forbidden!('Private token must be specified in order to use sudo') unless private_token_used?
@impersonator = @current_user
@current_user = User.by_username_or_id(identifier) @current_user = User.by_username_or_id(identifier)
not_found!("No user id or username for: #{identifier}") if @current_user.nil? not_found!("No user id or username for: #{identifier}") if @current_user.nil?
end end
...@@ -383,6 +386,10 @@ module API ...@@ -383,6 +386,10 @@ module API
links.join(', ') links.join(', ')
end end
def private_token_used?
private_token == @current_user.private_token
end
def secret_token def secret_token
Gitlab::Shell.secret_token Gitlab::Shell.secret_token
end end
......
module API module API
class Session < Grape::API class Session < Grape::API
desc 'Login to get token' do desc 'Login to get token' do
success Entities::UserLogin success Entities::UserWithPrivateToken
end end
params do params do
optional :login, type: String, desc: 'The username' optional :login, type: String, desc: 'The username'
...@@ -14,7 +14,7 @@ module API ...@@ -14,7 +14,7 @@ module API
return unauthorized! unless user return unauthorized! unless user
return render_api_error!('401 Unauthorized. You have 2FA enabled. Please use a personal access token to access the API', 401) if user.two_factor_enabled? return render_api_error!('401 Unauthorized. You have 2FA enabled. Please use a personal access token to access the API', 401) if user.two_factor_enabled?
present user, with: Entities::UserLogin present user, with: Entities::UserWithPrivateToken
end end
end end
end end
...@@ -51,7 +51,7 @@ module API ...@@ -51,7 +51,7 @@ module API
users = users.external if params[:external] && current_user.is_admin? users = users.external if params[:external] && current_user.is_admin?
end end
entity = current_user.is_admin? ? Entities::UserFull : Entities::UserBasic entity = current_user.is_admin? ? Entities::UserPublic : Entities::UserBasic
present paginate(users), with: entity present paginate(users), with: entity
end end
...@@ -66,7 +66,7 @@ module API ...@@ -66,7 +66,7 @@ module API
not_found!('User') unless user not_found!('User') unless user
if current_user && current_user.is_admin? if current_user && current_user.is_admin?
present user, with: Entities::UserFull present user, with: Entities::UserPublic
elsif can?(current_user, :read_user, user) elsif can?(current_user, :read_user, user)
present user, with: Entities::User present user, with: Entities::User
else else
...@@ -75,7 +75,7 @@ module API ...@@ -75,7 +75,7 @@ module API
end end
desc 'Create a user. Available only for admins.' do desc 'Create a user. Available only for admins.' do
success Entities::UserFull success Entities::UserPublic
end end
params do params do
requires :email, type: String, desc: 'The email of the user' requires :email, type: String, desc: 'The email of the user'
...@@ -99,7 +99,7 @@ module API ...@@ -99,7 +99,7 @@ module API
end end
if user.save if user.save
present user, with: Entities::UserFull present user, with: Entities::UserPublic
else else
conflict!('Email has already been taken') if User. conflict!('Email has already been taken') if User.
where(email: user.email). where(email: user.email).
...@@ -114,7 +114,7 @@ module API ...@@ -114,7 +114,7 @@ module API
end end
desc 'Update a user. Available only for admins.' do desc 'Update a user. Available only for admins.' do
success Entities::UserFull success Entities::UserPublic
end end
params do params do
requires :id, type: Integer, desc: 'The ID of the user' requires :id, type: Integer, desc: 'The ID of the user'
...@@ -161,7 +161,7 @@ module API ...@@ -161,7 +161,7 @@ module API
user_params.delete(:provider) user_params.delete(:provider)
if user.update_attributes(user_params) if user.update_attributes(user_params)
present user, with: Entities::UserFull present user, with: Entities::UserPublic
else else
render_validation_error!(user) render_validation_error!(user)
end end
...@@ -350,10 +350,10 @@ module API ...@@ -350,10 +350,10 @@ module API
resource :user do resource :user do
desc 'Get the currently authenticated user' do desc 'Get the currently authenticated user' do
success Entities::UserFull success Entities::UserPublic
end end
get do get do
present current_user, with: Entities::UserFull present current_user, with: @impersonator ? Entities::UserWithPrivateToken : Entities::UserPublic
end end
desc "Get the currently authenticated user's SSH keys" do desc "Get the currently authenticated user's SSH keys" do
......
{
"type": "object",
"required": [
"id",
"username",
"email",
"name",
"state",
"avatar_url",
"web_url",
"created_at",
"is_admin",
"bio",
"location",
"skype",
"linkedin",
"twitter",
"website_url",
"organization",
"last_sign_in_at",
"confirmed_at",
"theme_id",
"color_scheme_id",
"projects_limit",
"current_sign_in_at",
"identities",
"can_create_group",
"can_create_project",
"two_factor_enabled",
"external",
"private_token"
],
"properties": {
"$ref": "full.json",
"private_token": { "type": "string" }
}
}
{
"type": "object",
"required": [
"id",
"username",
"email",
"name",
"state",
"avatar_url",
"web_url",
"created_at",
"is_admin",
"bio",
"location",
"skype",
"linkedin",
"twitter",
"website_url",
"organization",
"last_sign_in_at",
"confirmed_at",
"theme_id",
"color_scheme_id",
"projects_limit",
"current_sign_in_at",
"identities",
"can_create_group",
"can_create_project",
"two_factor_enabled",
"external"
],
"properties": {
"id": { "type": "integer" },
"username": { "type": "string" },
"email": {
"type": "string",
"pattern": "^[^@]+@[^@]+$"
},
"name": { "type": "string" },
"state": {
"type": "string",
"enum": ["active", "blocked"]
},
"avatar_url": { "type": "string" },
"web_url": { "type": "string" },
"created_at": { "type": "date" },
"is_admin": { "type": "boolean" },
"bio": { "type": ["string", "null"] },
"location": { "type": ["string", "null"] },
"skype": { "type": "string" },
"linkedin": { "type": "string" },
"twitter": { "type": "string "},
"website_url": { "type": "string" },
"organization": { "type": ["string", "null"] },
"last_sign_in_at": { "type": "date" },
"confirmed_at": { "type": ["date", "null"] },
"theme_id": { "type": "integer" },
"color_scheme_id": { "type": "integer" },
"projects_limit": { "type": "integer" },
"current_sign_in_at": { "type": "date" },
"identities": {
"type": "array",
"items": {
"type": "object",
"properties": {
"provider": {
"type": "string",
"enum": ["github", "bitbucket", "google_oauth2"]
},
"extern_uid": { "type": ["number", "string"] }
}
}
},
"can_create_group": { "type": "boolean" },
"can_create_project": { "type": "boolean" },
"two_factor_enabled": { "type": "boolean" },
"external": { "type": "boolean" }
}
}
...@@ -153,85 +153,144 @@ describe API::Helpers, api: true do ...@@ -153,85 +153,144 @@ describe API::Helpers, api: true do
end end
end end
it "changes current user to sudo when admin" do context 'sudo usage' do
context 'with admin' do
context 'with header' do
context 'with id' do
it 'changes current_user to sudo' do
set_env(admin, user.id) set_env(admin, user.id)
expect(current_user).to eq(user)
set_param(admin, user.id)
expect(current_user).to eq(user)
set_env(admin, user.username)
expect(current_user).to eq(user)
set_param(admin, user.username)
expect(current_user).to eq(user) expect(current_user).to eq(user)
end end
it "throws an error when the current user is not an admin and attempting to sudo" do it 'handles sudo to oneself' do
set_env(user, admin.id) set_env(admin, admin.id)
expect { current_user }.to raise_error(Exception)
set_param(user, admin.id) expect(current_user).to eq(admin)
expect { current_user }.to raise_error(Exception)
set_env(user, admin.username)
expect { current_user }.to raise_error(Exception)
set_param(user, admin.username)
expect { current_user }.to raise_error(Exception)
end end
it "throws an error when the user cannot be found for a given id" do it 'throws an error when user cannot be found' do
id = user.id + admin.id id = user.id + admin.id
expect(user.id).not_to eq(id) expect(user.id).not_to eq(id)
expect(admin.id).not_to eq(id) expect(admin.id).not_to eq(id)
set_env(admin, id) set_env(admin, id)
expect { current_user }.to raise_error(Exception)
set_param(admin, id)
expect { current_user }.to raise_error(Exception) expect { current_user }.to raise_error(Exception)
end end
end
context 'with username' do
it 'changes current_user to sudo' do
set_env(admin, user.username)
expect(current_user).to eq(user)
end
it 'handles sudo to oneself' do
set_env(admin, admin.username)
expect(current_user).to eq(admin)
end
it "throws an error when the user cannot be found for a given username" do it "throws an error when the user cannot be found for a given username" do
username = "#{user.username}#{admin.username}" username = "#{user.username}#{admin.username}"
expect(user.username).not_to eq(username) expect(user.username).not_to eq(username)
expect(admin.username).not_to eq(username) expect(admin.username).not_to eq(username)
set_env(admin, username) set_env(admin, username)
expect { current_user }.to raise_error(Exception)
set_param(admin, username)
expect { current_user }.to raise_error(Exception) expect { current_user }.to raise_error(Exception)
end end
end
end
it "handles sudo's to oneself" do context 'with param' do
set_env(admin, admin.id) context 'with id' do
expect(current_user).to eq(admin) it 'changes current_user to sudo' do
set_param(admin, user.id)
expect(current_user).to eq(user)
end
it 'handles sudo to oneself' do
set_param(admin, admin.id) set_param(admin, admin.id)
expect(current_user).to eq(admin)
set_env(admin, admin.username)
expect(current_user).to eq(admin)
set_param(admin, admin.username)
expect(current_user).to eq(admin) expect(current_user).to eq(admin)
end end
it "handles multiple sudo's to oneself" do it 'handles sudo to oneself using string' do
set_env(admin, user.id) set_env(admin, user.id.to_s)
expect(current_user).to eq(user)
expect(current_user).to eq(user)
set_env(admin, user.username)
expect(current_user).to eq(user)
expect(current_user).to eq(user)
set_param(admin, user.id)
expect(current_user).to eq(user)
expect(current_user).to eq(user) expect(current_user).to eq(user)
end
it 'throws an error when user cannot be found' do
id = user.id + admin.id
expect(user.id).not_to eq(id)
expect(admin.id).not_to eq(id)
set_param(admin, id)
expect { current_user }.to raise_error(Exception)
end
end
context 'with username' do
it 'changes current_user to sudo' do
set_param(admin, user.username) set_param(admin, user.username)
expect(current_user).to eq(user)
expect(current_user).to eq(user) expect(current_user).to eq(user)
end end
it "handles multiple sudo's to oneself using string ids" do it 'handles sudo to oneself' do
set_env(admin, user.id.to_s) set_param(admin, admin.username)
expect(current_user).to eq(user)
expect(current_user).to eq(user)
set_param(admin, user.id.to_s) expect(current_user).to eq(admin)
expect(current_user).to eq(user) end
expect(current_user).to eq(user)
it "throws an error when the user cannot be found for a given username" do
username = "#{user.username}#{admin.username}"
expect(user.username).not_to eq(username)
expect(admin.username).not_to eq(username)
set_param(admin, username)
expect { current_user }.to raise_error(Exception)
end
end
end
end
context 'with regular user' do
context 'with env' do
it 'changes current_user to sudo when admin and user id' do
set_env(user, admin.id)
expect { current_user }.to raise_error(Exception)
end
it 'changes current_user to sudo when admin and user username' do
set_env(user, admin.username)
expect { current_user }.to raise_error(Exception)
end
end
context 'with params' do
it 'changes current_user to sudo when admin and user id' do
set_param(user, admin.id)
expect { current_user }.to raise_error(Exception)
end
it 'changes current_user to sudo when admin and user username' do
set_param(user, admin.username)
expect { current_user }.to raise_error(Exception)
end
end
end
end end
end end
......
...@@ -651,22 +651,77 @@ describe API::Users, api: true do ...@@ -651,22 +651,77 @@ describe API::Users, api: true do
end end
describe "GET /user" do describe "GET /user" do
it "returns current user" do let(:personal_access_token) { create(:personal_access_token, user: user) }
let(:private_token) { user.private_token }
context 'with regular user' do
context 'with personal access token' do
it 'returns 403 without private token when sudo is defined' do
get api("/user?private_token=#{personal_access_token.token}&sudo=#{user.id}")
expect(response).to have_http_status(403)
end
end
context 'with private token' do
it 'returns 403 without private token when sudo defined' do
get api("/user?private_token=#{private_token}&sudo=#{user.id}")
expect(response).to have_http_status(403)
end
end
it 'returns current user without private token when sudo not defined' do
get api("/user", user) get api("/user", user)
expect(response).to have_http_status(200) expect(response).to have_http_status(200)
expect(json_response['email']).to eq(user.email) expect(response).to match_response_schema('user/public')
expect(json_response['is_admin']).to eq(user.is_admin?) end
expect(json_response['can_create_project']).to eq(user.can_create_project?) end
expect(json_response['can_create_group']).to eq(user.can_create_group?)
expect(json_response['projects_limit']).to eq(user.projects_limit) context 'with admin' do
expect(json_response['private_token']).to be_blank let(:user) { create(:admin) }
context 'with personal access token' do
it 'returns 403 without private token when sudo defined' do
get api("/user?private_token=#{personal_access_token.token}&sudo=#{user.id}")
expect(response).to have_http_status(403)
end
it 'returns current user without private token when sudo not defined' do
get api("/user?private_token=#{personal_access_token.token}")
expect(response).to have_http_status(200)
expect(response).to match_response_schema('user/public')
end
end
context 'with private token' do
it 'returns current user with private token when sudo defined' do
get api("/user?private_token=#{private_token}&sudo=#{user.id}")
expect(response).to have_http_status(200)
expect(response).to match_response_schema('user/login')
end end
it 'returns current user without private token when sudo not defined' do
get api("/user?private_token=#{private_token}")
expect(response).to have_http_status(200)
expect(response).to match_response_schema('user/public')
end
end
end
context 'with unauthenticated user' do
it "returns 401 error if user is unauthenticated" do it "returns 401 error if user is unauthenticated" do
get api("/user") get api("/user")
expect(response).to have_http_status(401) expect(response).to have_http_status(401)
end end
end end
end
describe "GET /user/keys" do describe "GET /user/keys" do
context "when unauthenticated" do context "when unauthenticated" do
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment