From b91b2032f4b4814812fd6d0a40bce8a0e85d0870 Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Thu, 23 Jun 2016 09:54:55 +0200 Subject: [PATCH] SONAR-7732 UserSession throws UnauthorizedException if null If user is not authorized (bad credentials, not authenticated when force authentication is true, etc.) the UserSession will throw an UnauthorizedException in order for rails to be able to deal with this use case (redirect to login page, render 401 in WS,etc.) --- .../server/user/AnonymousUserSession.java | 46 ------------------- .../org/sonar/server/user/DoPrivileged.java | 6 ++- .../server/user/ThreadLocalUserSession.java | 8 +++- .../authentication/ws/LoginActionTest.java | 2 +- .../server/qualitygate/QualityGatesTest.java | 28 +++++------ .../QProfileComparisonMediumTest.java | 9 +++- .../user/ThreadLocalUserSessionTest.java | 34 +++++++++----- .../app/controllers/api/java_ws_controller.rb | 1 + .../app/controllers/application_controller.rb | 6 ++- .../WEB-INF/app/views/layouts/_head.html.erb | 6 +-- .../app/views/layouts/_navbar.html.erb | 2 +- .../WEB-INF/lib/authenticated_system.rb | 14 +++++- 12 files changed, 77 insertions(+), 85 deletions(-) delete mode 100644 server/sonar-server/src/main/java/org/sonar/server/user/AnonymousUserSession.java diff --git a/server/sonar-server/src/main/java/org/sonar/server/user/AnonymousUserSession.java b/server/sonar-server/src/main/java/org/sonar/server/user/AnonymousUserSession.java deleted file mode 100644 index 985a3972133..00000000000 --- a/server/sonar-server/src/main/java/org/sonar/server/user/AnonymousUserSession.java +++ /dev/null @@ -1,46 +0,0 @@ -/* - * SonarQube - * Copyright (C) 2009-2016 SonarSource SA - * mailto:contact AT sonarsource DOT com - * - * This program is free software; you can redistribute it and/or - * modify it under the terms of the GNU Lesser General Public - * License as published by the Free Software Foundation; either - * version 3 of the License, or (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU - * Lesser General Public License for more details. - * - * You should have received a copy of the GNU Lesser General Public License - * along with this program; if not, write to the Free Software Foundation, - * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. - */ -package org.sonar.server.user; - -import java.util.Collections; -import java.util.List; - -public final class AnonymousUserSession extends AbstractUserSession { - public static final UserSession INSTANCE = new AnonymousUserSession(); - - private AnonymousUserSession() { - super(AnonymousUserSession.class); - } - - @Override - public List globalPermissions() { - return Collections.emptyList(); - } - - @Override - public boolean hasComponentPermission(String permission, String componentKey) { - return false; - } - - @Override - public boolean hasComponentUuidPermission(String permission, String componentUuid) { - return false; - } -} diff --git a/server/sonar-server/src/main/java/org/sonar/server/user/DoPrivileged.java b/server/sonar-server/src/main/java/org/sonar/server/user/DoPrivileged.java index 3742b1977e4..84687896178 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/user/DoPrivileged.java +++ b/server/sonar-server/src/main/java/org/sonar/server/user/DoPrivileged.java @@ -92,13 +92,15 @@ public final class DoPrivileged { } private void start() { - oldUserSession = threadLocalUserSession.get(); + oldUserSession = threadLocalUserSession.hasSession() ? threadLocalUserSession.get() : null; threadLocalUserSession.set(new PrivilegedUserSession().setLocale(Locale.getDefault())); } private void stop() { threadLocalUserSession.remove(); - threadLocalUserSession.set(oldUserSession); + if (oldUserSession != null) { + threadLocalUserSession.set(oldUserSession); + } } } } diff --git a/server/sonar-server/src/main/java/org/sonar/server/user/ThreadLocalUserSession.java b/server/sonar-server/src/main/java/org/sonar/server/user/ThreadLocalUserSession.java index b8f0fc4a8f4..9ec8f3c34ba 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/user/ThreadLocalUserSession.java +++ b/server/sonar-server/src/main/java/org/sonar/server/user/ThreadLocalUserSession.java @@ -19,12 +19,12 @@ */ package org.sonar.server.user; -import com.google.common.base.MoreObjects; import java.util.Collection; import java.util.List; import java.util.Locale; import java.util.Set; import javax.annotation.CheckForNull; +import org.sonar.server.exceptions.UnauthorizedException; /** * Part of the current HTTP session @@ -34,7 +34,11 @@ public class ThreadLocalUserSession implements UserSession { private static final ThreadLocal THREAD_LOCAL = new ThreadLocal<>(); public UserSession get() { - return MoreObjects.firstNonNull(THREAD_LOCAL.get(), AnonymousUserSession.INSTANCE); + UserSession currentUserSession = THREAD_LOCAL.get(); + if (currentUserSession != null) { + return currentUserSession; + } + throw new UnauthorizedException(); } public void set(UserSession session) { diff --git a/server/sonar-server/src/test/java/org/sonar/server/authentication/ws/LoginActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/authentication/ws/LoginActionTest.java index 3f6fd7c8fee..8e5c664f5b8 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/authentication/ws/LoginActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/authentication/ws/LoginActionTest.java @@ -113,7 +113,7 @@ public class LoginActionTest { executeRequest(LOGIN, PASSWORD); verify(response).setStatus(401); - assertThat(threadLocalUserSession.isLoggedIn()).isFalse(); + assertThat(threadLocalUserSession.hasSession()).isFalse(); } @Test diff --git a/server/sonar-server/src/test/java/org/sonar/server/qualitygate/QualityGatesTest.java b/server/sonar-server/src/test/java/org/sonar/server/qualitygate/QualityGatesTest.java index 6e32b15d4b3..56f401ab687 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/qualitygate/QualityGatesTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/qualitygate/QualityGatesTest.java @@ -19,6 +19,18 @@ */ package org.sonar.server.qualitygate; +import static java.util.Arrays.asList; +import static java.util.Collections.singletonList; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyLong; +import static org.mockito.Matchers.anyString; +import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import java.util.Collection; @@ -57,23 +69,11 @@ import org.sonar.db.qualitygate.QualityGateDto; import org.sonar.server.exceptions.BadRequestException; import org.sonar.server.exceptions.ForbiddenException; import org.sonar.server.exceptions.NotFoundException; +import org.sonar.server.tester.AnonymousMockUserSession; import org.sonar.server.tester.MockUserSession; import org.sonar.server.tester.UserSessionRule; -import org.sonar.server.user.AnonymousUserSession; import org.sonar.server.user.UserSession; -import static java.util.Arrays.asList; -import static java.util.Collections.singletonList; -import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Matchers.any; -import static org.mockito.Matchers.anyLong; -import static org.mockito.Matchers.anyString; -import static org.mockito.Matchers.eq; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; - @RunWith(MockitoJUnitRunner.class) public class QualityGatesTest { @@ -116,7 +116,7 @@ public class QualityGatesTest { UserSession authorizedProfileAdminUserSession = new MockUserSession("gaudol").setName("Olivier").setGlobalPermissions(GlobalPermissions.QUALITY_GATE_ADMIN); UserSession authorizedProjectAdminUserSession = new MockUserSession("gaudol").setName("Olivier").addProjectPermissions(UserRole.ADMIN, PROJECT_KEY); UserSession unauthorizedUserSession = new MockUserSession("polop").setName("Polop"); - UserSession unauthenticatedUserSession = AnonymousUserSession.INSTANCE; + UserSession unauthenticatedUserSession = new AnonymousMockUserSession(); @Before public void initialize() { diff --git a/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileComparisonMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileComparisonMediumTest.java index 34fff148da2..cb3be9900b8 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileComparisonMediumTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/qualityprofile/QProfileComparisonMediumTest.java @@ -19,11 +19,14 @@ */ package org.sonar.server.qualityprofile; +import static org.assertj.core.api.Assertions.assertThat; + import com.google.common.collect.MapDifference.ValueDifference; import org.assertj.core.data.MapEntry; import org.junit.After; import org.junit.Before; import org.junit.ClassRule; +import org.junit.Rule; import org.junit.Test; import org.sonar.api.rule.Severity; import org.sonar.api.server.rule.RuleParamType; @@ -36,14 +39,16 @@ import org.sonar.db.rule.RuleTesting; import org.sonar.server.qualityprofile.QProfileComparison.ActiveRuleDiff; import org.sonar.server.qualityprofile.QProfileComparison.QProfileComparisonResult; import org.sonar.server.tester.ServerTester; - -import static org.assertj.core.api.Assertions.assertThat; +import org.sonar.server.tester.UserSessionRule; public class QProfileComparisonMediumTest { @ClassRule public static ServerTester tester = new ServerTester().withEsIndexes().addXoo(); + @Rule + public UserSessionRule userSession = UserSessionRule.forServerTester(tester).anonymous(); + DbClient db; DbSession dbSession; RuleActivator ruleActivator; diff --git a/server/sonar-server/src/test/java/org/sonar/server/user/ThreadLocalUserSessionTest.java b/server/sonar-server/src/test/java/org/sonar/server/user/ThreadLocalUserSessionTest.java index 773c2dc23d0..8968b31b604 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/user/ThreadLocalUserSessionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/user/ThreadLocalUserSessionTest.java @@ -19,19 +19,22 @@ */ package org.sonar.server.user; +import static org.assertj.core.api.Assertions.assertThat; + import java.util.Locale; import org.junit.After; import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; +import org.sonar.server.exceptions.UnauthorizedException; +import org.sonar.server.tester.AnonymousMockUserSession; import org.sonar.server.tester.MockUserSession; -import static org.assertj.core.api.Assertions.assertThat; - public class ThreadLocalUserSessionTest { ThreadLocalUserSession threadLocalUserSession = new ThreadLocalUserSession(); + @Rule public ExpectedException thrown = ExpectedException.none(); @@ -48,9 +51,22 @@ public class ThreadLocalUserSessionTest { } @Test - public void getSession_get_anonymous_by_default() { + public void get_session_for_user() { + threadLocalUserSession.set(new MockUserSession("karadoc").setUserId(123).setLocale(Locale.FRENCH)); + UserSession session = threadLocalUserSession.get(); + assertThat(session).isNotNull(); + assertThat(session.getUserId()).isEqualTo(123); + assertThat(session.getLogin()).isEqualTo("karadoc"); + assertThat(session.isLoggedIn()).isTrue(); + assertThat(session.locale()).isEqualTo(Locale.FRENCH); + } + + @Test + public void get_session_for_anonymous() { + threadLocalUserSession.set(new AnonymousMockUserSession()); + UserSession session = threadLocalUserSession.get(); assertThat(session).isNotNull(); assertThat(session.getLogin()).isNull(); assertThat(session.getUserId()).isNull(); @@ -60,15 +76,9 @@ public class ThreadLocalUserSessionTest { } @Test - public void get_session() { - threadLocalUserSession.set(new MockUserSession("karadoc").setUserId(123).setLocale(Locale.FRENCH)); - - UserSession session = threadLocalUserSession.get(); - assertThat(session).isNotNull(); - assertThat(session.getUserId()).isEqualTo(123); - assertThat(session.getLogin()).isEqualTo("karadoc"); - assertThat(session.isLoggedIn()).isTrue(); - assertThat(session.locale()).isEqualTo(Locale.FRENCH); + public void throw_UnauthorizedException_when_no_session() { + thrown.expect(UnauthorizedException.class); + threadLocalUserSession.get(); } } diff --git a/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/api/java_ws_controller.rb b/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/api/java_ws_controller.rb index d9a159be64b..cb74a0700b3 100644 --- a/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/api/java_ws_controller.rb +++ b/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/api/java_ws_controller.rb @@ -52,6 +52,7 @@ class Api::JavaWsController < Api::ApiController (params[:wspath]=='batch' && params[:wsaction]=='index') || (params[:wspath]=='batch' && params[:wsaction]=='file') || (params[:wspath]=='api/system' && params[:wsaction]=='db_migration_status') || + (params[:wspath]=='api/system' && params[:wsaction]=='migrate_db') || (params[:wspath]=='api/system' && params[:wsaction]=='status') end diff --git a/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/application_controller.rb b/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/application_controller.rb index 83b7a6ebff8..b2e07cd7b86 100644 --- a/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/application_controller.rb +++ b/server/sonar-web/src/main/webapp/WEB-INF/app/controllers/application_controller.rb @@ -102,7 +102,11 @@ class ApplicationController < ActionController::Base end def check_authentication - access_denied if !current_user && java_facade.getConfigurationValue('sonar.forceAuthentication')=='true' + begin + current_user + rescue Java::OrgSonarServerExceptions::UnauthorizedException => ex + access_denied + end end # i18n diff --git a/server/sonar-web/src/main/webapp/WEB-INF/app/views/layouts/_head.html.erb b/server/sonar-web/src/main/webapp/WEB-INF/app/views/layouts/_head.html.erb index 66c5645c688..30673cc211b 100644 --- a/server/sonar-web/src/main/webapp/WEB-INF/app/views/layouts/_head.html.erb +++ b/server/sonar-web/src/main/webapp/WEB-INF/app/views/layouts/_head.html.erb @@ -26,9 +26,9 @@ <%# The two lines below mean that before full removal of Rails, we have to find a way to handle config properties %> window.SS = { hoursInDay: <%= configuration('sonar.technicalDebt.hoursInDay', 8) %>, - user: '<%= escape_javascript current_user.login if current_user -%>', - userName: '<%= escape_javascript current_user.name if current_user -%>', - userEmail: '<%= escape_javascript current_user.email if current_user -%>', + user: '<%= escape_javascript current_user.login if logged_in? -%>', + userName: '<%= escape_javascript current_user.name if logged_in? -%>', + userEmail: '<%= escape_javascript current_user.email if logged_in? -%>', lf: { enableGravatar: <%= configuration('sonar.lf.enableGravatar', true) %>, gravatarServerUrl: '<%= configuration('sonar.lf.gravatarServerUrl') %>' diff --git a/server/sonar-web/src/main/webapp/WEB-INF/app/views/layouts/_navbar.html.erb b/server/sonar-web/src/main/webapp/WEB-INF/app/views/layouts/_navbar.html.erb index 88a863b31d4..a99e21339f7 100644 --- a/server/sonar-web/src/main/webapp/WEB-INF/app/views/layouts/_navbar.html.erb +++ b/server/sonar-web/src/main/webapp/WEB-INF/app/views/layouts/_navbar.html.erb @@ -18,6 +18,6 @@ window.sonarqube.space = 'settings'; <% end %> - window.SS.isUserAdmin = <%= current_user && is_admin? ? 'true' : 'false' -%>; + window.SS.isUserAdmin = <%= logged_in? && is_admin? ? 'true' : 'false' -%>; })(); 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 c9be7f12c8a..83ac2b912c4 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 @@ -2,11 +2,19 @@ module AuthenticatedSystem # Returns true or false if the user is logged in. # Preloads @current_user with the user model if they're logged in. def logged_in? - !!current_user + if Java::OrgSonarServerPlatform::Platform.component(Java::OrgSonarServerUser::ThreadLocalUserSession.java_class).hasSession() + !!current_user + else + false + end end # Accesses the current user from the session. # Future calls avoid the database because nil is not equal to false. + # + # This method will generate a Java::OrgSonarServerExceptions::UnauthorizedException if user is unauthorized + # (bad credentials, not authenticated by force authentication is set to true, etc...) + # def current_user @current_user ||= login_from_java_user_session unless @current_user == false end @@ -118,6 +126,10 @@ module AuthenticatedSystem # # Called from #current_user. First attempt to login by the user id stored in the session. + # + # This method will generate a Java::OrgSonarServerExceptions::UnauthorizedException if user is unauthorized + # (bad credentials, not authenticated by force authentication is set to true, etc...) + # def login_from_java_user_session userSession = Java::OrgSonarServerPlatform::Platform.component(Java::OrgSonarServerUser::UserSession.java_class) user_id = userSession.getUserId() if userSession && userSession.isLoggedIn() -- 2.39.5