Add authorization to views

[Metal-Mighty]

Description

• Adds authorization handling to all views
• Adds a default Admin group to which the first connected user is added (unless done with API before)

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• Tests
• Other

In general, to fix a “constant condition” that arises from a null-check being effectively redundant, you should remove the unnecessary null-conditional usage and rely on a single, clear condition (or on established invariants) instead of sprinkling ?. everywhere. The goal is to preserve behavior while making the analyzer understand that the condition is meaningful, not constant.

In this specific case, the problematic line is:

<MudText Typo="Typo.h5" Class="overflow-ellipsis" Align="Align.Center">@(string.IsNullOrEmpty(EdgeDevice?.DeviceName) ? EdgeDevice?.DeviceId : EdgeDevice?.DeviceName)</MudText>

We can keep the intended logic—“show the device name if not empty; otherwise show the device ID”—without repeatedly using the null-conditional on EdgeDevice. Assuming EdgeDevice is always initialized for this form (which is consistent with typical Razor page models), we can drop the ?. on EdgeDevice itself while still being safe against DeviceName being null or empty, and against DeviceId potentially being null. That gives:

@(string.IsNullOrEmpty(EdgeDevice.DeviceName) ? EdgeDevice.DeviceId : EdgeDevice.DeviceName)

This preserves the behavior: if DeviceName is null or empty, show DeviceId; otherwise show DeviceName. The only change is removing the unnecessary null-conditionals that lead the analyzer to infer a constant nullness state. No new methods, imports, or other definitions are required; it’s a single-line edit within CreateEdgeDevicePage.razor.

In general, to fix this pattern you apply a .Where(...) filter to the sequence you’re iterating, moving the condition from inside the loop into the LINQ predicate. Then you remove the now-redundant if and its braces, keeping the loop body unchanged otherwise.

Here, we should filter PortalPermissionsHelper.GetAllPermissionStrings() so that the loop only runs for permissions where options.GetPolicy(permission) is null. The best non-invasive fix is to change line 20 to iterate over PortalPermissionsHelper.GetAllPermissionStrings().Where(permission => options.GetPolicy(permission) is null) and then remove the if/block on lines 23–27, leaving just the AddPolicy call directly in the loop body. We must add using System.Linq; at the top of the file (within the shown code) so that .Where(...) is available. No other behavior changes are needed.

Specifically:

• In src/IoTHub.Portal.Server/Security/PortalPermissions.cs, add using System.Linq; alongside existing using directives.
• Replace the foreach header and guarded body with a foreach over a Where-filtered sequence and an unguarded options.AddPolicy(...) call.

In general, to fix a “generic catch clause” issue, you should catch only the specific exception types you expect and know how to handle. Reserve truly generic catch blocks for very top-level boundaries (e.g., global exception handlers), and even then consider logging and rethrow/termination as appropriate.

For this file, the best targeted fix that preserves existing behavior is:

• Replace catch (Exception ex) with more specific catches. Given the context (an HTTP-like client service call from UI), the most likely expected exception is HttpRequestException (or potentially a custom service exception). We can:
  • First catch HttpRequestException and show the same snackbar + empty result.
  • Optionally add a second broader catch that still logs/shows the message but is clearly marked as last-resort. However, to satisfy the rule and avoid swallowing all bugs, we should avoid catching plain Exception where possible. With the information provided, we’ll narrow to HttpRequestException only.
• Add any required using (here, System.Net.Http) at the top of the Razor file if it’s not already present. Since we’re allowed to introduce imports of well-known libraries, this is safe.

Concretely:

• In src/IoTHub.Portal.Client/Pages/RBAC/UsersListPage.razor, replace lines 69–73:
  • Change catch (Exception ex) to catch (HttpRequestException ex).
• Add a @using System.Net.Http directive near the top of the Razor file so HttpRequestException is in scope.

This keeps functionality the same for typical network/service failures while no longer generically catching every possible exception.

Generally, to fix this pattern you replace an if/else that solely assigns or returns values with a single expression using the conditional (? :) operator, preserving behavior while shortening and clarifying the intent. In this specific case, we can select between SelectedPermissions.Add(permission) and SelectedPermissions.Remove(permission) via a conditional expression rather than an if/else.

Within src/IoTHub.Portal.Client/Components/Roles/EditRole.razor, in the TogglePermission method (around lines 170–176), we replace:

if (val)
    _ = SelectedPermissions.Add(permission);
else
    _ = SelectedPermissions.Remove(permission);

with a single statement that uses the ternary operator. Because both Add and Remove return a bool that is discarded, we can write:

_ = val ? SelectedPermissions.Add(permission) : SelectedPermissions.Remove(permission);

This keeps the same semantics (calling exactly one of the two methods based on val), keeps types consistent (bool on both branches), and removes the explicit if/else. No additional imports, helper methods, or definitions are needed; we only change that short region.

