From b46d54e92260cdc574549bcf42c7e765b271d45e Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Fri, 13 May 2016 16:42:56 +0200 Subject: [PATCH] SONAR-7640 Return unauthenticated when login/password are bad --- .../java/it/user/LocalAuthenticationTest.java | 48 ++++++++++++++----- .../user/RailsExternalAuthenticationTest.java | 42 ++++++++++++++++ .../api/authentication_controller.rb | 8 +++- .../app/controllers/sessions_controller.rb | 30 ++++++++---- .../WEB-INF/lib/authenticated_system.rb | 7 +-- 5 files changed, 109 insertions(+), 26 deletions(-) diff --git a/it/it-tests/src/test/java/it/user/LocalAuthenticationTest.java b/it/it-tests/src/test/java/it/user/LocalAuthenticationTest.java index 61df33b9eea..07a6ec4830a 100644 --- a/it/it-tests/src/test/java/it/user/LocalAuthenticationTest.java +++ b/it/it-tests/src/test/java/it/user/LocalAuthenticationTest.java @@ -183,7 +183,7 @@ public class LocalAuthenticationTest { userRule.createUser(login, LOGIN, null, password); // authenticate - assertThat(checkAuthenticationThroughWebService(login, password)).isFalse(); + assertThat(checkAuthenticationWithAuthenticateWebService(login, password)).isFalse(); } @Test @@ -219,25 +219,51 @@ public class LocalAuthenticationTest { } @Test - public void authentication_with_web_service() { - assertThat(checkAuthenticationThroughWebService("admin", "admin")).isTrue(); - assertThat(checkAuthenticationThroughWebService("wrong", "admin")).isFalse(); - assertThat(checkAuthenticationThroughWebService("admin", "wrong")).isFalse(); - assertThat(checkAuthenticationThroughWebService(null, null)).isTrue(); + public void authentication_with_authentication_ws() { + assertThat(checkAuthenticationWithAuthenticateWebService("admin", "admin")).isTrue(); + assertThat(checkAuthenticationWithAuthenticateWebService("wrong", "admin")).isFalse(); + assertThat(checkAuthenticationWithAuthenticateWebService("admin", "wrong")).isFalse(); + assertThat(checkAuthenticationWithAuthenticateWebService(null, null)).isTrue(); setServerProperty(ORCHESTRATOR, "sonar.forceAuthentication", "true"); - assertThat(checkAuthenticationThroughWebService("admin", "admin")).isTrue(); - assertThat(checkAuthenticationThroughWebService("wrong", "admin")).isFalse(); - assertThat(checkAuthenticationThroughWebService("admin", "wrong")).isFalse(); - assertThat(checkAuthenticationThroughWebService(null, null)).isFalse(); + assertThat(checkAuthenticationWithAuthenticateWebService("admin", "admin")).isTrue(); + assertThat(checkAuthenticationWithAuthenticateWebService("wrong", "admin")).isFalse(); + assertThat(checkAuthenticationWithAuthenticateWebService("admin", "wrong")).isFalse(); + assertThat(checkAuthenticationWithAuthenticateWebService(null, null)).isFalse(); } - private boolean checkAuthenticationThroughWebService(String login, String password) { + /** + * SONAR-7640 + */ + @Test + public void authentication_with_any_ws() throws Exception { + assertThat(checkAuthenticationWithAnyWebService("admin", "admin").code()).isEqualTo(200); + assertThat(checkAuthenticationWithAnyWebService("wrong", "admin").code()).isEqualTo(401); + assertThat(checkAuthenticationWithAnyWebService("admin", "wrong").code()).isEqualTo(401); + assertThat(checkAuthenticationWithAnyWebService("admin", null).code()).isEqualTo(401); + assertThat(checkAuthenticationWithAnyWebService(null, null).code()).isEqualTo(200); + + setServerProperty(ORCHESTRATOR, "sonar.forceAuthentication", "true"); + + assertThat(checkAuthenticationWithAnyWebService("admin", "admin").code()).isEqualTo(200); + assertThat(checkAuthenticationWithAnyWebService("wrong", "admin").code()).isEqualTo(401); + assertThat(checkAuthenticationWithAnyWebService("admin", "wrong").code()).isEqualTo(401); + assertThat(checkAuthenticationWithAnyWebService("admin", null).code()).isEqualTo(401); + assertThat(checkAuthenticationWithAnyWebService(null, null).code()).isEqualTo(401); + } + + private boolean checkAuthenticationWithAuthenticateWebService(String login, String password) { String result = ORCHESTRATOR.getServer().wsClient(login, password).get("/api/authentication/validate"); return result.contains("{\"valid\":true}"); } + private WsResponse checkAuthenticationWithAnyWebService(String login, String password) { + WsClient wsClient = WsClientFactories.getDefault().newClient(HttpConnector.newBuilder().url(ORCHESTRATOR.getServer().getUrl()).credentials(login, password).build()); + // Call any WS + return wsClient.wsConnector().call(new GetRequest("api/rules/search")); + } + private static void addUserPermission(String login, String permission) { adminWsClient.permissions().addUser(new AddUserWsRequest() .setLogin(login) diff --git a/it/it-tests/src/test/java/it/user/RailsExternalAuthenticationTest.java b/it/it-tests/src/test/java/it/user/RailsExternalAuthenticationTest.java index d25c0c0b4e7..fd7280fef4d 100644 --- a/it/it-tests/src/test/java/it/user/RailsExternalAuthenticationTest.java +++ b/it/it-tests/src/test/java/it/user/RailsExternalAuthenticationTest.java @@ -43,9 +43,16 @@ import org.sonar.wsclient.services.AuthenticationQuery; import org.sonar.wsclient.services.UserPropertyCreateQuery; import org.sonar.wsclient.services.UserPropertyQuery; import org.sonar.wsclient.user.UserParameters; +import org.sonarqube.ws.client.GetRequest; +import org.sonarqube.ws.client.HttpConnector; +import org.sonarqube.ws.client.WsClient; +import org.sonarqube.ws.client.WsClientFactories; +import org.sonarqube.ws.client.WsResponse; import util.QaOnly; import util.selenium.SeleneseTest; +import static java.net.HttpURLConnection.HTTP_OK; +import static java.net.HttpURLConnection.HTTP_UNAUTHORIZED; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.fail; import static util.ItUtils.pluginArtifact; @@ -340,6 +347,35 @@ public class RailsExternalAuthenticationTest { } } + /** + * SONAR-7640 + */ + @Test + public void authentication_with_ws() throws Exception { + // Given clean Sonar installation and no users in external system + String login = USER_LOGIN; + String password = "1234567"; + Map users = Maps.newHashMap(); + + // When user created in external system + users.put(login + ".password", password); + updateUsersInExtAuth(users); + + assertThat(checkAuthenticationWithWebService(login, password).code()).isEqualTo(HTTP_OK); + assertThat(checkAuthenticationWithWebService("wrong", password).code()).isEqualTo(HTTP_UNAUTHORIZED); + assertThat(checkAuthenticationWithWebService(login, "wrong").code()).isEqualTo(HTTP_UNAUTHORIZED); + assertThat(checkAuthenticationWithWebService(login, null).code()).isEqualTo(HTTP_UNAUTHORIZED); + assertThat(checkAuthenticationWithWebService(null, null).code()).isEqualTo(HTTP_OK); + + setServerProperty(orchestrator, "sonar.forceAuthentication", "true"); + + assertThat(checkAuthenticationWithWebService(login, password).code()).isEqualTo(HTTP_OK); + assertThat(checkAuthenticationWithWebService("wrong", password).code()).isEqualTo(HTTP_UNAUTHORIZED); + assertThat(checkAuthenticationWithWebService(login, "wrong").code()).isEqualTo(HTTP_UNAUTHORIZED); + assertThat(checkAuthenticationWithWebService(login, null).code()).isEqualTo(HTTP_UNAUTHORIZED); + assertThat(checkAuthenticationWithWebService(null, null).code()).isEqualTo(HTTP_UNAUTHORIZED); + } + protected void verifyHttpException(Exception e, int expectedCode) { assertThat(e).isInstanceOf(HttpException.class); HttpException exception = (HttpException) e; @@ -408,4 +444,10 @@ public class RailsExternalAuthenticationTest { return sb.toString(); } + private WsResponse checkAuthenticationWithWebService(String login, String password) { + WsClient wsClient = WsClientFactories.getDefault().newClient(HttpConnector.newBuilder().url(orchestrator.getServer().getUrl()).credentials(login, password).build()); + // Call any WS + return wsClient.wsConnector().call(new GetRequest("api/rules/search")); + } + } diff --git a/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/api/authentication_controller.rb b/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/api/authentication_controller.rb index 5ab761d41b8..ad887ca3dae 100644 --- a/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/api/authentication_controller.rb +++ b/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/api/authentication_controller.rb @@ -18,7 +18,7 @@ # Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. # class Api::AuthenticationController < Api::ApiController - skip_before_filter :check_authentication + skip_before_filter :check_authentication, :set_user_session # prevent HTTP proxies from caching authentication status before_filter :set_cache_buster @@ -46,7 +46,11 @@ class Api::AuthenticationController < Api::ApiController private def valid? - logged_in? || (!force_authentication? && anonymous?) + begin + logged_in? || (!force_authentication? && anonymous?) + rescue Errors::AccessDenied + false + end end def force_authentication? diff --git a/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/sessions_controller.rb b/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/sessions_controller.rb index 5385d577fb1..6916ab175c6 100644 --- a/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/sessions_controller.rb +++ b/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/sessions_controller.rb @@ -38,16 +38,19 @@ class SessionsController < ApplicationController # else the original uri can be set by ApplicationController#access_denied end - self.current_user = User.authenticate(params[:login], params[:password], servlet_request) - if logged_in? - if params[:remember_me] == '1' - self.current_user.remember_me - cookies[:auth_token] = { :value => self.current_user.remember_token , :expires => self.current_user.remember_token_expires_at, :http_only => true } + begin + self.current_user = User.authenticate(params[:login], params[:password], servlet_request) + if logged_in? + if params[:remember_me] == '1' + self.current_user.remember_me + cookies[:auth_token] = { :value => self.current_user.remember_token , :expires => self.current_user.remember_token_expires_at, :http_only => true } + end + redirect_back_or_default(home_url) + else + render_unauthenticated end - redirect_back_or_default(home_url) - else - @return_to_anchor = params[:return_to_anchor] - flash.now[:loginerror] = message('session.flash_notice.authentication_failed') + rescue Errors::AccessDenied + render_unauthenticated end end @@ -56,7 +59,7 @@ class SessionsController < ApplicationController self.current_user.on_logout self.current_user.forget_me end - cookies.delete :auth_token + cookies.delete :auth_token flash[:notice]=message('session.flash_notice.logged_out') redirect_to(home_path) reset_session @@ -70,4 +73,11 @@ class SessionsController < ApplicationController end end + private + + def render_unauthenticated + @return_to_anchor = params[:return_to_anchor] + flash.now[:loginerror] = message('session.flash_notice.authentication_failed') + end + end diff --git a/server/sonar-web/src/main/webapp/WEB-INF/lib/authenticated_system.rb b/server/sonar-web/src/main/webapp/WEB-INF/lib/authenticated_system.rb index b97eff8c155..ad49b055c13 100644 --- a/server/sonar-web/src/main/webapp/WEB-INF/lib/authenticated_system.rb +++ b/server/sonar-web/src/main/webapp/WEB-INF/lib/authenticated_system.rb @@ -137,14 +137,15 @@ module AuthenticatedSystem user = User.find_active_by_login(authenticated_login.get()) if user user.token_authenticated=true - self.current_user = user - self.current_user + result = user end end else # regular Basic authentication with login and password - self.current_user = User.authenticate(login, password, servlet_request) + result = User.authenticate(login, password, servlet_request) end + raise Errors::AccessDenied unless login.blank? || result + self.current_user = result end end -- 2.39.5