[MM-28000] Fix system manager can download export files without right permissions (#16569)

Automatic Merge

api4/apitestlib.go

@@ -57,6 +57,9 @@ type TestHelper struct {
 	SystemAdminUser   *model.User
 	tempWorkspace     string
 
+	SystemManagerClient *model.Client4
+	SystemManagerUser   *model.User
+
 	LocalClient *model.Client4
 
 	IncludeCacheLayer bool
@@ -167,6 +170,7 @@ func setupTestHelper(dbStore store.Store, searchEngine *searchengine.Broker, ent
 
 	th.Client = th.CreateClient()
 	th.SystemAdminClient = th.CreateClient()
+	th.SystemManagerClient = th.CreateClient()
 
 	// Verify handling of the supported true/false values by randomizing on each run.
 	rand.Seed(time.Now().UTC().UnixNano())
@@ -299,10 +303,11 @@ func (th *TestHelper) TearDown() {
 
 var initBasicOnce sync.Once
 var userCache struct {
-	SystemAdminUser *model.User
+	SystemAdminUser   *model.User
-	TeamAdminUser   *model.User
+	SystemManagerUser *model.User
-	BasicUser       *model.User
+	TeamAdminUser     *model.User
-	BasicUser2      *model.User
+	BasicUser         *model.User
+	BasicUser2        *model.User
 }
 
 func (th *TestHelper) InitLogin() *TestHelper {
@@ -315,6 +320,11 @@ func (th *TestHelper) InitLogin() *TestHelper {
 		th.SystemAdminUser, _ = th.App.GetUser(th.SystemAdminUser.Id)
 		userCache.SystemAdminUser = th.SystemAdminUser.DeepCopy()
 
+		th.SystemManagerUser = th.CreateUser()
+		th.App.UpdateUserRoles(th.SystemManagerUser.Id, model.SYSTEM_USER_ROLE_ID+" "+model.SYSTEM_MANAGER_ROLE_ID, false)
+		th.SystemManagerUser, _ = th.App.GetUser(th.SystemManagerUser.Id)
+		userCache.SystemManagerUser = th.SystemManagerUser.DeepCopy()
+
 		th.TeamAdminUser = th.CreateUser()
 		th.App.UpdateUserRoles(th.TeamAdminUser.Id, model.SYSTEM_USER_ROLE_ID, false)
 		th.TeamAdminUser, _ = th.App.GetUser(th.TeamAdminUser.Id)
@@ -330,22 +340,28 @@ func (th *TestHelper) InitLogin() *TestHelper {
 	})
 	// restore cached users
 	th.SystemAdminUser = userCache.SystemAdminUser.DeepCopy()
+	th.SystemManagerUser = userCache.SystemManagerUser.DeepCopy()
 	th.TeamAdminUser = userCache.TeamAdminUser.DeepCopy()
 	th.BasicUser = userCache.BasicUser.DeepCopy()
 	th.BasicUser2 = userCache.BasicUser2.DeepCopy()
-	mainHelper.GetSQLStore().GetMaster().Insert(th.SystemAdminUser, th.TeamAdminUser, th.BasicUser, th.BasicUser2)
+	mainHelper.GetSQLStore().GetMaster().Insert(th.SystemAdminUser, th.TeamAdminUser, th.BasicUser, th.BasicUser2, th.SystemManagerUser)
 	// restore non hashed password for login
 	th.SystemAdminUser.Password = "Pa$$word11"
 	th.TeamAdminUser.Password = "Pa$$word11"
 	th.BasicUser.Password = "Pa$$word11"
 	th.BasicUser2.Password = "Pa$$word11"
+	th.SystemManagerUser.Password = "Pa$$word11"
 
 	var wg sync.WaitGroup
-	wg.Add(2)
+	wg.Add(3)
 	go func() {
 		th.LoginSystemAdmin()
 		wg.Done()
 	}()
+	go func() {
+		th.LoginSystemManager()
+		wg.Done()
+	}()
 	go func() {
 		th.LoginTeamAdmin()
 		wg.Done()
@@ -420,6 +436,10 @@ func (th *TestHelper) CreateWebSocketSystemAdminClient() (*model.WebSocketClient
 	return model.NewWebSocketClient4(fmt.Sprintf("ws://localhost:%v", th.App.Srv().ListenAddr.Port), th.SystemAdminClient.AuthToken)
 }
 
+func (th *TestHelper) CreateWebSocketSystemManagerClient() (*model.WebSocketClient, *model.AppError) {
+	return model.NewWebSocketClient4(fmt.Sprintf("ws://localhost:%v", th.App.Srv().ListenAddr.Port), th.SystemManagerClient.AuthToken)
+}
+
 func (th *TestHelper) CreateWebSocketClientWithClient(client *model.Client4) (*model.WebSocketClient, *model.AppError) {
 	return model.NewWebSocketClient4(fmt.Sprintf("ws://localhost:%v", th.App.Srv().ListenAddr.Port), client.AuthToken)
 }
@@ -632,6 +652,10 @@ func (th *TestHelper) LoginSystemAdmin() {
 	th.LoginSystemAdminWithClient(th.SystemAdminClient)
 }
 
+func (th *TestHelper) LoginSystemManager() {
+	th.LoginSystemManagerWithClient(th.SystemManagerClient)
+}
+
 func (th *TestHelper) LoginBasicWithClient(client *model.Client4) {
 	utils.DisableDebugLogForTest()
 	_, resp := client.Login(th.BasicUser.Email, th.BasicUser.Password)
@@ -659,6 +683,15 @@ func (th *TestHelper) LoginTeamAdminWithClient(client *model.Client4) {
 	utils.EnableDebugLogForTest()
 }
 
+func (th *TestHelper) LoginSystemManagerWithClient(client *model.Client4) {
+	utils.DisableDebugLogForTest()
+	_, resp := client.Login(th.SystemManagerUser.Email, th.SystemManagerUser.Password)
+	if resp.Error != nil {
+		panic(resp.Error)
+	}
+	utils.EnableDebugLogForTest()
+}
+
 func (th *TestHelper) LoginSystemAdminWithClient(client *model.Client4) {
 	utils.DisableDebugLogForTest()
 	_, resp := client.Login(th.SystemAdminUser.Email, th.SystemAdminUser.Password)

api4/job.go

@@ -57,17 +57,22 @@ func downloadJob(c *Context, w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if !c.App.SessionHasPermissionTo(*c.App.Session(), model.PERMISSION_READ_JOBS) {
-		c.SetPermissionError(model.PERMISSION_READ_JOBS)
-		return
-	}
-
 	job, err := c.App.GetJob(c.Params.JobId)
 	if err != nil {
 		c.Err = err
 		return
 	}
 
+	// Currently, this endpoint only supports downloading the compliance report.
+	// If you need to download another job type, you will need to alter this section of the code to accommodate it.
+	if job.Type == model.JOB_TYPE_MESSAGE_EXPORT && !c.App.SessionHasPermissionTo(*c.App.Session(), model.PERMISSION_DOWNLOAD_COMPLIANCE_EXPORT_RESULT) {
+		c.SetPermissionError(model.PERMISSION_DOWNLOAD_COMPLIANCE_EXPORT_RESULT)
+		return
+	} else if job.Type != model.JOB_TYPE_MESSAGE_EXPORT {
+		c.Err = model.NewAppError("unableToDownloadJob", "api.job.unable_to_download_job.incorrect_job_type", nil, "", http.StatusBadRequest)
+		return
+	}
+
 	isDownloadable, _ := strconv.ParseBool(job.Data["is_downloadable"])
 	if !isDownloadable {
 		c.Err = model.NewAppError("unableToDownloadJob", "api.job.unable_to_download_job", nil, "", http.StatusBadRequest)

api4/job_test.go

@@ -19,12 +19,15 @@ func TestCreateJob(t *testing.T) {
 	defer th.TearDown()
 
 	job := &model.Job{
-		Type: model.JOB_TYPE_DATA_RETENTION,
+		Type: model.JOB_TYPE_MESSAGE_EXPORT,
 		Data: map[string]string{
 			"thing": "stuff",
 		},
 	}
 
+	_, resp := th.SystemManagerClient.CreateJob(job)
+	require.Nil(t, resp.Error)
+
 	received, resp := th.SystemAdminClient.CreateJob(job)
 	require.Nil(t, resp.Error)
 
@@ -173,6 +176,9 @@ func TestGetJobsByType(t *testing.T) {
 
 	_, resp = th.Client.GetJobsByType(jobType, 0, 60)
 	CheckForbiddenStatus(t, resp)
+
+	_, resp = th.SystemManagerClient.GetJobsByType(model.JOB_TYPE_MESSAGE_EXPORT, 0, 60)
+	require.Nil(t, resp.Error)
 }
 
 func TestDownloadJob(t *testing.T) {
@@ -196,9 +202,9 @@ func TestDownloadJob(t *testing.T) {
 		*cfg.MessageExportSettings.DownloadExportResults = true
 	})
 
-	// Normal user cannot download the results of these job (Doesn't have permission)
+	// Normal user cannot download the results of these job (non-existent job)
 	_, resp = th.Client.DownloadJob(job.Id)
-	CheckForbiddenStatus(t, resp)
+	CheckNotFoundStatus(t, resp)
 
 	// System admin trying to download the results of a non-existent job
 	_, resp = th.SystemAdminClient.DownloadJob(job.Id)
@@ -215,6 +221,14 @@ func TestDownloadJob(t *testing.T) {
 	require.Nil(t, mkdirAllErr)
 	os.Create(filePath)
 
+	// Normal user cannot download the results of these job (not the right permission)
+	_, resp = th.Client.DownloadJob(job.Id)
+	CheckForbiddenStatus(t, resp)
+
+	// System manager with default permissions cannot download the results of these job (Doesn't have correct permissions)
+	_, resp = th.SystemManagerClient.DownloadJob(job.Id)
+	CheckForbiddenStatus(t, resp)
+
 	_, resp = th.SystemAdminClient.DownloadJob(job.Id)
 	CheckBadRequestStatus(t, resp)
 
@@ -235,6 +249,24 @@ func TestDownloadJob(t *testing.T) {
 
 	_, resp = th.SystemAdminClient.DownloadJob(job.Id)
 	require.Nil(t, resp.Error)
+
+	// Here we are creating a new job which doesn't have type of message export
+	jobName = model.NewId()
+	job = &model.Job{
+		Id:   jobName,
+		Type: model.JOB_TYPE_CLOUD,
+		Data: map[string]string{
+			"export_type": "csv",
+		},
+		Status: model.JOB_STATUS_SUCCESS,
+	}
+	_, err = th.App.Srv().Store.Job().Save(job)
+	require.Nil(t, err)
+	defer th.App.Srv().Store.Job().Delete(job.Id)
+
+	// System admin shouldn't be able to download since the job type is not message export
+	_, resp = th.SystemAdminClient.DownloadJob(job.Id)
+	CheckBadRequestStatus(t, resp)
 }
 
 func TestCancelJob(t *testing.T) {

api4/user_test.go

@@ -2448,7 +2448,7 @@ func TestGetUsersNotInTeam(t *testing.T) {
 	for _, u := range rusers {
 		CheckUserSanitization(t, u)
 	}
-	require.Len(t, rusers, 1, "should be 1 user in total")
+	require.Len(t, rusers, 2, "should be 2 users in total")
 
 	rusers, resp = th.Client.GetUsersNotInTeam(teamId, 0, 60, resp.Etag)
 	CheckEtag(t, rusers, resp)
@@ -2457,7 +2457,7 @@ func TestGetUsersNotInTeam(t *testing.T) {
 	CheckNoError(t, resp)
 	require.Len(t, rusers, 1, "should be 1 per page")
 
-	rusers, resp = th.Client.GetUsersNotInTeam(teamId, 1, 1, "")
+	rusers, resp = th.Client.GetUsersNotInTeam(teamId, 2, 1, "")
 	CheckNoError(t, resp)
 	require.Empty(t, rusers, "should be no users")
 

app/permissions_migrations.go

@@ -534,6 +534,34 @@ func (a *App) getAddManageRemoteClustersPermissionsMigration() (permissionsMap,
 	}, nil
 }
 
+func (a *App) getAddDownloadComplianceExportResult() (permissionsMap, error) {
+	transformations := []permissionTransformation{}
+
+	permissionsToAddComplianceRead := []string{model.PERMISSION_DOWNLOAD_COMPLIANCE_EXPORT_RESULT.Id, model.PERMISSION_READ_JOBS.Id}
+	permissionsToAddComplianceWrite := []string{model.PERMISSION_MANAGE_JOBS.Id}
+
+	// add the new permissions to system admin
+	transformations = append(transformations,
+		permissionTransformation{
+			On:  isRole(model.SYSTEM_ADMIN_ROLE_ID),
+			Add: []string{model.PERMISSION_DOWNLOAD_COMPLIANCE_EXPORT_RESULT.Id},
+		})
+
+	// add Download Compliance Export Result and Read Jobs to all roles with sysconsole_read_compliance
+	transformations = append(transformations, permissionTransformation{
+		On:  permissionExists(model.PERMISSION_SYSCONSOLE_READ_COMPLIANCE.Id),
+		Add: permissionsToAddComplianceRead,
+	})
+
+	// add manage_jobs to all roles with sysconsole_write_compliance
+	transformations = append(transformations, permissionTransformation{
+		On:  permissionExists(model.PERMISSION_SYSCONSOLE_WRITE_COMPLIANCE.Id),
+		Add: permissionsToAddComplianceWrite,
+	})
+
+	return transformations, nil
+}
+
 // DoPermissionsMigrations execute all the permissions migrations need by the current version.
 func (a *App) DoPermissionsMigrations() error {
 	PermissionsMigrations := []struct {
@@ -557,6 +585,7 @@ func (a *App) DoPermissionsMigrations() error {
 		{Key: model.MIGRATION_KEY_ADD_MANAGE_REMOTE_CLUSTERS_PERMISSIONS, Migration: a.getAddManageRemoteClustersPermissionsMigration},
 		{Key: model.MIGRATION_KEY_ADD_SYSTEM_ROLES_PERMISSIONS, Migration: a.getSystemRolesPermissionsMigration},
 		{Key: model.MIGRATION_KEY_ADD_BILLING_PERMISSIONS, Migration: a.getBillingPermissionsMigration},
+		{Key: model.MIGRATION_KEY_ADD_DOWNLOAD_COMPLIANCE_EXPORT_RESULTS, Migration: a.getAddDownloadComplianceExportResult},
 	}
 
 	roles, err := a.GetAllRoles()

i18n/en.json

@@ -1456,6 +1456,10 @@
     "id": "api.job.unable_to_download_job",
     "translation": "Unable to download this job"
   },
+  {
+    "id": "api.job.unable_to_download_job.incorrect_job_type",
+    "translation": "The job type you are trying to download is not supported at the moment"
+  },
   {
     "id": "api.ldap_group.not_found",
     "translation": "ldap group not found"

model/migration.go

@@ -25,4 +25,5 @@ const (
 	MIGRATION_KEY_ADD_BILLING_PERMISSIONS                     = "add_billing_permissions"
 	MIGRATION_KEY_ADD_MANAGE_SHARED_CHANNEL_PERMISSIONS       = "manage_shared_channel_permissions"
 	MIGRATION_KEY_ADD_MANAGE_REMOTE_CLUSTERS_PERMISSIONS      = "manage_remote_clusters_permissions"
+	MIGRATION_KEY_ADD_DOWNLOAD_COMPLIANCE_EXPORT_RESULTS      = "download_compliance_export_results"
 )

model/permission.go

@@ -101,6 +101,7 @@ var PERMISSION_READ_OTHER_USERS_TEAMS *Permission
 var PERMISSION_EDIT_BRAND *Permission
 var PERMISSION_MANAGE_SHARED_CHANNELS *Permission
 var PERMISSION_MANAGE_REMOTE_CLUSTERS *Permission
+var PERMISSION_DOWNLOAD_COMPLIANCE_EXPORT_RESULT *Permission
 
 var PERMISSION_SYSCONSOLE_READ_ABOUT *Permission
 var PERMISSION_SYSCONSOLE_WRITE_ABOUT *Permission
@@ -536,6 +537,14 @@ func initializePermissions() {
 		"authentication.permissions.manage_remote_clusters.description",
 		PermissionScopeSystem,
 	}
+
+	PERMISSION_DOWNLOAD_COMPLIANCE_EXPORT_RESULT = &Permission{
+		"download_compliance_export_result",
+		"authentication.permissions.download_compliance_export_result.name",
+		"authentication.permissions.download_compliance_export_result.description",
+		PermissionScopeSystem,
+	}
+
 	PERMISSION_REMOVE_USER_FROM_TEAM = &Permission{
 		"remove_user_from_team",
 		"authentication.permissions.remove_user_from_team.name",
@@ -962,6 +971,7 @@ func initializePermissions() {
 		PERMISSION_EDIT_BRAND,
 		PERMISSION_MANAGE_SHARED_CHANNELS,
 		PERMISSION_MANAGE_REMOTE_CLUSTERS,
+		PERMISSION_DOWNLOAD_COMPLIANCE_EXPORT_RESULT,
 	}
 
 	TeamScopedPermissions := []*Permission{

model/role.go

@@ -61,6 +61,13 @@ func init() {
 			PERMISSION_LIST_PUBLIC_TEAMS,
 			PERMISSION_VIEW_TEAM,
 		},
+		PERMISSION_SYSCONSOLE_WRITE_COMPLIANCE.Id: {
+			PERMISSION_MANAGE_JOBS,
+		},
+		PERMISSION_SYSCONSOLE_READ_COMPLIANCE.Id: {
+			PERMISSION_READ_JOBS,
+			PERMISSION_DOWNLOAD_COMPLIANCE_EXPORT_RESULT,
+		},
 		PERMISSION_SYSCONSOLE_READ_ENVIRONMENT.Id: {
 			PERMISSION_READ_JOBS,
 		},

testlib/store.go

@@ -48,6 +48,7 @@ func GetMockStoreForSetupFunctions() *mocks.Store {
 	systemStore.On("GetByName", model.MIGRATION_KEY_ADD_CONVERT_CHANNEL_PERMISSIONS).Return(&model.System{Name: model.MIGRATION_KEY_ADD_CONVERT_CHANNEL_PERMISSIONS, Value: "true"}, nil)
 	systemStore.On("GetByName", model.MIGRATION_KEY_ADD_SYSTEM_ROLES_PERMISSIONS).Return(&model.System{Name: model.MIGRATION_KEY_ADD_SYSTEM_ROLES_PERMISSIONS, Value: "true"}, nil)
 	systemStore.On("GetByName", model.MIGRATION_KEY_ADD_BILLING_PERMISSIONS).Return(&model.System{Name: model.MIGRATION_KEY_ADD_BILLING_PERMISSIONS, Value: "true"}, nil)
+	systemStore.On("GetByName", model.MIGRATION_KEY_ADD_DOWNLOAD_COMPLIANCE_EXPORT_RESULTS).Return(&model.System{Name: model.MIGRATION_KEY_ADD_DOWNLOAD_COMPLIANCE_EXPORT_RESULTS, Value: "true"}, nil)
 	systemStore.On("GetByName", model.MIGRATION_KEY_ADD_MANAGE_SHARED_CHANNEL_PERMISSIONS).Return(&model.System{Name: model.MIGRATION_KEY_ADD_MANAGE_SHARED_CHANNEL_PERMISSIONS, Value: "true"}, nil)
 	systemStore.On("GetByName", model.MIGRATION_KEY_ADD_MANAGE_REMOTE_CLUSTERS_PERMISSIONS).Return(&model.System{Name: model.MIGRATION_KEY_ADD_MANAGE_REMOTE_CLUSTERS_PERMISSIONS, Value: "true"}, nil)
 	systemStore.On("Get").Return(make(model.StringMap), nil)