Skip to content

Commit f0fafac

Browse files
imnasnainaecimnasnainaec
authored
Tighten security on user info (#3868)
Updated Project/User Controller methods: `GetAllProjectUsers`, `GetAllUsers`, `GetUsersByFilter`, `GetUserIdByEmailOrUsername`
1 parent 16bfcc0 commit f0fafac

File tree

25 files changed

+376
-123
lines changed

25 files changed

+376
-123
lines changed

‎Backend.Tests/Controllers/UserControllerTests.cs

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -83,48 +83,48 @@ public void TestGetMissingUser()
8383
}
8484

8585
[Test]
86-
public void TestGetUserByEmailOrUsernameWithEmail()
86+
public void TestGetUserIdByEmailOrUsernameWithEmail()
8787
{
8888
const string email = "[email protected]";
8989
var user = _userRepo.Create(
9090
new User { Email = email, Username = Util.RandString(10), Password = Util.RandString(10) }
9191
).Result ?? throw new UserCreationException();
9292

93-
var result = _userController.GetUserByEmailOrUsername(email).Result;
93+
var result = _userController.GetUserIdByEmailOrUsername(email).Result;
9494
Assert.That(result, Is.InstanceOf<ObjectResult>());
95-
Assert.That(((ObjectResult)result).Value, Is.EqualTo(user).UsingPropertiesComparer());
95+
Assert.That(((ObjectResult)result).Value, Is.EqualTo(user.Id));
9696
}
9797

9898
[Test]
99-
public void TestGetUserByEmailOrUsernameWithUsername()
99+
public void TestGetUserIdByEmailOrUsernameWithUsername()
100100
{
101101
const string username = "example-name";
102102
var user = _userRepo.Create(
103103
new User { Username = username, Password = Util.RandString(10) }
104104
).Result ?? throw new UserCreationException();
105105

106-
var result = _userController.GetUserByEmailOrUsername(username).Result;
106+
var result = _userController.GetUserIdByEmailOrUsername(username).Result;
107107
Assert.That(result, Is.InstanceOf<ObjectResult>());
108-
Assert.That(((ObjectResult)result).Value, Is.EqualTo(user).UsingPropertiesComparer());
108+
Assert.That(((ObjectResult)result).Value, Is.EqualTo(user.Id));
109109
}
110110

111111
[Test]
112-
public void TestGetUserByEmailOrUsernameMissing()
112+
public void TestGetUserIdByEmailOrUsernameMissing()
113113
{
114-
var result = _userController.GetUserByEmailOrUsername("[email protected]").Result;
115-
Assert.That(result, Is.InstanceOf<NotFoundObjectResult>());
114+
var result = _userController.GetUserIdByEmailOrUsername("[email protected]").Result;
115+
Assert.That(result, Is.InstanceOf<NotFoundResult>());
116116
}
117117

118118
[Test]
119-
public void TestGetUserByEmailOrUsernameNoPermission()
119+
public void TestGetUserIdByEmailOrUsernameNoPermission()
120120
{
121121
_userController.ControllerContext.HttpContext = PermissionServiceMock.UnauthorizedHttpContext();
122122
const string email = "[email protected]";
123123
var _ = _userRepo.Create(
124124
new User { Email = email, Username = Util.RandString(10), Password = Util.RandString(10) }
125125
).Result ?? throw new UserCreationException();
126126

127-
var result = _userController.GetUserByEmailOrUsername(email).Result;
127+
var result = _userController.GetUserIdByEmailOrUsername(email).Result;
128128
Assert.That(result, Is.InstanceOf<ForbidResult>());
129129
}
130130

‎Backend.Tests/Mocks/UserRepositoryMock.cs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,15 @@ public Task<List<User>> GetAllUsers()
2222
return Task.FromResult(_users.Select(user => user.Clone()).ToList());
2323
}
2424

25+
public Task<List<User>> GetAllUsersByFilter(string filter)
26+
{
27+
var filteredUsers = _users.Where(user =>
28+
user.Name.Contains(filter, StringComparison.OrdinalIgnoreCase) ||
29+
user.Email.Contains(filter, StringComparison.OrdinalIgnoreCase) ||
30+
user.Username.Contains(filter, StringComparison.OrdinalIgnoreCase));
31+
return Task.FromResult(filteredUsers.Select(user => user.Clone()).ToList());
32+
}
33+
2534
public Task<User?> GetUser(string userId, bool sanitize = true)
2635
{
2736
try

‎Backend/Controllers/ProjectController.cs

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System.Collections.Generic;
2+
using System.Linq;
23
using System.Threading.Tasks;
34
using BackendFramework.Helper;
45
using BackendFramework.Interfaces;
@@ -41,10 +42,11 @@ public async Task<IActionResult> GetAllProjects()
4142
return Ok(await _projRepo.GetAllProjects());
4243
}
4344

