[PermissionService] Update authenticated user handling#3876

imnasnainaec · 2025-06-12T19:05:37Z

This change is

codecov · 2025-06-12T20:08:07Z

Codecov Report

Attention: Patch coverage is 46.66667% with 8 lines in your changes missing coverage. Please review.

Project coverage is 74.41%. Comparing base (f04266c) to head (b0838bf).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
Backend/Services/PermissionService.cs	20.00%	5 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3876      +/-   ##
==========================================
- Coverage   74.48%   74.41%   -0.07%     
==========================================
  Files         287      287              
  Lines       10672    10667       -5     
  Branches     1327     1330       +3     
==========================================
- Hits         7949     7938      -11     
- Misses       2339     2344       +5     
- Partials      384      385       +1

Flag	Coverage Δ
backend	`85.50% <46.66%> (-0.15%)`	⬇️
frontend	`66.00% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

hahn-kev

nice job, I left a few suggestions, it's mostly API design related and not security related.

I would suggest changing IsSiteAdmin so that if the user's not logged in it just returns false, where as right now it'll throw an error, that's quite unexpected. Additionally IsCurrentUserAuthenticated seems to be used a lot, even in places where we know the request came from an authenticated context, I would only expect that API to be used in places where the request was AllowAnonymous (or maybe highly sensitive areas where code accidentally calling without auth would be catastrophic).

hahn-kev · 2025-06-13T02:46:25Z

Backend/Controllers/AvatarController.cs

        public async Task<IActionResult> UploadAvatar(IFormFile? file)
        {
-            if (!_permissionService.IsCurrentUserAuthorized(HttpContext))
+            if (!_permissionService.IsCurrentUserAuthenticated(HttpContext))


reading this new name makes sense, but now it begs the question. Why does UploadAvatar require Authentication when DownloadAvatar does not? Well actually since this Controller has the Authorize attribute, the user is already authenticated. So maybe this whole branch can just go away? along with any other controllers where the request is authenticated at the controller level?

Any user can see the avatar of other users in the project, and the frontend uses the DownloadAvatar api as a direct link, so there are no permissions required.

so are you saying you can hit the DownloadAvatar API when not logged in? because I don't think that's actually the case because of the Authorize attribute on the controller.

Not when logged out. My understanding was that we add [AllowAnonymous] to specific methods whose url the frontend is using as (e.g.) an image source.

hahn-kev · 2025-06-13T02:48:28Z

Backend.Tests/Services/PermissionServiceTests.cs

-            var result = _permService.MakeJwt(user).Result;
+            const string longEnough = "0123456789abcdefghijklmnopqrstuvwxyz";
+            Environment.SetEnvironmentVariable(PermissionService.JwtSecretKeyEnv, longEnough);
+            var result = _permService.MakeJwt(_userRepo.Create(new()).Result!).Result;


might I suggest making this test function async? This line should become more readable: var result = await _permService.MakeJwt(await _userRepo.Create(new()))

None of the test in this file are async, using .Result everywhere instead. I also think making them async/await is more readable, but that's better left for another time.

hahn-kev · 2025-06-13T02:55:56Z

Backend/Controllers/UserController.cs

        {
-            if (!_permissionService.IsUserIdAuthorized(HttpContext, userId))
+            if (!_permissionService.IsUserAuthenticated(HttpContext, userId)
+                && !await _permissionService.IsSiteAdmin(HttpContext))


might I suggest a more descriptive API for permission requests? _permissionService.CanModifyUser, you pass in a UserId internally it checks if you're that user, or if you're an admin. Sure technically this API is for reading the user, but if the permissions are the same between reading and writing then it doesn't matter. Take a look at our permissions API for more Ideas. Interestingly we don't actually have a CanModifyUser permission because we have different APIs for Admins to update a user vs a user updating themselves.

I suggest this because I initially read this incorrectly and thought it would only let an admin update their own account.

Good idea! Done.

imnasnainaec

IsSiteAdmin is being removed in another pr, so I'll leave it alone here.

Since we apparently use IsCurrentUserAuthenticated unnecessarily in so many places (also noted by Tim), that's probably good for it's own pr.

Reviewable status: 0 of 11 files reviewed, 3 unresolved discussions (waiting on @hahn-kev and @imnasnainaec)

imnasnainaec · 2025-06-13T12:52:47Z

Backend/Controllers/AvatarController.cs

        public async Task<IActionResult> UploadAvatar(IFormFile? file)
        {
-            if (!_permissionService.IsCurrentUserAuthorized(HttpContext))
+            if (!_permissionService.IsCurrentUserAuthenticated(HttpContext))


Any user can see the avatar of other users in the project, and the frontend uses the DownloadAvatar api as a direct link, so there are no permissions required.

imnasnainaec · 2025-06-13T12:52:47Z

Backend/Controllers/UserController.cs

        {
-            if (!_permissionService.IsUserIdAuthorized(HttpContext, userId))
+            if (!_permissionService.IsUserAuthenticated(HttpContext, userId)
+                && !await _permissionService.IsSiteAdmin(HttpContext))


Good idea! Done.

imnasnainaec · 2025-06-13T12:52:48Z

Backend.Tests/Services/PermissionServiceTests.cs

-            var result = _permService.MakeJwt(user).Result;
+            const string longEnough = "0123456789abcdefghijklmnopqrstuvwxyz";
+            Environment.SetEnvironmentVariable(PermissionService.JwtSecretKeyEnv, longEnough);
+            var result = _permService.MakeJwt(_userRepo.Create(new()).Result!).Result;


None of the test in this file are async, using .Result everywhere instead. I also think making them async/await is more readable, but that's better left for another time.

imnasnainaec

Reviewable status: 0 of 11 files reviewed, 3 unresolved discussions (waiting on @hahn-kev and @imnasnainaec)

imnasnainaec · 2025-06-13T16:22:18Z

Backend/Controllers/AvatarController.cs

        public async Task<IActionResult> UploadAvatar(IFormFile? file)
        {
-            if (!_permissionService.IsCurrentUserAuthorized(HttpContext))
+            if (!_permissionService.IsCurrentUserAuthenticated(HttpContext))


Not when logged out. My understanding was that we add [AllowAnonymous] to specific methods whose url the frontend is using as (e.g.) an image source.

hahn-kev

looks good to me!

imnasnainaec added 2 commits June 12, 2025 14:51

[PermissionService] Update authenticated user handling

5d6e43c

Simplify mock

c61028c

imnasnainaec self-assigned this Jun 12, 2025

imnasnainaec added backend security labels Jun 12, 2025

Check authentication when getting current user id

3b59764

imnasnainaec added 6 commits June 12, 2025 16:27

Use consts and clean up mock

4fd1fa1

Fix test

cfe62d7

Use more consts

f7dc967

Merge branch 'master' into perm-service

2f233bc

Avoid ambiguity

38aeae0

Merge branch 'master' into perm-service

7a70fcc

hahn-kev suggested changes Jun 13, 2025

View reviewed changes

imnasnainaec added 2 commits June 13, 2025 08:30

Merge branch 'master' into perm-service

81ff245

Add CanModifyUser permission method

5443a5a

imnasnainaec commented Jun 13, 2025

View reviewed changes

[PermissionServiceMock] Don't make request nullable

bb6f434

imnasnainaec commented Jun 13, 2025

View reviewed changes

Merge branch 'master' into perm-service

b0838bf

hahn-kev approved these changes Jun 17, 2025

View reviewed changes

imnasnainaec merged commit 43b56a0 into master Jun 17, 2025
17 of 18 checks passed

imnasnainaec deleted the perm-service branch June 17, 2025 13:08

Uh oh!

Conversation

imnasnainaec commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hahn-kev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imnasnainaec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imnasnainaec left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hahn-kev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

imnasnainaec commented Jun 12, 2025 •

edited

Loading

codecov bot commented Jun 12, 2025 •

edited

Loading