Feature/userinfo - #273
Conversation
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #273 +/- ##
==========================================
- Coverage 41.74% 38.56% -3.19%
==========================================
Files 992 1184 +192
Lines 49697 56490 +6793
Branches 5854 6129 +275
==========================================
+ Hits 20748 21785 +1037
- Misses 28038 33721 +5683
- Partials 911 984 +73
Flags with carried forward coverage won't be shown. Click here to find out more.
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
This comment was marked as resolved.
This comment was marked as resolved.
…modal) and responds to code-quality bot userinfo recommendations
22567dc to
694ecd4
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 218 out of 220 changed files in this pull request and generated no new comments.
Suppressed comments (3)
web/Areas/Directory/Controllers/DirectoryController.cs:145
- This refactor dropped the length guard that the original inline code (and the comment just below in
AddVmacsContactInfoAsync) relied on.Nextel,LDPager, andUnitarestring[]?, and empty XML element lists deserialize as empty (length‑0) arrays rather than null. With only a!= nullcheck, indexing[0]on an empty array will throwIndexOutOfRangeException. Restore the{ Length: > 0 }guard (consistent withUserInfoService.cswhich uses?.Length > 0).
if (vm.item.Nextel != null) result.Nextel = vm.item.Nextel[0];
if (vm.item.LDPager != null) result.LDPager = vm.item.LDPager[0];
if (vm.item.Unit != null) result.Department = vm.item.Unit[0];
web/Areas/Directory/Models/IndividualSearchResultWithIDs.cs:49
LdapUserContact.PostalAddressis declared= null!and is only assigned when the LDAP entry contains apostalAddressattribute, so it can be null at runtime. Removing the null guard here meansPostalAddress.Replace(...)can throw aNullReferenceException. Note the baseIndividualSearchResultconstructor already uses the null-safe form (?.Replace(...) ?? ""); this override should match it.
PostalAddress = ldapUserContact.PostalAddress.Replace("$", '\n'.ToString());
web/Classes/Utilities/LdapService.cs:123
- The param documentation is inaccurate: this method looks up by MothraID (the filter uses
ucdpersonuuid, which maps toLdapUserContact.MothraId), not by iamID. Describing the parameter as "iamID" could lead callers to pass the wrong identifier.
| <ImplicitUsings>enable</ImplicitUsings> | ||
| <AppendRuntimeIdentifierToOutputPath>false</AppendRuntimeIdentifierToOutputPath> | ||
| <PublishDir>bin\Release\net10.0\publish\</PublishDir> | ||
| <NoWarn>$(NoWarn);NU1902;NU1608</NoWarn> |
There was a problem hiding this comment.
@JasonRobertFrancis Suppressing NU1902 silences future NuGet vulnerability advisories for the whole project, please drop it.
| } | ||
| @if (!string.IsNullOrEmpty(Model.PostalAddress)) | ||
| { | ||
| <li><strong>Address:</strong> @Html.Raw(Model.PostalAddress)</li> |
There was a problem hiding this comment.
@JasonRobertFrancis Html.Raw on LDAP data opens a possible XSS vector here. Encode before substituting the delimiter:
@Html.Raw(Html.Encode(Model.PostalAddress).Replace("$", "<br>"))
and drop the .Replace at UserInfoService.cs:267. Line 151 needs no raw at all, StudentPriorName contains no markup.
|
|
||
| try | ||
| { | ||
| var uinformService = new UinformService(); |
There was a problem hiding this comment.
@JasonRobertFrancis The unit tests are calling a live API service. UinformService uses its own static HttpClient with a hardcoded host, so the mocked IHttpClientFactory is bypassed and both GetUserInfoAsync_* tests hit ws.uinform-test.ucdavis.edu on every run. Line 260 does the same to ldap.ucdavis.edu:636 via LdapService. The catch filters hide it, which is why they still pass. Either inject these behind interfaces or give UinformService the same "skip when unconfigured" guard VMACSService has.
|
@JasonRobertFrancis Can you remove unused Student fields from the UserInfoResult model and the code that builds it? Looks like it should just be PriorName, BannerId, Status, PrimaryMajor, AllMajors, RegistrationStatus, ClassLevel and ClassOf. I don't want other developers (or AI) to see these fields and infer they are only gated by the directory permissions, when many of them are more sensitive. |
|
@coderabbitai review |
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 219 out of 221 changed files in this pull request and generated no new comments.
Suppressed comments (1)
web/Areas/Directory/Models/IndividualSearchResultWithIDs.cs:49
- This branch removes the null-guard on
PostalAddress, butLdapUserContact.PostalAddressis declared= null!and is only assigned when the LDAP entry actually contains apostalAddressattribute, so it can be null at runtime. For a contact without a postal address this will throw aNullReferenceException. Note the base classIndividualSearchResulthandles the same field null-safely (ldapUserContact.PostalAddress?.Replace(...) ?? ""), so the two paths are now inconsistent. Please restore the null-safe access here.
PostalAddress = ldapUserContact.PostalAddress.Replace("$", '\n'.ToString());
| namespace Viper.Areas.Directory.Controllers | ||
| { | ||
| [Area("Directory")] | ||
| [Permission(Allow = "SVMSecure")] |
There was a problem hiding this comment.
@JasonRobertFrancis Line 12 allows all of SVMSecure while Table.cshtml:52 gates the link on SVMSecure.userinfo, so /userinfo/{mothraID} is reachable by anyone. Please match the controller to SVMSecure.userinfo and add the role lock DirectoryController:19 has.
| if (individual != null) iamId = individual.IamId; | ||
|
|
||
| // Get user information | ||
| var userInfo = await _userInfo.GetUserInfoAsync(iamId, mothraID); |
There was a problem hiding this comment.
@JasonRobertFrancis GetUserInfoAsync runs at line 64, before the CanView* flags at 71-81, and UserInfoService.cs:113-126 populates all 11 sources unconditionally. Permissions need to gate the fetch, not just the rendering.
| </div> | ||
| } | ||
| @* Student Information *@ | ||
| @if (Model.IsStudent && Model.CanViewDirectoryDetail) |
There was a problem hiding this comment.
@JasonRobertFrancis StudentConfidentialScope is gone entirely rather than used, so the student block at line 144 renders prior name, Banner ID and majors for FERPA-blocked students. Please restore it as the gate.
| if (!string.IsNullOrEmpty(tokenResponse.AccessToken)) | ||
| { | ||
| // Cache token for slightly less than expiry time (subtract 2 hours as in CF code) | ||
| var cacheExpiry = TimeSpan.FromSeconds(tokenResponse.ExpiresIn - 7200); // 2 hours buffer |
There was a problem hiding this comment.
@JasonRobertFrancis Line 1440 goes negative whenever the token lives under two hours, and ExpiresIn is 0 when expires_in is absent. IMemoryCache.Set then throws past the filter at 1455 and 500s the page, so clamp with Math.Max.
| var apiUrl = _configuration["Instinct:ApiUrl"] ?? "https://uc-davis.api.instinctvet.com/"; | ||
| var httpClient = _httpClientFactory.CreateClient(); | ||
|
|
||
| var variablesJson = JsonSerializer.Serialize(new { name = lastName }); |
There was a problem hiding this comment.
@JasonRobertFrancis Line 1326 searches last name only and 1340 takes FirstOrDefault on first name, so same-name people get each other's Instinct account and roles. Disambiguate on username, or treat multiple matches as none.
|
@JasonRobertFrancis Three threads still open after c18d9c8:
|
rlorenzo
left a comment
rlorenzo
left a comment
There was a problem hiding this comment.
Medium and low items from the same pass, inline.
| }"; | ||
|
|
||
| // Execute GraphQL query | ||
| var apiUrl = _configuration["Instinct:ApiUrl"] ?? "https://uc-davis.api.instinctvet.com/"; |
There was a problem hiding this comment.
@JasonRobertFrancis Production URL as the fallback means a misconfigured dev or test box hits prod Instinct. Same at 1398. Better to fail fast when the setting is missing.
| { | ||
| if (HttpHelper.Settings != null) | ||
| { | ||
| optionsBuilder.UseSqlServer(HttpHelper.Settings["ConnectionStrings:Keys"]); |
There was a problem hiding this comment.
@JasonRobertFrancis Unguarded UseSqlServer here overrides the DI registration, so Program.cs:226-229's VIPER string and UseCompatibilityLevel(130) are discarded for ConnectionStrings:Keys. Same in the other three new contexts, and no other context in the repo has an OnConfiguring.
| return Ok(SessionTimeoutService.GetSessionTimeout(_viperContext)); | ||
| } | ||
|
|
||
| [Route("/GetSessionTimeout")] |
There was a problem hiding this comment.
@JasonRobertFrancis Collides with #304, which adds a dedicated SessionTimeoutController for this and also edits SessionTimeoutService. Unrelated to userinfo, worth dropping from this PR.
| @@ -1 +1,3 @@ | |||
| global using Xunit; | |||
|
|
|||
| [assembly: CollectionBehavior(DisableTestParallelization = true)] | |||
There was a problem hiding this comment.
@JasonRobertFrancis Assembly level, so this disables parallelism for the whole suite, not just the new tests.
| catch (Exception ex) when (ex is DbException || ex is InvalidOperationException) | ||
| { | ||
| // Exceptions during student info retrieval are caught and ignored to allow other directory details to load. | ||
| _logger.LogWarning(ex, "PopulateStudentInfoAsync failed"); |
There was a problem hiding this comment.
@JasonRobertFrancis 27 catch blocks in this file and none rethrow, so a SIS or UCPath outage renders a page that looks complete with sections silently missing. Worth surfacing a partial-data notice.
| public bool CanViewLoans { get; set; } | ||
| public bool CanViewInstinct { get; set; } | ||
| public bool CanViewADGroups { get; set; } | ||
| public bool IsOwnPage { get; set; } |
There was a problem hiding this comment.
@JasonRobertFrancis Never assigned, but UserInfo.cshtml:12 reads it, so it is always false. Harmless only because UserInfoController:71 folds ownPage into CanViewDirectoryDetail.
| return dt; | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
@JasonRobertFrancis Returns null for anything it cannot parse, so a bad date disappears rather than erroring. Registered globally for DateTime?, so it affects every IAM response.
| stdio: "pipe", | ||
| cwd: projectRoot, | ||
| encoding: "utf8", | ||
| shell: true, |
There was a problem hiding this comment.
@JasonRobertFrancis shell: true routes these paths through cmd.exe on Windows, so a path with a space or & breaks. npx.cmd avoids it. Same at 273.
|
@JasonRobertFrancis I was testing this on TEST: https://secure-test.vetmed.ucdavis.edu/2/UserInfo/02725606 and comparing against VIPER1: https://secure-test.vetmed.ucdavis.edu/default.cfm?page=userinfo&id=1000610632&mothraID=02725606
|
No description provided.