44-
/// <summary> Get a list of <see cref="User"/>s of a specific project </summary>
45-
/// <returns> A list of <see cref="User"/>s </returns>
45+
/// <summary> Get all users of a specific project. </summary>
46+
/// <returns> A list of <see cref="UserStub"/>s. </returns>
4647
[HttpGet("{projectId}/users", Name = "GetAllProjectUsers")]
47-
[ProducesResponseType(StatusCodes.Status200OK, Type = typeof(List<User>))]
48+
[ProducesResponseType(StatusCodes.Status200OK, Type = typeof(List<UserStub>))]
49+
[ProducesResponseType(StatusCodes.Status403Forbidden)]
4850
public async Task<IActionResult> GetAllProjectUsers(string projectId)
4951
{
5052
if (!await _permissionService.HasProjectPermission(
@@ -54,9 +56,10 @@ public async Task<IActionResult> GetAllProjectUsers(string projectId)
5456
}
5557

5658
var allUsers = await _userRepo.GetAllUsers();
57-
var projectUsers = allUsers.FindAll(user => user.ProjectRoles.ContainsKey(projectId));
59+
var projectUsers = allUsers.FindAll(u => u.ProjectRoles.ContainsKey(projectId))
60+
.Select((u) => new UserStub(u) { RoleId = u.ProjectRoles[projectId] });
5861

59-
return Ok(projectUsers);
62+
return Ok(projectUsers.ToList());
6063
}
6164

6265
/// <summary> Deletes all <see cref="Project"/>s </summary>

‎Backend/Controllers/UserController.cs

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System.Collections.Generic;
2+
using System.Linq;
23
using System.Threading.Tasks;
34
using BackendFramework.Helper;
45
using BackendFramework.Interfaces;
@@ -105,15 +106,33 @@ public async Task<IActionResult> ResetPassword([FromBody, BindRequired] Password
105106
/// <summary> Returns all <see cref="User"/>s </summary>
106107
[HttpGet(Name = "GetAllUsers")]
107108
[ProducesResponseType(StatusCodes.Status200OK, Type = typeof(List<User>))]
109+
[ProducesResponseType(StatusCodes.Status403Forbidden)]
108110
public async Task<IActionResult> GetAllUsers()
109111
{
110-
if (string.IsNullOrEmpty(_permissionService.GetUserId(HttpContext)))
112+
if (!await _permissionService.IsSiteAdmin(HttpContext))
111113
{
112114
return Forbid();
113115
}
114116
return Ok(await _userRepo.GetAllUsers());
115117
}
116118

119+
/// <summary> Gets all users with email, name, or username matching the given filter. </summary>
120+
/// <remarks> Only site admins can use filters shorter than 3 characters long. </remarks>
121+
/// <returns> A list of <see cref="UserStub"/>s. </returns>
122+
[HttpGet("filter/{filter}", Name = "GetUsersByFilter")]
123+
[ProducesResponseType(StatusCodes.Status200OK, Type = typeof(List<UserStub>))]
124+
[ProducesResponseType(StatusCodes.Status403Forbidden)]
125+
public async Task<IActionResult> GetUsersByFilter(string filter)
126+
{
127+
filter = filter.Trim();
128+
if (!await _permissionService.IsSiteAdmin(HttpContext) && filter.Length < 3)
129+
{
130+
return Forbid();
131+
}
132+
133+
return Ok((await _userRepo.GetAllUsersByFilter(filter)).Select(u => new UserStub(u)).ToList());
134+
}
135+
117136
/// <summary> Logs in a <see cref="User"/> and gives a token </summary>
118137
[AllowAnonymous]
119138
[HttpPost("authenticate", Name = "Authenticate")]
@@ -152,21 +171,20 @@ public async Task<IActionResult> GetUser(string userId)
152171
return Ok(user);
153172
}
154173

155-
/// <summary> Returns <see cref="User"/> with the specified email address or username. </summary>
156-
[HttpPut("getbyemailorusername", Name = "GetUserByEmailOrUsername")]
157-
[ProducesResponseType(StatusCodes.Status200OK, Type = typeof(User))]
158-
public async Task<IActionResult> GetUserByEmailOrUsername([FromBody, BindRequired] string emailOrUsername)
174+
/// <summary> Gets id of user with the specified email address or username. </summary>
175+
[HttpPut("getbyemailorusername", Name = "GetUserIdByEmailOrUsername")]
176+
[ProducesResponseType(StatusCodes.Status200OK, Type = typeof(string))]
177+
[ProducesResponseType(StatusCodes.Status404NotFound)]
178+
[ProducesResponseType(StatusCodes.Status403Forbidden)]
179+
public async Task<IActionResult> GetUserIdByEmailOrUsername([FromBody, BindRequired] string emailOrUsername)
159180
{
160181
if (!_permissionService.IsCurrentUserAuthorized(HttpContext))
161182
{
162183
return Forbid();
163184
}
185+
164186
var user = await _userRepo.GetUserByEmailOrUsername(emailOrUsername);
165-
if (user is null)
166-
{
167-
return NotFound(emailOrUsername);
168-
}
169-
return Ok(user);
187+
return user is null ? NotFound() : Ok(user.Id);
170188
}
171189

172190
/// <summary> Creates specified <see cref="User"/>. </summary>

‎Backend/Interfaces/IUserRepository.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ namespace BackendFramework.Interfaces
88
public interface IUserRepository
99
{
1010
Task<List<User>> GetAllUsers();
11+
Task<List<User>> GetAllUsersByFilter(string filter);
1112
Task<User?> GetUser(string userId, bool sanitize = true);
1213
Task<ResultOfUpdate> ChangePassword(string userId, string password);
1314
Task<User?> Create(User user);

‎Backend/Models/User.cs

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,31 @@ public void Sanitize()
137137
}
138138
}
139139

