Feature/reply option requires https #615
Merged
+105
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📝 WalkthroughWalkthroughThis pull request introduces a new method Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Andreass2
added
kind/enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Altinn.Correspondence.API/Models/CorrespondenceReplyOptionExt.cs (1)
25-60: Enhance validation logic for better maintainability and user experience.The validation implementation is good but could be improved in several ways:
internal class IsLinkAttribute : ValidationAttribute { + private const int MaxLength = 255; + private const string HttpsPrefix = "https://"; + private const string HttpPrefix = "http://"; + public IsLinkAttribute() { + ErrorMessage = "The {0} must be a valid HTTPS URL"; } protected override ValidationResult? IsValid(object? value, ValidationContext validationContext) { - if (value == null || value.GetType() != typeof(string)) + if (value is not string strValue) { - return new ValidationResult("LinkURL is not of type string"); + return new ValidationResult($"{validationContext.DisplayName} must be a string"); } - if (((string)value).Length > 255) + if (strValue.Length > MaxLength) { - return new ValidationResult("LinkURL is too long"); + return new ValidationResult($"{validationContext.DisplayName} must not exceed {MaxLength} characters"); } - if (!Uri.IsWellFormedUriString((string)value, UriKind.Absolute)) + if (strValue.Contains(" ")) { - return new ValidationResult("LinkURL is not a valid URL"); + return new ValidationResult($"{validationContext.DisplayName} must not contain whitespace"); } - if (((string)value).Contains(" ")) + if (!Uri.IsWellFormedUriString(strValue, UriKind.Absolute)) { - return new ValidationResult("LinkURL contains whitespace"); + return new ValidationResult($"{validationContext.DisplayName} must be a valid absolute URL"); } - if (((string)value).StartsWith("http://")) + if (strValue.StartsWith(HttpPrefix, StringComparison.OrdinalIgnoreCase)) { - return new ValidationResult("LinkURL is not secure"); + return new ValidationResult($"{validationContext.DisplayName} must use HTTPS"); } - if (!((string)value).StartsWith("https://")) + if (!strValue.StartsWith(HttpsPrefix, StringComparison.OrdinalIgnoreCase)) { - return new ValidationResult("LinkURL must start with https://"); + return new ValidationResult($"{validationContext.DisplayName} must start with {HttpsPrefix}"); } return ValidationResult.Success; } }Improvements:
- Added constants for better maintainability
- Improved error messages using DisplayName
- Optimized validation order (check whitespace before URI parsing)
- Added case-insensitive URL scheme comparison
- Used pattern matching for type checking
Test/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceInitializationTests.cs (2)
503-524: Consider adding more test cases for valid URLs.While the test is good, consider converting it to a theory to test more valid URL scenarios.
-[Fact] -public async Task IntializeCorrespondence_WithValidReplyOptions_ReturnsOK() +[Theory] +[InlineData("https://www.altinn.no", "Altinn")] +[InlineData("https://altinn.no", "Altinn")] +[InlineData("https://www.altinn.no/path?param=value", "Altinn")] +[InlineData("https://sub.altinn.no:8080/path", "Altinn")] +public async Task IntializeCorrespondence_WithValidReplyOptions_ReturnsOK(string url, string text) { // Arrange var payload = new CorrespondenceBuilder() .CreateCorrespondence() .WithReplyOptions(new List<CorrespondenceReplyOptionExt> { new CorrespondenceReplyOptionExt { - LinkURL = "https://www.altinn.no", - LinkText = "Altinn" + LinkURL = url, + LinkText = text } }) .Build();
525-560: Convert invalid URL tests to a theory for better maintainability.The test method could be refactored to use a theory for better organization and maintainability.
-[Fact] -public async Task InitializeCorrespondence_WithInvalidReplyOptions_ReturnsBadRequest() +[Theory] +[InlineData("http://www.altinn.no", "Not HTTPS")] +[InlineData("www.altinn.no", "Missing scheme")] +[InlineData("https://www.al tinn.no", "Contains whitespace")] +[InlineData("C:\\Users\\User\\Desktop", "Local path")] +[InlineData("https://altinn.no/".PadRight(256, 'a'), "Too long")] +public async Task InitializeCorrespondence_WithInvalidReplyOptions_ReturnsBadRequest(string url, string reason) { // Arrange var payload = new CorrespondenceBuilder() .CreateCorrespondence() .WithReplyOptions(new List<CorrespondenceReplyOptionExt> { new CorrespondenceReplyOptionExt { - LinkURL = "http://www.altinn.no", + LinkURL = url, LinkText = "Altinn" } }) .Build(); // Act var initializeCorrespondenceResponse = await _senderClient.PostAsJsonAsync("correspondence/api/v1/correspondence", payload); // Assert Assert.Equal(HttpStatusCode.BadRequest, initializeCorrespondenceResponse.StatusCode); - - payload.Correspondence.ReplyOptions.First().LinkURL = "www.altinn.no"; - initializeCorrespondenceResponse = await _senderClient.PostAsJsonAsync("correspondence/api/v1/correspondence", payload); - // Assert - Assert.Equal(HttpStatusCode.BadRequest, initializeCorrespondenceResponse.StatusCode); - - payload.Correspondence.ReplyOptions.First().LinkURL = "https://www.al tinn.no"; - initializeCorrespondenceResponse = await _senderClient.PostAsJsonAsync("correspondence/api/v1/correspondence", payload); - // Assert - Assert.Equal(HttpStatusCode.BadRequest, initializeCorrespondenceResponse.StatusCode); - - payload.Correspondence.ReplyOptions.First().LinkURL = "C:\\Users\\User\\Desktop"; - initializeCorrespondenceResponse = await _senderClient.PostAsJsonAsync("correspondence/api/v1/correspondence", payload); - // Assert - Assert.Equal(HttpStatusCode.BadRequest, initializeCorrespondenceResponse.StatusCode); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Test/Altinn.Correspondence.Tests/Factories/CorrespondenceBuilder.cs(1 hunks)Test/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceInitializationTests.cs(1 hunks)src/Altinn.Correspondence.API/Models/CorrespondenceReplyOptionExt.cs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test application
🔇 Additional comments (2)
src/Altinn.Correspondence.API/Models/CorrespondenceReplyOptionExt.cs (1)
15-17: LGTM! Good use of validation attributes.The [IsLink] attribute is appropriately applied to the LinkURL property, and the property is correctly marked as required.
Test/Altinn.Correspondence.Tests/Factories/CorrespondenceBuilder.cs (1)
93-97: LGTM! Clean builder method implementation.The WithReplyOptions method follows the established builder pattern and maintains consistency with other methods in the class.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/Altinn.Correspondence.API/Models/CorrespondenceReplyOptionExt.cs (5)
31-34: Remove unnecessary empty constructor.The empty constructor doesn't serve any purpose and can be removed to improve code clarity.
- public IsLinkAttribute() - { - - }
56-56: Fix string interpolation spacing.Add a space before the HTTPS prefix in the error message for better readability.
- return new ValidationResult($"LinkURL must start with{httpsPrefix}"); + return new ValidationResult($"LinkURL must start with {httpsPrefix}");
46-46: Avoid potential null reference in URI validation.The cast to string is redundant and potentially unsafe since we've already validated it's a string using pattern matching.
- if (!Uri.IsWellFormedUriString((string)value, UriKind.Absolute)) + if (!Uri.IsWellFormedUriString(strValue, UriKind.Absolute))
28-30: Optimize validation order and improve constant declarations.Consider the following improvements:
- Move constants to top of class and make them public for reusability
- Reorder validations for better performance (check length before URI validation)
- Combine HTTP/HTTPS checks into a single validation step
- private const int maxLength = 255; - private const string httpsPrefix = "https://"; - private const string httpPrefix = "http://"; + public const int MaxLength = 255; + public const string HttpsPrefix = "https://"; + public const string HttpPrefix = "http://"; protected override ValidationResult? IsValid(object? value, ValidationContext validationContext) { if (value is not string strValue) { return new ValidationResult("LinkURL is not of type string"); } - if (strValue.Length > maxLength) + if (string.IsNullOrWhiteSpace(strValue)) + { + return new ValidationResult("LinkURL cannot be empty"); + } + + if (strValue.Length > MaxLength) { - return new ValidationResult($"LinkURL must not exceed {maxLength} characters"); + return new ValidationResult($"LinkURL must not exceed {MaxLength} characters"); } - if (!Uri.IsWellFormedUriString(strValue, UriKind.Absolute)) + if (strValue.StartsWith(HttpPrefix, StringComparison.OrdinalIgnoreCase)) { - return new ValidationResult("LinkURL is not a valid URL"); + return new ValidationResult("LinkURL must use HTTPS"); } - if (strValue.StartsWith(httpPrefix, StringComparison.OrdinalIgnoreCase)) - { - return new ValidationResult("LinkURL must use HTTPS"); - } - - if (!strValue.StartsWith(httpsPrefix, StringComparison.OrdinalIgnoreCase)) + if (!strValue.StartsWith(HttpsPrefix, StringComparison.OrdinalIgnoreCase)) { - return new ValidationResult($"LinkURL must start with{httpsPrefix}"); + return new ValidationResult($"LinkURL must start with {HttpsPrefix}"); } + if (!Uri.IsWellFormedUriString(strValue, UriKind.Absolute)) + { + return new ValidationResult("LinkURL is not a valid URL"); + } return ValidationResult.Success; }Also applies to: 36-59
25-60: Consider additional URL security validations.While HTTPS validation is good, consider adding these security checks:
- Validate against localhost/internal IP addresses
- Check for common protocol exploits (javascript:, data:, file:)
- Consider validating against allowed domains list
Would you like me to provide an implementation example with these additional security checks?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Altinn.Correspondence.API/Models/CorrespondenceReplyOptionExt.cs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test application
🔇 Additional comments (1)
src/Altinn.Correspondence.API/Models/CorrespondenceReplyOptionExt.cs (1)
2-2: LGTM! Property attributes are well configured.The required modifier on LinkURL combined with the IsLink validation provides strong guarantees for URL validity.
Also applies to: 15-17
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Reply options now validates the link as valid
Verification
Documentation
Summary by CodeRabbit
New Features
Tests
Improvements