[PM-34387] Add organization invite link creation endpoint#7477
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7477 +/- ##
==========================================
+ Coverage 63.88% 63.99% +0.10%
==========================================
Files 2088 2096 +8
Lines 92383 92534 +151
Branches 8205 8220 +15
==========================================
+ Hits 59019 59214 +195
+ Misses 31338 31289 -49
- Partials 2026 2031 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
New Issues (2)Checkmarx found the following issues in this Pull Request
|
r-tome
added
the
ai-review-vnext
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the new No new findings. Prior review threads (TOCTOU on the duplicate-link check, domain format validation, duplicate migration scripts, unused Code Review DetailsNo new findings. |
| TimeProvider timeProvider) | ||
| : ICreateOrganizationInviteLinkCommand | ||
| { | ||
| private static readonly DomainNameValidatorAttribute _domainValidator = new(); |
There was a problem hiding this comment.
This is a data validation attribute designed to be applied to request models. e.g. [DomainNameValidator]. I don't think we should call it manually here. Either:
- put it on the api request model so the api handles validation, or
- extract its logic to a static class that can be called in different contexts
We have a bit of an inconsistency in where/how we do validation. I think this is OK to do at the api layer as it's simple validation on the input value rather than more complex business rules usually checked by the core layer.
There was a problem hiding this comment.
I have moved the logic over to CreateOrganizationInviteLinkRequestModel.Validate method. I couldn't use the attribute simply because its a list of domains.
There was a problem hiding this comment.
(non-blocking) 💭 You could create a generic validator that takes another validator and applies it to each item in a list. e.g. ValidateSequence<DomainNameValidatorAttribute>. Your solution is also valid.
There was a problem hiding this comment.
That is a great idea! Gave it a go
r-tome
changed the base branch from
main
to
ac/pm-35253/add-useinvitelinks-organization-ability
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
The integration test is failing because the Pricing service hasn't been deployed on QA yet. |
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
LGTM once the base PR is merged, aside from 1 question about the test.
| _factory.SubstituteService<IApplicationCacheService>(cacheService => | ||
| { | ||
| cacheService | ||
| .GetOrganizationAbilityAsync(Arg.Any<Guid>()) | ||
| .Returns(new OrganizationAbility { UseInviteLinks = true }); | ||
| }); |
There was a problem hiding this comment.
I'm interested why this is necessary - the signup helper should upsert the new org into the cache. We don't want to have to mock this behavior for every single integration test that may check the cache. Can you please look into this?
There was a problem hiding this comment.
I think this is the reason. QA pricing service hasn't updated yet so it does not return UseInviteLinks therefore it defaults to false. I just mocked it so we wouldn't depend on QA env.
| return ValidationResult.Success; | ||
| } | ||
|
|
||
| var items = ((IEnumerable)value).Cast<object>().ToList(); |
There was a problem hiding this comment.
This will throw, but maybe not with a helpful error message. I was thinking more like:
| var items = ((IEnumerable)value).Cast<object>().ToList(); | |
| if (value is not IEnumerable<object> items) | |
| { | |
| throw new ArgumentException("ValidateSequenceAttribute can only be used with an IEnumerable"); | |
| } |
| { | ||
| if (value is null) | ||
| { | ||
| return ValidationResult.Success; |
There was a problem hiding this comment.
It's not clear whether an empty list should pass validation or not. In our case it's invalid because the user is meant to supply at least 1 domain name, but it really depends on the business logic.
I think your approach is fine because it matches the behaviour of LINQ .All(). But we should mention it in the xmldoc. e.g. An empty list will always pass validation. Add additional validation if an empty list is invalid. This is our situation because your command checks it, and/or you can use minLength. It just needs to be made clear to callers.
There was a problem hiding this comment.
Clarified that in the xmldoc
…InviteLinkCommand - Introduced a new method to verify if an organization can use invite links based on its ability. - Added a new error type for cases where invite links are not available due to organizational plan restrictions. - Updated tests to cover scenarios where the organization lacks the ability to create invite links.
- Added XML summary comments to the Code property to clarify its purpose and generation method. - Explained the choice of using Guid.NewGuid for the Code to avoid predictability and ensure uniqueness.
- Added IValidatableObject implementation to CreateOrganizationInviteLinkRequestModel for domain validation. - Introduced Validate method to check the format of allowed domains and return appropriate validation results. - Updated tests to cover scenarios for invalid domain formats and mixed valid/invalid domains. - Removed redundant domain validation logic from CreateOrganizationInviteLinkCommand.
- Deleted tests for validating EncryptedInviteKey and EncryptedOrgKey as they are no longer relevant. - Cleaned up the test class to focus on current validation logic for allowed domains.
- Updated the GetAllowedDomains method to return an empty array instead of throwing a JsonException when deserialization fails. - This change improves the method's resilience by providing a default value for invalid or missing allowed domains.
- Deleted the InviteLinkInvalidDomains record as it is no longer needed. - This cleanup aligns with recent changes in domain validation logic and improves code maintainability.
…r command registration - Changed the registration of ICreateOrganizationInviteLinkCommand to use TryAddScoped instead of AddScoped.
…enceAttribute for domain validation and update unit tests for improved error handling.
…rror messaging format
…or improved type handling
…ror handling for IEnumerable types
…ts in BaseAdminConsoleController.cs
…eLinkRequestModel and implement unit test for empty AllowedDomains scenario
r-tome
force-pushed
the
ac/pm-34387/create-invite-link-command
branch
from
bae625e to
73a02de
Compare
…y method for better organization and readability
… response management
…tialization syntax for AllowedDomains so that MinLength attribute works
|
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-34387
📔 Objective
Adds a new
POST organizations/{orgId}/invite-linkendpoint that lets organization admins create a shareable invite link scoped to specific email domains.The endpoint is gated behind the generate invite link feature flag and requires the
ManageUserspermission like the existing invite users endpoint