140+
public class UserStub
141+
{
142+
[Required]
143+
public string Id { get; set; }
144+
145+
[Required]
146+
public string Name { get; set; }
147+
148+
[Required]
149+
public string Username { get; set; }
150+
151+
[Required]
152+
public bool HasAvatar { get; set; }
153+
154+
public string? RoleId { get; set; }
155+
156+
public UserStub(User user)
157+
{
158+
Id = user.Id;
159+
Name = user.Name;
160+
Username = user.Username;
161+
HasAvatar = user.HasAvatar;
162+
}
163+
}
164+
140165
/// <summary> Contains email/username and password for authentication. </summary>
141166
/// <remarks>
142167
/// This is used in a [FromBody] serializer, so its attributes cannot be set to readonly.

‎Backend/Repositories/UserRepository.cs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
using System;
22
using System.Collections.Generic;
33
using System.Diagnostics.CodeAnalysis;
4+
using System.Linq;
45
using System.Threading.Tasks;
56
using BackendFramework.Helper;
67
using BackendFramework.Interfaces;
@@ -28,6 +29,15 @@ public async Task<List<User>> GetAllUsers()
2829
return users;
2930
}
3031

32+
/// <summary> Finds all <see cref="User"/>s matching a given filter </summary>
33+
public async Task<List<User>> GetAllUsersByFilter(string filter)
34+
{
35+
return (await GetAllUsers()).Where(u =>
36+
u.Email.Contains(filter, StringComparison.OrdinalIgnoreCase) ||
37+
u.Name.Contains(filter, StringComparison.OrdinalIgnoreCase) ||
38+
u.Username.Contains(filter, StringComparison.OrdinalIgnoreCase)).ToList();
39+
}
40+
3141
/// <summary> Removes all <see cref="User"/>s </summary>
3242
/// <returns> A bool: success of operation </returns>
3343
public async Task<bool> DeleteAllUsers()

‎src/api/.openapi-generator/FILES

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ models/user-created-project.ts
6464
models/user-edit-step-wrapper.ts
6565
models/user-edit.ts
6666
models/user-role.ts
67+
models/user-stub.ts
6768
models/user.ts
6869
models/word.ts
6970
models/words-per-day-per-user-count.ts

‎src/api/api/project-api.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,9 @@ import {
3939
// @ts-ignore
4040
import { Project } from "../models";
4141
// @ts-ignore
42-
import { User } from "../models";
43-
// @ts-ignore
4442
import { UserCreatedProject } from "../models";
43+
// @ts-ignore
44+
import { UserStub } from "../models";
4545
/**
4646
* ProjectApi - axios parameter creator
4747
* @export
@@ -551,7 +551,10 @@ export const ProjectApiFp = function (configuration?: Configuration) {
551551
projectId: string,
552552
options?: any
553553
): Promise<
554-
(axios?: AxiosInstance, basePath?: string) => AxiosPromise<Array<User>>
554+
(
555+
axios?: AxiosInstance,
556+
basePath?: string
557+
) => AxiosPromise<Array<UserStub>>
555558
> {
556559
const localVarAxiosArgs =
557560
await localVarAxiosParamCreator.getAllProjectUsers(projectId, options);
@@ -738,7 +741,7 @@ export const ProjectApiFactory = function (
738741
getAllProjectUsers(
739742
projectId: string,
740743
options?: any
741-
): AxiosPromise<Array<User>> {
744+
): AxiosPromise<Array<UserStub>> {
742745
return localVarFp
743746
.getAllProjectUsers(projectId, options)
744747
.then((request) => request(axios, basePath));

0 commit comments

Comments
 (0)