In general, the fix is to replace the generic catch (Exception ex) with more specific exception types that represent expected, recoverable failures (e.g., communication or domain exceptions thrown by RoleClientService), and to avoid treating things like task cancellation as an error. For unexpected exceptions, either let them bubble up or at least separate their handling so they’re not silently normalized into the same user message as ordinary API errors.

In this file, the main problematic clause is in LoadPermissions. The smallest change that improves correctness without altering user-visible behavior too much is:

• Add a dedicated catch (OperationCanceledException) (or TaskCanceledException) that simply returns, so that cancellation doesn’t trigger an error snackbar or overwrite AllPermissions.
• Keep a narrower catch for expected failures. Because we don’t see the definition of RoleClientService, we shouldn’t invent custom exception types; instead, we can:
  • Catch HttpRequestException (common for HTTP-based client services) for user-facing “load failed” messages.
  • Let other exceptions propagate, which is safer than silently converting them into “Erreur chargement permissions…”.

However, the instructions say we should not assume too much about the rest of the code and should prefer well-known libraries. HttpRequestException is part of System.Net.Http (well-known), but we also must not change imports we haven’t seen. Since this is a Razor file, using HttpRequestException without an explicit using is legal if fully qualified, but that’s noisy. To stay minimal and safe, we can at least:

• Separate OperationCanceledException (do nothing special).
• Keep a generic catch (Exception ex) but narrow the scope by:
  • Moving only RoleClientService.GetPermissions() into the try-block and doing the LINQ transform outside, or
  • Filtering out OperationCanceledException via a when-clause.

Filtering via when is the least invasive functional change and keeps the same overall behavior for all non-cancellation failures. We can write:

catch (OperationCanceledException)
{
    // Allow cancellation to propagate without showing an error or modifying permissions
    throw;
}
catch (Exception ex)
{
    Snackbar.Add(...);
    AllPermissions = Array.Empty<string>();
}

This technically still uses a generic catch, so CodeQL may still flag it. A better compromise (and still quite safe) is:

• catch (OperationCanceledException) — rethrow or just return.
• catch only domain-appropriate, well-known exception HttpRequestException for API failures.
• Let anything else bubble up.

That removes the generic catch entirely and is the clearest fix in line with the recommendation.

Concretely, in LoadPermissions (lines 151–168), we will:

1. Replace the single catch (Exception ex) with:
   • catch (OperationCanceledException) that simply returns so cancellation is not treated as an error.
   • catch (HttpRequestException ex) that shows the snackbar and sets AllPermissions to empty.
2. Leave the finally block unchanged.

No other methods in the provided snippet have a generic catch; the Save method has one too (catch (Exception ex) at line 201). For consistency and to satisfy the analyzer, we should also narrow that one similarly (e.g., HttpRequestException plus maybe OperationCanceledException), but we must be careful not to change semantics much: cancellation shouldn’t show an error; HTTP failures should.

In general, to fix a generic catch clause you identify the specific exception types that are expected from the operations in the try block and catch only those, optionally letting other, unexpected exceptions bubble up. This avoids unintentionally masking programming errors or unrelated failures.

Here, HasPermissionAsync is calling PermissionsService.GetUserPermissions(), which is likely to throw HttpRequestException for network / HTTP problems and possibly specific status codes like 401/403. The rest of the code in this class already singles out HttpRequestException with StatusCode filters in CheckAuthorizationAsync. The safest minimal change that keeps existing functional behavior (log and return false on permission-check errors) while addressing the “generic catch” issue is:

• Replace catch (Exception ex) with one or more specific catches for expected exceptions, in particular HttpRequestException.
• Optionally add separate branches for 401/403, analogous to CheckAuthorizationAsync, while still returning false (so callers see the same boolean behavior).
• Let other unexpected exceptions propagate (no generic catch) so they can be diagnosed correctly instead of being silently converted into false.

Concretely, in HasPermissionAsync (around lines 101–112) we will:

• Replace the single catch (Exception ex) with:
  • catch (HttpRequestException ex) when (ex.StatusCode == HttpStatusCode.Forbidden)
  • catch (HttpRequestException ex) when (ex.StatusCode == HttpStatusCode.Unauthorized)
  • catch (HttpRequestException ex) for other HTTP issues
• In all these branches, log the error and return false as before.
• Do not introduce a new generic catch (Exception) so we satisfy the CodeQL rule.

System.Net and System.Net.Http are already imported at the top of the file, so no new imports are required.

In general, to fix a generic catch clause, replace catch (Exception) with one or more catch blocks for specific, expected exception types, and avoid swallowing unexpected exceptions. If there is a need for a last‑resort catch, it should normally rethrow after logging, so that real bugs are not silently masked.

In this file, the main risk is in CheckAuthorizationAsync, where catch (Exception ex) logs the error and simply sets IsAuthorized = false. A better approach is:

