feat: Implementation of TextController for org level #14583
Conversation
Konrad-Simso
added
solution/studio/designer
📝 WalkthroughWalkthroughThis pull request introduces functionality for managing organization-specific Git repositories and text resources. It adds a new method in the Git repository factory and corresponding interface updates for creating organization-specific repositories. A new Git repository class is implemented to handle language-specific text resources. Additionally, an organization text service and its interface are added alongside new API controller endpoints for retrieving, creating, and updating text resources. Service registration changes and minor documentation updates are also included. Changes
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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 (
|
github-actions
bot
added
the
area/text-editor
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Konrad-Simso
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
backend/src/Designer/Services/Interfaces/Organisation/IOrgTextsService.cs (2)
31-40: Complete the documentation for UpdateTextsForKeys method.The XML documentation for the UpdateTextsForKeys method is incomplete. The summary section should explain what values are being updated.
/// <summary> - /// Updates values for + /// Updates values for specified keys in the text resource. /// </summary>
18-40: Add return value documentation and cancellation token support.
- Add XML documentation for return values in SaveText and UpdateTextsForKeys methods.
- Consider adding CancellationToken support for these potentially long-running operations, similar to how GetText might be implemented.
/// <param name="textResource">The text resource to be saved</param> /// <param name="languageCode">LanguageCode</param> - /// <returns></returns> + /// <returns>A task that represents the asynchronous operation</returns> + /// <param name="cancellationToken">A token to cancel the operation</param> - public Task SaveText(string org, string repo, string developer, TextResource textResource, string languageCode); + public Task SaveText(string org, string repo, string developer, TextResource textResource, string languageCode, CancellationToken cancellationToken = default); /// <summary> /// Updates values for specified keys in the text resource. /// </summary> /// <param name="org">Organisation</param> /// <param name="repo">Repository</param> /// <param name="developer">Username of developer</param> /// <param name="keysTexts">KeysTexts</param> /// <param name="languageCode">LanguageCode</param> - /// <returns></returns> + /// <returns>A task that represents the asynchronous operation</returns> + /// <param name="cancellationToken">A token to cancel the operation</param> - public Task UpdateTextsForKeys(string org, string repo, string developer, Dictionary<string, string> keysTexts, string languageCode); + public Task UpdateTextsForKeys(string org, string repo, string developer, Dictionary<string, string> keysTexts, string languageCode, CancellationToken cancellationToken = default);backend/src/Designer/Services/Interfaces/IAltinnGitRepositoryFactory.cs (1)
29-29: Fix typo in parameter documentation.There's a typo in the parameter documentation for "org".
- /// <param name="org">The organization owning the repository identfied by it's short name as defined in Gitea.</param> + /// <param name="org">The organization owning the repository identified by its short name as defined in Gitea.</param>backend/src/Designer/Infrastructure/GitRepository/AltinnOrgGitRepository.cs (1)
26-28: Fix incorrect XML documentation reference.The XML documentation references the wrong class.
/// <summary> - /// Initializes a new instance of the <see cref="AltinnGitRepository"/> class. + /// Initializes a new instance of the <see cref="AltinnOrgGitRepository"/> class. /// </summary>backend/src/Designer/Controllers/Organisation/TextController.cs (1)
16-31: Consider adding API versioning.The controller structure is good, but consider adding API versioning to future-proof the endpoints.
[ApiController] [Authorize] +[ApiVersion("1.0")] [Route("designer/api/{org}/text")] public class TextController : ControllerBase
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/src/Designer/Controllers/Organisation/TextController.cs(1 hunks)backend/src/Designer/Factories/AltinnGitRepositoryFactory.cs(1 hunks)backend/src/Designer/Infrastructure/GitRepository/AltinnOrgGitRepository.cs(1 hunks)backend/src/Designer/Infrastructure/ServiceRegistration.cs(2 hunks)backend/src/Designer/Services/Implementation/Organisation/OrgTextsService.cs(1 hunks)backend/src/Designer/Services/Interfaces/IAltinnGitRepositoryFactory.cs(1 hunks)backend/src/Designer/Services/Interfaces/Organisation/IOrgTextsService.cs(1 hunks)
🧰 Additional context used
📓 Learnings (1)
backend/src/Designer/Controllers/Organisation/TextController.cs (1)
Learnt from: Konrad-Simso
PR: Altinn/altinn-studio#14458
File: backend/src/Designer/Controllers/TextController.cs:24-24
Timestamp: 2025-01-20T12:22:42.661Z
Learning: TextsController in backend/src/Designer/Controllers/TextsController.cs is planned to be removed in PR #14418, making any modifications to it unnecessary.
🔇 Additional comments (4)
backend/src/Designer/Infrastructure/GitRepository/AltinnOrgGitRepository.cs (1)
17-24: Review JSON serialization security settings.The use of
UnsafeRelaxedJsonEscapingcould potentially lead to security issues if the text resources contain malicious content. Consider using the default encoder unless there's a specific requirement for unsafe escaping.✅ Verification successful
🌐 Web query:
What are the security implications of using UnsafeRelaxedJsonEscaping in System.Text.Json?💡 Result:
Using `UnsafeRelaxedJsonEscaping` in `System.Text.Json` introduces several security considerations due to its permissive encoding behavior. Here’s a breakdown of the implications: ## Security Risks 1. **Cross-Site Scripting (XSS) Vulnerabilities** - The encoder **does not escape HTML-sensitive characters** like `<`, `>`, `&`, or `'`[1][7]. If JSON output is mistakenly embedded in HTML or `<script>` tags without proper escaping, attackers could inject malicious scripts[1][7]. - Example: A payload like `{"userInput": "<script>alert('xss')</script>"}` would pass through unescaped[1]. 2. **Information Disclosure** - Allows non-ASCII characters (via `UnicodeRanges.All`) and symbols like `+` to pass unescaped[7]. If clients misinterpret encoding (e.g., using a non-UTF-8 charset), this could lead to data corruption or unintended data exposure[1][7]. 3. **Dependency on Client Validation** - Safe usage requires clients to explicitly interpret payloads as UTF-8 JSON (e.g., via `Content-Type: application/json; charset=utf-8`). Misconfigured clients risk parsing errors or vulnerabilities[1][7]. ## Mitigation Guidelines - **Use Cases**: Only apply `UnsafeRelaxedJsonEscaping` when: - Output is **not embedded in HTML/XML**. - Clients are guaranteed to process payloads as UTF-8 JSON[1][7]. - **Alternatives**: Prefer default encoding for most scenarios, as it escapes HTML-sensitive characters and non-ASCII symbols by default[8]. ## Additional Considerations - A separate `System.Text.Json` vulnerability (CVE-2024-43485) highlights risks in deserialization logic (e.g., DoS via `[ExtensionData]`), but this is unrelated to `UnsafeRelaxedJsonEscaping`[2]. Always update to patched versions (e.g., 8.0.10+) for general security fixes[2]. In summary, while `UnsafeRelaxedJsonEscaping` improves performance by reducing escaping, it shifts responsibility for security onto developers. Ensure strict client validation and avoid exposing outputs to contexts where HTML/script injection is possible[1][7][8]. Citations: - 1: https://learn.microsoft.com/vi-vn/dotnet/standard/serialization/system-text-json/character-encoding - 2: https://vulert.com/vuln-db/CVE-2024-43485 - 3: https://www.nuget.org/packages/System.Text.Json/8.0.1 - 4: https://github.com/dotnet/runtime/issues/86463 - 5: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.Json/docs/ThreatModel.md - 6: https://app.studyraid.com/en/read/12321/397657/api-authentication-with-json - 7: https://learn.microsoft.com/nb-no/dotnet/api/system.text.encodings.web.javascriptencoder.unsaferelaxedjsonescaping?view=net-8.0 - 8: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/migrate-from-newtonsoft?WT.mc_id=blog-docs-shboyer - 9: https://cloudguard.ai/resources/powershell-json/ - 10: https://learn.microsoft.com/en-us/answers/questions/1820671/system-text-json-is-vulnerable
Action: Revisit Usage of UnsafeRelaxedJsonEscaping in JSON Serialization
The use of
UnsafeRelaxedJsonEscapingdoes introduce potential security risks, notably around XSS vulnerabilities, since it does not escape HTML-sensitive characters. Unless there is a specific rationale for its use (e.g., performance considerations in a controlled context where JSON output is never embedded in HTML), it is generally safer to use the default encoder.
- XSS Risk: HTML characters such as
<,>, and&are not escaped. If JSON data is embedded in an HTML context, this can lead to script injection.- Information Disclosure: Permissive escaping can also lead to scenarios where unexpected characters pass through unescaped, increasing risk.
- Recommendation: Confirm that relaxed escaping is strictly necessary; otherwise, revert to the default secure encoding settings to mitigate potential vulnerabilities.
backend/src/Designer/Factories/AltinnGitRepositoryFactory.cs (1)
70-74: LGTM!The implementation follows the established pattern and correctly creates the AltinnOrgGitRepository instance.
backend/src/Designer/Services/Implementation/Organisation/OrgTextsService.cs (1)
12-26: LGTM! Well-structured class with proper dependency injection.The class follows good practices with constructor-based dependency injection and clear interface implementation.
backend/src/Designer/Infrastructure/ServiceRegistration.cs (1)
15-15: LGTM! Service registration follows existing patterns.The registration of
IOrgTextsServiceis consistent with other service registrations in the file.Also applies to: 19-19, 81-81
backend/src/Designer/Infrastructure/GitRepository/AltinnOrgGitRepository.cs
Outdated
Show resolved
Hide resolved
backend/src/Designer/Services/Implementation/Organisation/OrgTextsService.cs
Show resolved
Hide resolved
backend/src/Designer/Services/Implementation/Organisation/OrgTextsService.cs
Show resolved
Hide resolved
backend/src/Designer/Services/Implementation/Organisation/OrgTextsService.cs
Outdated
Show resolved
Hide resolved
backend/src/Designer/Controllers/Organisation/TextController.cs
Outdated
Show resolved
Hide resolved
backend/src/Designer/Controllers/Organisation/TextController.cs
Outdated
Show resolved
Hide resolved
…parent directory if it does not already exist
backend/src/Designer/Services/Interfaces/Organisation/IOrgTextsService.cs
Outdated
Show resolved
Hide resolved
backend/src/Designer/Services/Interfaces/Organisation/IOrgTextsService.cs
Outdated
Show resolved
Hide resolved
backend/src/Designer/Services/Interfaces/IAltinnGitRepositoryFactory.cs
Outdated
Show resolved
Hide resolved
backend/src/Designer/Services/Interfaces/IAltinnGitRepositoryFactory.cs
Outdated
Show resolved
Hide resolved
backend/src/Designer/Controllers/Organisation/TextController.cs
Outdated
Show resolved
Hide resolved
backend/src/Designer/Controllers/Organisation/TextController.cs
Outdated
Show resolved
Hide resolved
backend/src/Designer/Infrastructure/GitRepository/AltinnAppGitRepository.cs
Show resolved
Hide resolved
backend/src/Designer/Infrastructure/GitRepository/AltinnOrgGitRepository.cs
Outdated
Show resolved
Hide resolved
backend/src/Designer/Infrastructure/GitRepository/AltinnOrgGitRepository.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/src/Designer/Controllers/Organisation/OrgTextController.cs (1)
21-23: Consider externalizing the "content" repository name.Storing
"content"as a constant is acceptable if it’s universal across the system. However, if there's any chance it could change or differ by environment, externalizing it (e.g., in configuration) would increase flexibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/src/Designer/Controllers/Organisation/OrgTextController.cs(1 hunks)backend/src/Designer/Infrastructure/GitRepository/AltinnOrgGitRepository.cs(1 hunks)backend/src/Designer/Services/Interfaces/IAltinnGitRepositoryFactory.cs(1 hunks)backend/src/Designer/Services/Interfaces/Organisation/IOrgTextsService.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/src/Designer/Services/Interfaces/IAltinnGitRepositoryFactory.cs
- backend/src/Designer/Infrastructure/GitRepository/AltinnOrgGitRepository.cs
- backend/src/Designer/Services/Interfaces/Organisation/IOrgTextsService.cs
🧰 Additional context used
🧠 Learnings (1)
backend/src/Designer/Controllers/Organisation/OrgTextController.cs (1)
Learnt from: Konrad-Simso
PR: Altinn/altinn-studio#14583
File: backend/src/Designer/Services/Implementation/Organisation/OrgTextsService.cs:65-94
Timestamp: 2025-02-06T11:20:54.269Z
Learning: In the Altinn Studio Designer, the UpdateTextsForKeys method in OrgTextsService can only be called for text resources that have already been created. The resource's existence is a precondition for this method, and handling non-existent resources is unnecessary except for potential race conditions.
🔇 Additional comments (5)
backend/src/Designer/Controllers/Organisation/OrgTextController.cs (5)
13-15: Good use of class-level XML documentation.These comments provide clarity on the controller’s purpose and intent.
16-19: Consistent attribute usage for routing and authorization.Defining the route with
Route("designer/api/{org}/text")and applying[Authorize]is straightforward. Verify that the policy for authorized users is properly configured so that only permitted roles or identities can access these endpoints.
24-31: Constructor dependency injection is correctly applied.The constructor follows recommended patterns for injecting the required service (
IOrgTextsService).
33-55: Align the exception to status code mapping.Catching
NotFoundExceptionand returning a 404 is correct. For consistency, ensure the same approach is used across all relevant endpoints if they encounter a not-found resource scenario.
56-79: Ensure adequate validation of incoming text resources.While argument checks are performed, consider validating the structure of
TextResourcebefore saving. This helps detect malformed objects earlier and return a 400 with a clear error message.
backend/src/Designer/Controllers/Organisation/OrgTextController.cs
Outdated
Show resolved
Hide resolved
backend/src/Designer/Controllers/Organisation/OrgTextController.cs
Outdated
Show resolved
Hide resolved
backend/src/Designer/Services/Implementation/Organisation/OrgTextsService.cs
Outdated
Show resolved
Hide resolved
…ing application metadata.
|
Need to remake this branch based off |
|
Closing this PR as it's been remade on a different branch to be able to be merged into |
Description
Added endpoints for text resources on org level.
There are no tests for the as it will require creating a new setup for org repos. This will be done in a different PR.
Related Issue
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Chores