[PM-13298] Modify members access logic #18
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 file(s) reviewed, 22 comment(s)
Edit PR Review Bot Settings | Greptile
| /// Organization member information containing a list of cipher ids | ||
| /// assigned | ||
| /// </summary> | ||
| /// <param name="orgId">Organzation Id</param> |
There was a problem hiding this comment.
syntax: Typo in 'Organzation', should be 'Organization'
| if (!await _currentContext.AccessReports(orgId)) | ||
| { | ||
| throw new NotFoundException(); | ||
| } |
There was a problem hiding this comment.
style: Consider using a more specific exception like UnauthorizedException instead of NotFoundException for permission issues
| throw new NotFoundException(); | ||
| } | ||
|
|
||
| var memberCipherDetails = await GetMemberCipherDetails(new MemberAccessCipherDetailsRequest { OrganizationId = orgId }); |
There was a problem hiding this comment.
logic: Method name clash with the public method. Rename private method to avoid confusion
| private async Task<IEnumerable<MemberAccessCipherDetails>> GetMemberCipherDetails(MemberAccessCipherDetailsRequest request) | ||
| { | ||
| var memberCipherDetails = | ||
| await _memberAccessCipherDetailsQuery.GetMemberAccessCipherDetails(request); | ||
| return memberCipherDetails; | ||
| } |
There was a problem hiding this comment.
style: This private method could be inlined as it's only called once and doesn't add much abstraction
| public Guid? UserGuid { get; set; } | ||
| public bool UsesKeyConnector { get; set; } |
There was a problem hiding this comment.
logic: UserGuid is not being set in the constructor
| var userCiphers = | ||
| report.AccessDetails | ||
| .Where(x => x.ItemCount > 0) | ||
| .SelectMany(y => y.CollectionCipherIds) | ||
| .Distinct(); | ||
| report.CipherIds = userCiphers; |
There was a problem hiding this comment.
style: This LINQ query could be optimized by combining Where and SelectMany: var userCiphers = report.AccessDetails.Where(x => x.ItemCount > 0).SelectMany(y => y.CollectionCipherIds ?? Enumerable.Empty()).Distinct();
| var distinctItems = report.AccessDetails.Where(x => x.CollectionId.HasValue).Select(x => x.CollectionId).Distinct(); | ||
| report.CollectionsCount = distinctItems.Count(); | ||
| report.GroupsCount = report.AccessDetails.Select(x => x.GroupId).Where(y => y.HasValue).Distinct().Count(); |
There was a problem hiding this comment.
style: These counts could be calculated more efficiently using a single LINQ query with grouping
| public interface IMemberAccessCipherDetailsQuery | ||
| { | ||
| Task<IEnumerable<MemberAccessCipherDetails>> GetMemberAccessCipherDetails(MemberAccessCipherDetailsRequest request); | ||
| } |
There was a problem hiding this comment.
style: Consider adding XML documentation comments to describe the purpose of the interface and method
|
|
||
| public static class ReportingServiceCollectionExtensions | ||
| { | ||
| public static void AddReportingServices(this IServiceCollection services) |
There was a problem hiding this comment.
style: consider adding XML documentation for this method
| public class MemberAccessCipherDetailsRequest | ||
| { | ||
| public Guid OrganizationId { get; set; } | ||
| } |
There was a problem hiding this comment.
style: Consider adding XML documentation comments to describe the purpose of this class and its property
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-13298
📔 Objective
An association of organization members to the ciphers they are assigned is needed. The members access report has been refactored to abstract the logic into a reusable query. The models will contain the cipher ids associated with each member. The existing members access report will continue using the same
MembersAccessReportResponseModelwhile the new endpoint for associating members to ciphers will use a new `MemberCipherDetailsResponseModel' that contains the member info along with a distinct list of assigned cipher ids.⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changesGreptile Summary
This pull request introduces changes to enhance member access logic and reporting capabilities in the Bitwarden server application. The modifications focus on associating organization members with their assigned ciphers and refactoring existing report generation.
MemberCipherDetailsResponseModelin/src/Api/Tools/Models/Response/MemberCipherDetailsResponseModel.csfor associating members with ciphersMemberAccessCipherDetailsQueryin/src/Core/Tools/ReportFeatures/MemberAccessCipherDetailsQuery.csto generate detailed access reportsReportsControllerin/src/Api/Tools/Controllers/ReportsController.csto include a new endpoint for retrieving member cipher detailsMemberAccessReportResponseModelin/src/Api/Tools/Models/Response/MemberAccessReportModel.csto useMemberAccessCipherDetailsIMemberAccessCipherDetailsQueryinterface in/src/Core/Tools/ReportFeatures/OrganizationReportMembers/Interfaces/IMemberAccessCipherDetailsQuery.csfor querying member access details