• Keep the existing specific HttpRequestException filters for Forbidden and Unauthorized.
• Add another catch (HttpRequestException ex) without a filter to handle other HTTP‑related errors (network issues, 5xx, etc.) and treat them as “not authorized but with error”, still logging the details.
• Remove the generic catch (Exception ex) from CheckAuthorizationAsync so unexpected exceptions are not silently converted into authorization failures and can surface appropriately.

For HasPermissionAsync, the behaviour of returning false for unexpected failures might be intentional (permissions checks often choose “fail closed”), and this method is narrow in scope. Leaving its generic catch in place maintains existing functionality. The CodeQL report is about line 85, which is in CheckAuthorizationAsync, so we only change that block.

Concretely, in src/IoTHub.Portal.Client/Components/Commons/AuthorizedComponentBase.cs:

• Replace the final catch (Exception ex) in CheckAuthorizationAsync with a catch (HttpRequestException ex) that logs with more context (e.g., including StatusCode) and sets IsAuthorized = false.
• Do not change the method signature, existing logging setup, or behaviour of other methods.
• No new imports are required; HttpRequestException and HttpStatusCode are already in use.

In general, to fix a missed Where opportunity in a foreach, move the filtering logic that appears as an if guard inside the loop into a LINQ Where clause (or even directly into Any / All / FirstOrDefault etc., if the loop is only used for a boolean or search outcome). This reduces nesting, makes the predicate explicit, and improves readability.

For this specific method, UserHasPermissionAsync only needs to know whether any access control has a role whose actions contain an entry matching the requested permission (case-insensitive). Instead of looping and returning true from inside the loop, we can compute a boolean with a single LINQ Any call. Concretely, replace the foreach block (lines 178–185) and the subsequent return false (line 186) with a single statement:

return accessControls.Any(ac =>
    ac.Role?.Actions != null &&
    ac.Role.Actions.Any(a => string.Equals(a.Name, permission, StringComparison.OrdinalIgnoreCase)));

This preserves the logic exactly:

• It only returns true if there exists at least one ac for which ac.Role?.Actions != null and at least one action name equals permission ignoring case.
• It returns false otherwise, because Any returns false when no element satisfies the predicate.

No extra imports are needed: System.Linq is already imported and Any is part of LINQ. All changes are confined to UserHasPermissionAsync in src/IoTHub.Portal.Application/Services/AccessControlService.cs.

---

In general, the problem is that potentially sensitive user-specific data (user.PrincipalId) is being written to logs, which are an external storage location that may not have the same access controls as runtime data. The fix is to avoid logging sensitive identifiers directly, or to log a less sensitive surrogate (for example, a stable but non-reversible hash or a generic message without the identifier).

The minimal, non-breaking change that best addresses this is to modify the log statement on line 78 to remove user.PrincipalId from the structured log parameters. We can still keep useful diagnostic information (such as the count of permissions) without including a unique identifier for the user. Given the constraints, we should not introduce new crypto helpers or complex hashing logic; the simplest and safest approach is to log only non-sensitive information. Concretely, in GetMyPermissions, replace:

this.logger.LogInformation("User with principal ID {PrincipalId} has {Count} permissions", user.PrincipalId, userPermissions.Count);

with something like:

this.logger.LogInformation("Current authenticated user has {Count} permissions", userPermissions.Count);

This preserves logging of authorization behavior without exposing the principal ID. No new imports, methods, or definitions are needed; only the one log line in PermissionsController.GetMyPermissions needs to be changed.

[codecov]

Codecov Report

❌ Patch coverage is 49.93252% with 742 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.04%. Comparing base (bde6186) to head (df51682).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...THub.Portal.Client/Components/Roles/EditRole.razor	0.00%	140 Missing ⚠️
...THub.Portal.Client/Pages/RBAC/CreateRolePage.razor	0.00%	103 Missing ⚠️
...THub.Portal.Client/Pages/RBAC/UserDetailPage.razor	0.00%	91 Missing ⚠️
...oTHub.Portal.Client/Pages/RBAC/RolesListPage.razor	0.00%	52 Missing ⚠️
...b.Portal.Shared/Extensions/PermissionsExtension.cs	0.00%	41 Missing ⚠️
...l.Server/Controllers/v1.0/PermissionsController.cs	0.00%	33 Missing ⚠️
...oTHub.Portal.Client/Pages/RBAC/UsersListPage.razor	0.00%	32 Missing ⚠️
...Portal.Server/Security/RBACAuthorizationHandler.cs	0.00%	27 Missing ⚠️
...onfigurations/CreateDeviceConfigurationsPage.razor	37.14%	19 Missing and 3 partials ⚠️
...ient/Components/Commons/AuthorizedComponentBase.cs	63.15%	19 Missing and 2 partials ⚠️
... and 30 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3284      +/-   ##
==========================================
- Coverage   82.92%   81.04%   -1.89%     
==========================================
  Files         372      375       +3     
  Lines       13752    14195     +443     
  Branches     1072     1185     +113     
==========================================
+ Hits        11404    11504     +100     
- Misses       2019     2353     +334     
- Partials      329      338       +9     

☔ 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.