Don't allow blocked users to authenticate through other means
Gitlab::Auth.find_with_user_password is currently used in these places: - resource_owner_from_credentials in config/initializers/doorkeeper.rb, which is used for the OAuth Resource Owner Password Credentials flow - the /session API call in lib/api/session.rb, which is used to reveal the user's current authentication_token In both cases users should only be authenticated if they're in the active state.
This commit is contained in:
parent
789db2cc19
commit
93daeee164
6 changed files with 81 additions and 3 deletions
|
@ -40,7 +40,7 @@ module Gitlab
|
||||||
|
|
||||||
Gitlab::LDAP::Authentication.login(login, password)
|
Gitlab::LDAP::Authentication.login(login, password)
|
||||||
else
|
else
|
||||||
user if user.valid_password?(password)
|
user if user.active? && user.valid_password?(password)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -210,6 +210,18 @@ describe Gitlab::Auth, lib: true do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
it "does not find user in blocked state" do
|
||||||
|
user.block
|
||||||
|
|
||||||
|
expect( gl_auth.find_with_user_password(username, password) ).not_to eql user
|
||||||
|
end
|
||||||
|
|
||||||
|
it "does not find user in ldap_blocked state" do
|
||||||
|
user.ldap_block
|
||||||
|
|
||||||
|
expect( gl_auth.find_with_user_password(username, password) ).not_to eql user
|
||||||
|
end
|
||||||
|
|
||||||
context "with ldap enabled" do
|
context "with ldap enabled" do
|
||||||
before do
|
before do
|
||||||
allow(Gitlab::LDAP::Config).to receive(:enabled?).and_return(true)
|
allow(Gitlab::LDAP::Config).to receive(:enabled?).and_return(true)
|
||||||
|
|
|
@ -39,4 +39,22 @@ describe API::API, api: true do
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
describe "when user is blocked" do
|
||||||
|
it "returns authentication error" do
|
||||||
|
user.block
|
||||||
|
get api("/user"), access_token: token.token
|
||||||
|
|
||||||
|
expect(response).to have_http_status(401)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
describe "when user is ldap_blocked" do
|
||||||
|
it "returns authentication error" do
|
||||||
|
user.ldap_block
|
||||||
|
get api("/user"), access_token: token.token
|
||||||
|
|
||||||
|
expect(response).to have_http_status(401)
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -29,5 +29,27 @@ describe API::API, api: true do
|
||||||
expect(json_response['access_token']).not_to be_nil
|
expect(json_response['access_token']).not_to be_nil
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
context "when user is blocked" do
|
||||||
|
it "does not create an access token" do
|
||||||
|
user = create(:user)
|
||||||
|
user.block
|
||||||
|
|
||||||
|
request_oauth_token(user)
|
||||||
|
|
||||||
|
expect(response).to have_http_status(401)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context "when user is ldap_blocked" do
|
||||||
|
it "does not create an access token" do
|
||||||
|
user = create(:user)
|
||||||
|
user.ldap_block
|
||||||
|
|
||||||
|
request_oauth_token(user)
|
||||||
|
|
||||||
|
expect(response).to have_http_status(401)
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -87,5 +87,23 @@ describe API::Session, api: true do
|
||||||
expect(response).to have_http_status(400)
|
expect(response).to have_http_status(400)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
context "when user is blocked" do
|
||||||
|
it "returns authentication error" do
|
||||||
|
user.block
|
||||||
|
post api("/session"), email: user.username, password: user.password
|
||||||
|
|
||||||
|
expect(response).to have_http_status(401)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
context "when user is ldap_blocked" do
|
||||||
|
it "returns authentication error" do
|
||||||
|
user.ldap_block
|
||||||
|
post api("/session"), email: user.username, password: user.password
|
||||||
|
|
||||||
|
expect(response).to have_http_status(401)
|
||||||
|
end
|
||||||
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -221,12 +221,20 @@ describe 'Git HTTP requests', lib: true do
|
||||||
end
|
end
|
||||||
|
|
||||||
context "when the user is blocked" do
|
context "when the user is blocked" do
|
||||||
it "responds with status 404" do
|
it "responds with status 401" do
|
||||||
user.block
|
user.block
|
||||||
project.team << [user, :master]
|
project.team << [user, :master]
|
||||||
|
|
||||||
download(path, env) do |response|
|
download(path, env) do |response|
|
||||||
expect(response).to have_http_status(404)
|
expect(response).to have_http_status(401)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
it "responds with status 401 for unknown projects (no project existence information leak)" do
|
||||||
|
user.block
|
||||||
|
|
||||||
|
download('doesnt/exist.git', env) do |response|
|
||||||
|
expect(response).to have_http_status(401)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
Loading…
Reference in a new issue