Skip to content

Authorization system#104

Merged
gitnasr merged 11 commits intodevfrom
AuthorizationSystem
Jun 29, 2025
Merged

Authorization system#104
gitnasr merged 11 commits intodevfrom
AuthorizationSystem

Conversation

@gitnasr
Copy link
Copy Markdown
Contributor

@gitnasr gitnasr commented Jun 28, 2025

Description by Korbit AI

What change is being made?

Implement a comprehensive authorization system to ensure secure access controls across various services and controllers in the application by introducing a BaseService class for centralized authorization checks.

Why are these changes being made?

These changes reinforce security by ensuring that only authorized users can perform actions such as creating, updating, or deleting sensitive resources within the application. This approach centralizes authorization logic, enhances code maintainability, and leverages role-based access controls to meet varying security requirements for different user roles, like Admin, Verified, and Partily Verified users. Additionally, this strengthens adherence to best practices in access management by ensuring proper checks before user actions are permitted.

Is this description stale? Ask me to generate a new description by commenting /korbit-generate-pr-description

Summary by CodeRabbit

  • New Features

    • Added an email property to user profiles, ensuring every user has a required email address.
  • Improvements

    • Introduced centralized authorization checks for updating and deleting assets, orders, posts, and reviews, allowing only admins or resource owners to perform these actions.
    • Applied specific authorization policies across controllers to restrict access based on user roles and verification status.
    • Enhanced user data validation by enforcing email requirements and formatting in the database.
  • Style

    • Improved code formatting for better readability in configuration files.
  • Refactor

    • Standardized and simplified authorization logic across multiple services for consistent access control.
    • Updated service methods to remove explicit user ID parameters, relying on context-based authorization instead.
    • Centralized ownership verification and authorization in a new base service class used by multiple services.
  • Chores

    • Removed a deprecated payment controller from the presentation layer.

gitnasr added 2 commits June 29, 2025 01:27
Updated AssetService, OrderService, PostService, ReviewService, and UserService to include IHttpContextAccessor for user identity access. Introduced BaseService for common functionality, requiring implementation of GetOwnerIdAsync for resource authorization. Improved method calls for authorization checks, enhanced error handling with NotFoundException, and refined code formatting for better readability.
- Updated `IUploadService` to change `DeleteAssetById` return type to `Task`.
- Added authorization checks in `AssetService`, `OrderService`, `PostService`, and `ReviewService` for admin or owner actions.
- Improved variable naming and transaction handling in `PaymentService`.
- Introduced a required `Email` property in `AppUser`.
- Enhanced formatting in `IdentityConfiguration` for consistency.
- Made `Email` property required in `UserConfiguration` with constraints.
@gitnasr gitnasr requested a review from Copilot June 28, 2025 22:47
@gitnasr gitnasr self-assigned this Jun 28, 2025
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Jun 28, 2025

Warning

Rate limit exceeded

@gitnasr has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 6 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between b9ed184 and 8eac81d.

📒 Files selected for processing (1)
  • Dentizone.Application/AutoMapper/Answers/AnswerProfile.cs (1 hunks)

Walkthrough

This update introduces a new BaseService abstract class for centralized ownership and admin authorization logic, refactors multiple service classes to inherit from it, and standardizes authorization checks for resource modification and deletion. The DeleteAssetById method in IUploadService and UploadService now returns Task instead of Task<AssetDto>. The AppUser entity gains a required Email property.

Changes

Files/Areas Changed Summary
.../Interfaces/IUploadService.cs, .../Services/UploadService.cs Changed DeleteAssetById to return Task (void async), refactored for centralized authorization, updated interface.
.../Services/BaseService.cs Introduced new abstract BaseService with shared ownership/admin authorization logic.
.../Services/AssetService.cs, .../Services/OrderService.cs, .../Services/PostService.cs, .../Services/ReviewService.cs Refactored to inherit from BaseService, added/updated GetOwnerIdAsync, centralized authorization.
.../Services/Payment/PaymentService.cs Renamed local variables for consistent camelCase, minor formatting adjustments.
.../Services/UserService.cs Simplified constructor type reference for AppDbContext.
.../Domain/Entity/AppUser.cs Added required Email property to AppUser.
.../Persistence/Configurations/UserConfiguration.cs Configured Email as required, max length 255, non-Unicode for AppUser.
.../Identity/IdentityConfiguration.cs Minor formatting and whitespace changes for clarity.
.../Interfaces/Order/IOrderService.cs Removed userId parameter from CancelOrderAsync method signature.
.../Presentation/Controllers (multiple controllers) Added or refined authorization policies on controllers and actions, restricting access with role-based policies.
.../Presentation/Controllers/PaymentController.cs Removed entire PaymentController class.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant UploadService
    participant BaseService
    participant AssetService

    Client->>UploadService: DeleteAssetById(assetId)
    UploadService->>BaseService: AuthorizeAdminOrOwnerAsync(assetId)
    BaseService->>UploadService: GetOwnerIdAsync(assetId)
    UploadService->>AssetService: DeleteAssetAsync(assetId)
    AssetService-->>UploadService: (deletion complete)
    UploadService-->>Client: (Task complete)
Loading

Possibly related PRs

  • Refactor asset management classes and interfaces #79: Modifies DeleteAssetById in IUploadService and UploadService with a userId parameter and returns Task<AssetDto>, related by changing the same method signature.
  • Image service #71: Introduces the initial IUploadService and UploadService classes, related by their shared focus on upload service design.
  • Nourleyy/post service #72: Adds DTOs, interfaces, and PostService implementation; related as this PR refactors PostService to inherit from BaseService and adds authorization logic.

Suggested labels

enhancement

Poem

A bunny hopped by with a codey delight,
"Authorization for all!" it cheered in the night.
Services inherit, ownership is checked,
Email is required—no detail is wrecked.
With refactored flows and a sequence anew,
The warren is safer for me and for you!
🐇💻✨

✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds a centralized authorization mechanism via a new BaseService and updates existing services to enforce Admin-or-owner checks, extends the AppUser entity with a required Email field, and includes minor DI and formatting tweaks.

  • Introduce BaseService for Admin/Owner authorization and integrate it into all application services
  • Add Email property to AppUser and configure it in EF Core mappings
  • Refactor constructors to inject IHttpContextAccessor, adjust method signatures, and clean up variable naming

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
Dentizone.Application/Services/BaseService.cs New abstract service implementing Admin-or-owner checks
Dentizone.Application/Services/*Service.cs Updated service constructors to inherit BaseService, added AuthorizeAdminOrOwnerAsync calls, and implemented GetOwnerIdAsync
Dentizone.Application/Interfaces/IUploadService.cs Changed DeleteAssetById to return Task instead of Task<AssetDto>
Dentizone.Infrastructure/Persistence/Configurations/UserConfiguration.cs Added EF Core configuration for Email column
Dentizone.Domain/Entity/AppUser.cs Added required Email property
Dentizone.Infrastructure/Identity/IdentityConfiguration.cs Minor formatting cleanup
Dentizone.Application/Services/Payment/PaymentService.cs Renamed DatabaseTransaction to databaseTransaction for consistency
Comments suppressed due to low confidence (1)

Dentizone.Application/Interfaces/IUploadService.cs:11

  • Changing the return type of DeleteAssetById is a breaking change for consumers; ensure dependent code is updated or versioned accordingly.
        public Task DeleteAssetById(string id, string userId);

This commit introduces a new namespace declaration
`namespace Dentizone.Application.Services;` in the
`BaseService.cs` file. This change enhances code
organization and adheres to best practices for
structuring C# applications.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

🔭 Outside diff range comments (2)
Dentizone.Application/Services/Payment/PaymentService.cs (1)

31-38: Consider adopting the BaseService pattern for authorization.

While other services in this PR have been refactored to inherit from BaseService for centralized authorization, PaymentService handles critical financial operations but lacks these authorization checks. Consider inheriting from BaseService and implementing authorization for methods like ConfirmPaymentAsync and CancelPaymentByOrderId.

Dentizone.Application/Services/PostService.cs (1)

133-142: Add authorization to other modification methods.

The DeletePost and UpdatePostStatus methods modify post resources but lack authorization checks. Consider adding AuthorizeAdminOrOwnerAsync to these methods for consistency:

 public async Task<PostViewDto> DeletePost(string postId)
 {
+    await AuthorizeAdminOrOwnerAsync(postId);
     var deletedPost = await repo.DeleteAsync(postId);
 public async Task<PostViewDto> UpdatePostStatus(string postId, PostStatus status)
 {
+    await AuthorizeAdminOrOwnerAsync(postId);
     var post = await repo.GetByIdAsync(postId);

Also applies to: 200-211

🧹 Nitpick comments (3)
Dentizone.Application/Interfaces/IUploadService.cs (1)

11-11: Method signature change looks good but consider parameter consistency.

The change from Task<AssetDto> to Task is appropriate for a delete operation. However, the userId parameter may be redundant given the new BaseService authorization pattern that extracts user information from HTTP context.

Consider updating the signature to remove the userId parameter for consistency with the new authorization approach:

-public Task DeleteAssetById(string id, string userId);
+public Task DeleteAssetById(string id);
Dentizone.Application/Services/BaseService.cs (1)

31-59: Excellent authorization design with minor improvement suggestion.

The authorization logic is well-structured and follows security best practices by checking admin privileges first, then ownership. The exception handling provides clear feedback.

Consider adding null safety for the HttpContext user:

-var currentUserId = _httpContextAccessor.HttpContext?.User.FindFirstValue(ClaimTypes.NameIdentifier);
+var currentUserId = _httpContextAccessor.HttpContext?.User?.FindFirstValue(ClaimTypes.NameIdentifier);
Dentizone.Infrastructure/Persistence/Configurations/UserConfiguration.cs (1)

20-23: Email configuration is mostly correct but reconsider Unicode setting.

The configuration properly sets the Email property as required with appropriate max length. However, the IsUnicode(false) setting may be too restrictive for modern email standards.

Consider allowing Unicode for email addresses to support international characters:

 builder.Property(u => u.Email)
     .IsRequired()
-    .HasMaxLength(255)
-    .IsUnicode(false);
+    .HasMaxLength(255);

International email addresses may contain Unicode characters in the local part or domain name according to RFC 6531.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 160fe14 and b89333d.

📒 Files selected for processing (12)
  • Dentizone.Application/Interfaces/IUploadService.cs (1 hunks)
  • Dentizone.Application/Services/AssetService.cs (2 hunks)
  • Dentizone.Application/Services/BaseService.cs (1 hunks)
  • Dentizone.Application/Services/OrderService.cs (4 hunks)
  • Dentizone.Application/Services/Payment/PaymentService.cs (5 hunks)
  • Dentizone.Application/Services/PostService.cs (10 hunks)
  • Dentizone.Application/Services/ReviewService.cs (4 hunks)
  • Dentizone.Application/Services/UploadService.cs (2 hunks)
  • Dentizone.Application/Services/UserService.cs (1 hunks)
  • Dentizone.Domain/Entity/AppUser.cs (1 hunks)
  • Dentizone.Infrastructure/Identity/IdentityConfiguration.cs (3 hunks)
  • Dentizone.Infrastructure/Persistence/Configurations/UserConfiguration.cs (1 hunks)
🧰 Additional context used
🧠 Learnings (9)
Dentizone.Application/Interfaces/IUploadService.cs (1)
Learnt from: gitnasr
PR: dentizone/api#16
File: Dentizone.Infrastructure/Persistence/Configurations/OrderConfiguration.cs:1-1
Timestamp: 2025-05-28T11:22:49.051Z
Learning: In the Dentizone.Infrastructure project, the IsDeleted property from IBaseEntity is handled through Entity Framework interceptors rather than explicit configuration in individual entity configuration files. This means entity configurations don't need to explicitly configure the IsDeleted property.
Dentizone.Infrastructure/Persistence/Configurations/UserConfiguration.cs (1)
Learnt from: gitnasr
PR: dentizone/api#16
File: Dentizone.Infrastructure/Persistence/Configurations/OrderConfiguration.cs:1-1
Timestamp: 2025-05-28T11:22:49.051Z
Learning: In the Dentizone.Infrastructure project, the IsDeleted property from IBaseEntity is handled through Entity Framework interceptors rather than explicit configuration in individual entity configuration files. This means entity configurations don't need to explicitly configure the IsDeleted property.
Dentizone.Application/Services/OrderService.cs (1)
Learnt from: gitnasr
PR: dentizone/api#16
File: Dentizone.Infrastructure/Persistence/Configurations/OrderConfiguration.cs:1-1
Timestamp: 2025-05-28T11:22:49.051Z
Learning: In the Dentizone.Infrastructure project, the IsDeleted property from IBaseEntity is handled through Entity Framework interceptors rather than explicit configuration in individual entity configuration files. This means entity configurations don't need to explicitly configure the IsDeleted property.
Dentizone.Application/Services/UserService.cs (1)
Learnt from: gitnasr
PR: dentizone/api#16
File: Dentizone.Infrastructure/Persistence/Configurations/OrderConfiguration.cs:1-1
Timestamp: 2025-05-28T11:22:49.051Z
Learning: In the Dentizone.Infrastructure project, the IsDeleted property from IBaseEntity is handled through Entity Framework interceptors rather than explicit configuration in individual entity configuration files. This means entity configurations don't need to explicitly configure the IsDeleted property.
Dentizone.Application/Services/Payment/PaymentService.cs (1)
Learnt from: gitnasr
PR: dentizone/api#16
File: Dentizone.Infrastructure/Persistence/Configurations/OrderConfiguration.cs:1-1
Timestamp: 2025-05-28T11:22:49.051Z
Learning: In the Dentizone.Infrastructure project, the IsDeleted property from IBaseEntity is handled through Entity Framework interceptors rather than explicit configuration in individual entity configuration files. This means entity configurations don't need to explicitly configure the IsDeleted property.
Dentizone.Application/Services/AssetService.cs (1)
Learnt from: gitnasr
PR: dentizone/api#16
File: Dentizone.Infrastructure/Persistence/Configurations/OrderConfiguration.cs:1-1
Timestamp: 2025-05-28T11:22:49.051Z
Learning: In the Dentizone.Infrastructure project, the IsDeleted property from IBaseEntity is handled through Entity Framework interceptors rather than explicit configuration in individual entity configuration files. This means entity configurations don't need to explicitly configure the IsDeleted property.
Dentizone.Application/Services/ReviewService.cs (1)
Learnt from: gitnasr
PR: dentizone/api#16
File: Dentizone.Infrastructure/Persistence/Configurations/OrderConfiguration.cs:1-1
Timestamp: 2025-05-28T11:22:49.051Z
Learning: In the Dentizone.Infrastructure project, the IsDeleted property from IBaseEntity is handled through Entity Framework interceptors rather than explicit configuration in individual entity configuration files. This means entity configurations don't need to explicitly configure the IsDeleted property.
Dentizone.Application/Services/UploadService.cs (1)
Learnt from: gitnasr
PR: dentizone/api#16
File: Dentizone.Infrastructure/Persistence/Configurations/OrderConfiguration.cs:1-1
Timestamp: 2025-05-28T11:22:49.051Z
Learning: In the Dentizone.Infrastructure project, the IsDeleted property from IBaseEntity is handled through Entity Framework interceptors rather than explicit configuration in individual entity configuration files. This means entity configurations don't need to explicitly configure the IsDeleted property.
Dentizone.Application/Services/BaseService.cs (1)
Learnt from: gitnasr
PR: dentizone/api#16
File: Dentizone.Infrastructure/Persistence/Configurations/OrderConfiguration.cs:1-1
Timestamp: 2025-05-28T11:22:49.051Z
Learning: In the Dentizone.Infrastructure project, the IsDeleted property from IBaseEntity is handled through Entity Framework interceptors rather than explicit configuration in individual entity configuration files. This means entity configurations don't need to explicitly configure the IsDeleted property.
🧬 Code Graph Analysis (2)
Dentizone.Application/Services/BaseService.cs (2)
Dentizone.Application/Services/AssetService.cs (5)
  • Task (14-22)
  • Task (24-29)
  • Task (31-41)
  • Task (43-47)
  • Task (49-58)
Dentizone.Application/Services/ReviewService.cs (6)
  • Task (19-29)
  • Task (31-36)
  • Task (38-44)
  • Task (46-63)
  • Task (65-78)
  • Task (80-89)
Dentizone.Application/Services/PostService.cs (4)
Dentizone.Application/Services/BaseService.cs (2)
  • BaseService (8-67)
  • BaseService (12-15)
Dentizone.Application/DTOs/PostFilterDTO/CategoryFilterDTO.cs (1)
  • CategoryFilterDto (3-8)
Dentizone.Application/DTOs/PostFilterDTO/SidebarFilterDTO.cs (1)
  • SidebarFilterDto (3-9)
Dentizone.Infrastructure/Repositories/PostRepsitory.cs (6)
  • Task (11-16)
  • Task (18-31)
  • Task (33-46)
  • Task (48-55)
  • Task (57-82)
  • Task (110-120)
🔇 Additional comments (15)
Dentizone.Infrastructure/Identity/IdentityConfiguration.cs (3)

29-29: Good formatting improvement.

The single-line format for DefaultLockoutTimeSpan improves readability.


45-49: Improved code formatting.

The reformatted IssuerSigningKey initialization is more readable while maintaining the same functionality.


63-66: Consistent formatting improvement.

The consistent formatting matches the style used in the other overload method.

Dentizone.Application/Services/BaseService.cs (1)

61-66: Good abstraction for ownership retrieval.

The abstract method design allows each service to implement resource-specific ownership logic while maintaining consistent authorization patterns.

Dentizone.Application/Services/Payment/PaymentService.cs (2)

90-90: LGTM! Consistent naming convention applied.

The variable naming change from DatabaseTransaction to databaseTransaction follows C# naming conventions for local variables.

Also applies to: 132-132, 139-139, 147-147, 182-182, 187-187


98-98: Good formatting improvements.

The indentation and line breaking changes improve code readability.

Also applies to: 112-113, 152-152, 172-173

Dentizone.Application/Services/OrderService.cs (2)

16-16: Excellent integration with BaseService pattern.

The service properly inherits from BaseService and correctly passes the IHttpContextAccessor to the base constructor, enabling centralized authorization.

Also applies to: 31-33


280-289: Well-implemented ownership retrieval.

The GetOwnerIdAsync correctly identifies the buyer as the owner of an order, which is appropriate for order cancellation authorization.

Dentizone.Application/Services/PostService.cs (3)

13-13: Proper BaseService integration implemented.

The service correctly inherits from BaseService with IHttpContextAccessor as the first constructor parameter, maintaining consistency across services.

Also applies to: 20-20, 29-29


185-185: Good authorization implementation for post updates.

The UpdatePost method correctly enforces that only admins or the post owner can update the post.


324-334: Correct ownership implementation.

The GetOwnerIdAsync properly identifies the seller as the owner of a post.

Dentizone.Application/Services/AssetService.cs (1)

7-7: Excellent authorization implementation throughout AssetService.

The service properly:

  • Inherits from BaseService with correct constructor setup
  • Implements authorization checks for both UpdateAssetAsync and DeleteAssetAsync
  • Correctly identifies asset owners through GetOwnerIdAsync

All modification operations are now properly secured.

Also applies to: 11-12, 36-36, 45-45, 49-58

Dentizone.Application/Services/ReviewService.cs (2)

8-8: Proper BaseService integration.

The service correctly inherits from BaseService with appropriate constructor changes.

Also applies to: 13-17


33-33: Correct authorization implementation.

Both DeleteReviewAsync authorization and GetOwnerIdAsync implementation are properly done.

Also applies to: 80-89

Dentizone.Application/Services/UploadService.cs (1)

11-15: Good refactoring to inherit from BaseService.

The constructor properly accepts IHttpContextAccessor and passes it to the base class, enabling centralized authorization logic.

using Dentizone.Domain.Exceptions;
using Dentizone.Domain.Interfaces.Repositories;
using System.Linq.Expressions;
using Dentizone.Infrastructure;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Application layer now imports an Infrastructure-level namespace – breaks clean architecture boundaries

Adding using Dentizone.Infrastructure; couples the Application project to the Infrastructure project. In a layered / clean-architecture setup, dependencies must point outwards (Infrastructure → Application), never the other way around.
This single using will force the compiler to reference the Infrastructure assembly and may create circular-dependency headaches the moment Infrastructure needs types from Application (which it usually does).

Proposed direction (no direct patch here because it spans multiple projects):

  1. Define a lightweight IAppDbContext (or IDbContext) interface in the Application layer exposing only what the services need (Database, Set<TEntity>(), etc.).
  2. Have AppDbContext in Infrastructure implement that interface.
  3. Inject IAppDbContext into services instead of the concrete AppDbContext.
  4. Remove this using Dentizone.Infrastructure; directive; the Application layer should only know the abstraction.
🤖 Prompt for AI Agents
In Dentizone.Application/Services/UserService.cs at line 10, remove the using
directive for Dentizone.Infrastructure to avoid breaking clean architecture
boundaries. Instead, define an interface like IAppDbContext in the Application
layer that exposes only the necessary database operations. Modify the
Infrastructure project's AppDbContext to implement this interface. Refactor
UserService to depend on the IAppDbContext abstraction via dependency injection
rather than the concrete AppDbContext, thus eliminating the direct
Infrastructure reference.

Comment on lines 14 to +18
public class UserService(
IUserRepository userRepository,
IMapper mapper,
IWalletService walletService,
Infrastructure.AppDbContext dbContext)
AppDbContext dbContext)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Concrete AppDbContext injection tightens coupling and hinders testing

UserService now receives the concrete AppDbContext. Besides the architectural boundary breach mentioned above, this also:

• Makes the service harder to unit-test (you must spin up a real EF Core context or a complex in-memory substitute).
• Locks the service to EF Core; swapping persistence tech becomes painful.

Refactor constructor signature:

-    IWalletService walletService,
-    AppDbContext dbContext)
+    IWalletService walletService,
+    IAppDbContext dbContext)

…after introducing the abstraction described in the previous comment.
This preserves transaction handling semantics while restoring proper separation of concerns and testability.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public class UserService(
IUserRepository userRepository,
IMapper mapper,
IWalletService walletService,
Infrastructure.AppDbContext dbContext)
AppDbContext dbContext)
public class UserService(
IUserRepository userRepository,
IMapper mapper,
IWalletService walletService,
IAppDbContext dbContext)
🤖 Prompt for AI Agents
In Dentizone.Application/Services/UserService.cs around lines 14 to 18, the
constructor currently injects the concrete AppDbContext, which increases
coupling and reduces testability. Refactor the constructor to inject an
abstraction (such as an interface) that encapsulates the required database
operations or transaction handling instead of the concrete AppDbContext. This
change will decouple UserService from EF Core specifics, improve testability by
allowing mocks or fakes, and maintain proper separation of concerns.

public string FullName { get; set; }
public string Username { get; set; }
public int AcademicYear { get; set; }
public required string Email { get; set; }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Good addition of required Email property.

The required Email property is appropriate for user entities and ensures proper initialization. This aligns well with the corresponding EF configuration.

[approve_code_changes]

Ensure existing data migration is handled properly for the new required field:


🏁 Script executed:

#!/bin/bash
# Description: Check for entity framework migrations related to Email property
# Expected: Migration files should handle the new required Email property

fd -e cs . | xargs grep -l "Email" | grep -i migration

Length of output: 337


Missing EF Core migration for new required Email property

It appears there isn’t a migration in Dentizone.Infrastructure/Migrations that adds the non-nullable Email column to your AppUser table. Please:

  • Create a new EF Core migration that adds a non-nullable Email column to the users table.
  • Implement a data-migration strategy (default values or backfill) for existing records.
  • Ensure AppDbContextModelSnapshot.cs is updated accordingly.
🤖 Prompt for AI Agents
In Dentizone.Domain/Entity/AppUser.cs at line 11, you added a new required Email
property but did not create an EF Core migration for it. Generate a new
migration in Dentizone.Infrastructure/Migrations that adds a non-nullable Email
column to the users table. Implement a data migration strategy within the
migration to handle existing records, such as setting a default or backfilling
values. Finally, ensure the AppDbContextModelSnapshot.cs file is updated to
reflect this schema change.

Comment on lines +45 to 54
protected override async Task<string> GetOwnerIdAsync(string resourceId)
{
var asset = await assetService.GetAssetByIdAsync(resourceId);
if (asset == null)
{
throw new UnauthorizedAccessException("You are not authorized to delete this asset");
throw new NotFoundException($"Asset with id {resourceId} not found");
}

await assetService.DeleteAssetAsync(asset.Id);
return asset;
return asset.UserId;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Verify if the null check is necessary.

The null check on line 48 might be redundant if GetAssetByIdAsync already throws NotFoundException for non-existent assets. Please verify the behavior of this method.

If GetAssetByIdAsync already throws NotFoundException, simplify the code:

protected override async Task<string> GetOwnerIdAsync(string resourceId)
{
    var asset = await assetService.GetAssetByIdAsync(resourceId);
-   if (asset == null)
-   {
-       throw new NotFoundException($"Asset with id {resourceId} not found");
-   }
-
    return asset.UserId;
}

🏁 Script executed:

#!/bin/bash
# Description: Check if GetAssetByIdAsync in IAssetService/AssetService throws NotFoundException

# Search for GetAssetByIdAsync implementation
ast-grep --pattern 'GetAssetByIdAsync($$$) {
  $$$
}'

# Also search with async keyword
ast-grep --pattern 'async $_ GetAssetByIdAsync($$$) {
  $$$
}'

# Check interface definition
rg -A 5 "GetAssetByIdAsync" --glob "*.cs"

Length of output: 2674


Remove redundant null check in GetOwnerIdAsync

The GetAssetByIdAsync implementation already throws a NotFoundException when the asset isn’t found, so the null check in UploadService.GetOwnerIdAsync will never be hit and can be removed.

• File: Dentizone.Application/Services/UploadService.cs
Method: protected override async Task<string> GetOwnerIdAsync(string resourceId)

 protected override async Task<string> GetOwnerIdAsync(string resourceId)
 {
     var asset = await assetService.GetAssetByIdAsync(resourceId);
-    if (asset == null)
-    {
-        throw new NotFoundException($"Asset with id {resourceId} not found");
-    }
     return asset.UserId;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
protected override async Task<string> GetOwnerIdAsync(string resourceId)
{
var asset = await assetService.GetAssetByIdAsync(resourceId);
if (asset == null)
{
throw new UnauthorizedAccessException("You are not authorized to delete this asset");
throw new NotFoundException($"Asset with id {resourceId} not found");
}
await assetService.DeleteAssetAsync(asset.Id);
return asset;
return asset.UserId;
}
protected override async Task<string> GetOwnerIdAsync(string resourceId)
{
var asset = await assetService.GetAssetByIdAsync(resourceId);
return asset.UserId;
}
🤖 Prompt for AI Agents
In Dentizone.Application/Services/UploadService.cs around lines 45 to 54, remove
the redundant null check for the asset after calling GetAssetByIdAsync, since
that method already throws a NotFoundException if the asset is not found.
Simplify the method by directly returning asset.UserId without checking if asset
is null.

Copy link
Copy Markdown

@korbit-ai korbit-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.
Category Issue Status
Security Unvalidated Cache Key Generation ▹ view
Readability Direct Enum Comparison Instead of String ▹ view ✅ Fix detected
Files scanned
File Path Reviewed
Dentizone.Application/Interfaces/IUploadService.cs
Dentizone.Domain/Entity/AppUser.cs
Dentizone.Application/Services/UploadService.cs
Dentizone.Application/Services/AssetService.cs
Dentizone.Infrastructure/Identity/IdentityConfiguration.cs
Dentizone.Infrastructure/Persistence/Configurations/UserConfiguration.cs
Dentizone.Application/Services/BaseService.cs
Dentizone.Application/Services/UserService.cs
Dentizone.Application/Services/ReviewService.cs
Dentizone.Application/Services/Payment/PaymentService.cs
Dentizone.Application/Services/OrderService.cs
Dentizone.Application/Services/PostService.cs

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

gitnasr added 2 commits June 29, 2025 01:50
- Updated `DeleteAssetById` in `IUploadService.cs` and `UploadService.cs` to remove the `userId` parameter, simplifying the method signature.
- Modified review retrieval logic in `ReviewService.cs` to specifically search for a review by its `Id`, ensuring accurate results and preventing potential issues with multiple matches.
Updated IOrderService and OrderService to eliminate the userId parameter from the CancelOrderAsync method. Adjusted OrderController to reflect this change, ensuring the method is called without the userId.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b89333d and d5f0edf.

📒 Files selected for processing (7)
  • Dentizone.Application/Interfaces/IUploadService.cs (1 hunks)
  • Dentizone.Application/Interfaces/Order/IOrderService.cs (1 hunks)
  • Dentizone.Application/Services/BaseService.cs (1 hunks)
  • Dentizone.Application/Services/OrderService.cs (4 hunks)
  • Dentizone.Application/Services/ReviewService.cs (4 hunks)
  • Dentizone.Application/Services/UploadService.cs (2 hunks)
  • Dentizone.Presentaion/Controllers/OrderController.cs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • Dentizone.Application/Services/BaseService.cs
  • Dentizone.Application/Services/ReviewService.cs
  • Dentizone.Application/Services/OrderService.cs
  • Dentizone.Application/Services/UploadService.cs
🧰 Additional context used
🧠 Learnings (1)
Dentizone.Application/Interfaces/IUploadService.cs (1)
Learnt from: gitnasr
PR: dentizone/api#16
File: Dentizone.Infrastructure/Persistence/Configurations/OrderConfiguration.cs:1-1
Timestamp: 2025-05-28T11:22:49.051Z
Learning: In the Dentizone.Infrastructure project, the IsDeleted property from IBaseEntity is handled through Entity Framework interceptors rather than explicit configuration in individual entity configuration files. This means entity configurations don't need to explicitly configure the IsDeleted property.
🧬 Code Graph Analysis (1)
Dentizone.Application/Interfaces/Order/IOrderService.cs (1)
Dentizone.Application/DTOs/Order/OrderViewDTO.cs (1)
  • OrderViewDto (3-14)
🔇 Additional comments (2)
Dentizone.Presentaion/Controllers/OrderController.cs (1)

44-44: Authorization Verified in Service Layer

  • In Dentizone.Application/Services/OrderService.cs, CancelOrderAsync (line 45) calls
    await AuthorizeAdminOrOwnerAsync(orderId);
    ensuring only the order’s owner or an admin can perform cancellations.
  • No further changes are needed; the service layer properly handles authorization.
Dentizone.Application/Interfaces/Order/IOrderService.cs (1)

11-11: All implementations updated to the new signature – change approved

  • OrderService.cs now defines CancelOrderAsync(string orderId) matching the interface.
  • No remaining overloads or calls using the old userId parameter were found.

public Task<AssetDto> FindAssetById(string id);

public Task<AssetDto> DeleteAssetById(string id, string userId);
Task DeleteAssetById(string id);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Verify that this breaking change has been properly handled across the codebase.

The method signature change removes the userId parameter and changes the return type from Task<AssetDto> to Task. This is a breaking change that could impact:

  1. All implementations of IUploadService
  2. All callers expecting the deleted asset to be returned
  3. Any logging or audit functionality that relied on the returned asset details

Please run the following script to verify all implementations and usages have been updated:


🏁 Script executed:

#!/bin/bash
# Description: Verify all implementations and usages of DeleteAssetById have been updated

echo "=== Checking for implementations of IUploadService ==="
ast-grep --pattern 'class $_ : $$$IUploadService$$$'

echo -e "\n=== Checking for DeleteAssetById method implementations ==="
ast-grep --pattern 'DeleteAssetById($_) {
  $$$
}'

echo -e "\n=== Checking for calls to DeleteAssetById ==="
rg -A 3 'DeleteAssetById\(' --type cs

echo -e "\n=== Checking if any code still expects AssetDto return type ==="
rg -B 2 -A 2 'DeleteAssetById.*AssetDto' --type cs

Length of output: 1934


Controller call must be updated to match new signature

The interface and service have been updated to Task DeleteAssetById(string id), but the controller still calls the old overload with two parameters. Update the caller accordingly.

• File: Dentizone.Presentaion/Controllers/UploadController.cs
– Change the service call to use the single-parameter signature
– Remove the userId argument

Recommended diff:

-            await uploadService.DeleteAssetById(id, userId);
+            await uploadService.DeleteAssetById(id);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Task DeleteAssetById(string id);
await uploadService.DeleteAssetById(id);
🤖 Prompt for AI Agents
In Dentizone.Presentaion/Controllers/UploadController.cs, locate the calls to
DeleteAssetById that pass two parameters and update them to call the service
method with only the single string id parameter, removing the userId argument to
match the updated interface signature in
Dentizone.Application/Interfaces/IUploadService.cs at line 11.

Comment on lines 43 to +44
var userId = User.FindFirstValue(ClaimTypes.NameIdentifier);
var result = await orderService.CancelOrderAsync(orderId, userId);
var result = await orderService.CancelOrderAsync(orderId);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Remove unused userId extraction.

The userId is extracted from claims on line 43 but no longer used in the service call on line 44. This creates dead code that should be removed as part of the authorization refactoring.

[HttpPut("{orderId}/cancel")]
public async Task<IActionResult> CancelOrder(string orderId)
{
-    var userId = User.FindFirstValue(ClaimTypes.NameIdentifier);
     var result = await orderService.CancelOrderAsync(orderId);
     return Ok(result);
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var userId = User.FindFirstValue(ClaimTypes.NameIdentifier);
var result = await orderService.CancelOrderAsync(orderId, userId);
var result = await orderService.CancelOrderAsync(orderId);
[HttpPut("{orderId}/cancel")]
public async Task<IActionResult> CancelOrder(string orderId)
{
var result = await orderService.CancelOrderAsync(orderId);
return Ok(result);
}
🤖 Prompt for AI Agents
In Dentizone.Presentaion/Controllers/OrderController.cs at lines 43-44, the
variable userId is assigned from user claims but never used afterward. Remove
the line extracting userId to eliminate dead code and clean up the method since
the authorization refactoring no longer requires it.

- Updated `CreateAppUser.cs` to add `Email` property.
- Reordered using directives and reformatted identity options in `AppIdentity.cs`.
- Introduced new authorization policies: "IsVerified", "IsPartilyVerified", and "IsAdmin".
- Refactored `Posts.cs` for consistency and improved user creation logic.
- Required "IsAdmin" policy in `AdminController.cs` and `AnalyticsController.cs`.
- Modified `AuthenticationController.cs` to include `Email` in access token generation.
- Updated `CartController.cs` to require "IsPartilyVerified" policy.
- Enhanced `CatalogController.cs` with "IsAdmin" authorization for category management.
- Updated `FavoritesController.cs` and `OrderController.cs` with new authorization requirements.
- Removed `PaymentController.cs`.
- Updated `PostsController.cs`, `ReviewController.cs`, `ShippingController.cs`, and `UniversitiesController.cs` to enforce "IsAdmin" policy.
- Updated `UploadController.cs` to require "IsVerified" policy.
- Modified `VerificationController.cs` to include user national ID update logic.
- Enhanced `WalletController.cs` with new withdrawal request and history endpoints.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🔭 Outside diff range comments (2)
Dentizone.Presentaion/Controllers/CartController.cs (1)

17-17: Add null checks for claims extraction.

Multiple instances of User.Claims.First() could throw exceptions if the expected claim is not present. Consider using safer claim extraction methods.

-            var userId = User.Claims.First(c => c.Type == ClaimTypes.NameIdentifier).Value;
+            var userId = User.FindFirst(ClaimTypes.NameIdentifier)?.Value;
+            if (string.IsNullOrEmpty(userId))
+                return Unauthorized("User ID not found in claims");

Also applies to: 27-27, 35-35, 43-43

Dentizone.Presentaion/Controllers/WalletController.cs (1)

18-18: Add null checks for claims extraction.

Similar to other controllers, the User.Claims.First() calls could throw exceptions if the expected claim is not present. Consider using safer claim extraction methods.

-            var userId = User.Claims.First(c => c.Type == ClaimTypes.NameIdentifier).Value;
+            var userId = User.FindFirst(ClaimTypes.NameIdentifier)?.Value;
+            if (string.IsNullOrEmpty(userId))
+                return Unauthorized("User ID not found in claims");

Also applies to: 28-28, 37-37

♻️ Duplicate comments (1)
Dentizone.Presentaion/Controllers/OrderController.cs (1)

44-45: Remove unused userId extraction (duplicate issue).

The userId is extracted from claims on line 44 but no longer used in the service call on line 45. This creates dead code that should be removed as part of the authorization refactoring.

🧹 Nitpick comments (2)
Dentizone.Application/DTOs/User/CreateAppUser.cs (1)

7-8: Fix formatting inconsistency.

The line break after the string type declaration is inconsistent with the rest of the file's formatting style.

-        public string
-            Id { get; set; }
+        public string Id { get; set; }
Dentizone.Presentaion/Controllers/PostsController.cs (1)

60-61: Consider the implications of removing exception handling.

The removal of try-catch blocks means unhandled exceptions from the service layer will propagate directly to clients, potentially exposing internal error details or causing poor user experience.

If the service layer now handles authorization exceptions appropriately, this change is acceptable. However, consider whether you still need controller-level exception handling for other types of errors (validation, not found, etc.).

Also applies to: 68-69

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d5f0edf and af319e4.

📒 Files selected for processing (18)
  • Dentizone.Application/DTOs/User/CreateAppUser.cs (1 hunks)
  • Dentizone.Infrastructure/DependencyInjection/AppIdentity.cs (3 hunks)
  • Dentizone.Infrastructure/Persistence/Seeder/Posts.cs (6 hunks)
  • Dentizone.Presentaion/Controllers/AdminController.cs (1 hunks)
  • Dentizone.Presentaion/Controllers/AnalyticsController.cs (1 hunks)
  • Dentizone.Presentaion/Controllers/AuthenticationController.cs (3 hunks)
  • Dentizone.Presentaion/Controllers/CartController.cs (1 hunks)
  • Dentizone.Presentaion/Controllers/CatalogController.cs (4 hunks)
  • Dentizone.Presentaion/Controllers/FavoritesController.cs (2 hunks)
  • Dentizone.Presentaion/Controllers/OrderController.cs (2 hunks)
  • Dentizone.Presentaion/Controllers/PaymentController.cs (0 hunks)
  • Dentizone.Presentaion/Controllers/PostsController.cs (2 hunks)
  • Dentizone.Presentaion/Controllers/ReviewController.cs (2 hunks)
  • Dentizone.Presentaion/Controllers/ShippingController.cs (1 hunks)
  • Dentizone.Presentaion/Controllers/UniversitiesController.cs (1 hunks)
  • Dentizone.Presentaion/Controllers/UploadController.cs (2 hunks)
  • Dentizone.Presentaion/Controllers/VerificationController.cs (1 hunks)
  • Dentizone.Presentaion/Controllers/WalletController.cs (3 hunks)
💤 Files with no reviewable changes (1)
  • Dentizone.Presentaion/Controllers/PaymentController.cs
✅ Files skipped from review due to trivial changes (3)
  • Dentizone.Presentaion/Controllers/VerificationController.cs
  • Dentizone.Infrastructure/Persistence/Seeder/Posts.cs
  • Dentizone.Presentaion/Controllers/AuthenticationController.cs
🧰 Additional context used
🧠 Learnings (5)
Dentizone.Presentaion/Controllers/AdminController.cs (1)
Learnt from: gitnasr
PR: dentizone/api#16
File: Dentizone.Infrastructure/Persistence/Configurations/OrderConfiguration.cs:1-1
Timestamp: 2025-05-28T11:22:49.051Z
Learning: In the Dentizone.Infrastructure project, the IsDeleted property from IBaseEntity is handled through Entity Framework interceptors rather than explicit configuration in individual entity configuration files. This means entity configurations don't need to explicitly configure the IsDeleted property.
Dentizone.Presentaion/Controllers/UploadController.cs (1)
Learnt from: gitnasr
PR: dentizone/api#16
File: Dentizone.Infrastructure/Persistence/Configurations/OrderConfiguration.cs:1-1
Timestamp: 2025-05-28T11:22:49.051Z
Learning: In the Dentizone.Infrastructure project, the IsDeleted property from IBaseEntity is handled through Entity Framework interceptors rather than explicit configuration in individual entity configuration files. This means entity configurations don't need to explicitly configure the IsDeleted property.
Dentizone.Infrastructure/DependencyInjection/AppIdentity.cs (1)
Learnt from: gitnasr
PR: dentizone/api#16
File: Dentizone.Infrastructure/Persistence/Configurations/OrderConfiguration.cs:1-1
Timestamp: 2025-05-28T11:22:49.051Z
Learning: In the Dentizone.Infrastructure project, the IsDeleted property from IBaseEntity is handled through Entity Framework interceptors rather than explicit configuration in individual entity configuration files. This means entity configurations don't need to explicitly configure the IsDeleted property.
Dentizone.Presentaion/Controllers/FavoritesController.cs (1)
Learnt from: gitnasr
PR: dentizone/api#16
File: Dentizone.Infrastructure/Persistence/Configurations/OrderConfiguration.cs:1-1
Timestamp: 2025-05-28T11:22:49.051Z
Learning: In the Dentizone.Infrastructure project, the IsDeleted property from IBaseEntity is handled through Entity Framework interceptors rather than explicit configuration in individual entity configuration files. This means entity configurations don't need to explicitly configure the IsDeleted property.
Dentizone.Presentaion/Controllers/ReviewController.cs (1)
Learnt from: gitnasr
PR: dentizone/api#16
File: Dentizone.Infrastructure/Persistence/Configurations/OrderConfiguration.cs:1-1
Timestamp: 2025-05-28T11:22:49.051Z
Learning: In the Dentizone.Infrastructure project, the IsDeleted property from IBaseEntity is handled through Entity Framework interceptors rather than explicit configuration in individual entity configuration files. This means entity configurations don't need to explicitly configure the IsDeleted property.
🔇 Additional comments (20)
Dentizone.Presentaion/Controllers/AnalyticsController.cs (1)

2-2: LGTM! Proper authorization implementation for analytics endpoints.

The addition of admin-only authorization is appropriate for analytics endpoints that contain sensitive business data. The import and authorization attribute are correctly implemented.

Also applies to: 9-9

Dentizone.Presentaion/Controllers/ShippingController.cs (1)

4-4: LGTM! Appropriate authorization for shipping operations.

The admin-only authorization policy is correctly applied to shipping operations, which should indeed be restricted to administrative users given the sensitive nature of shipment status updates.

Also applies to: 12-12

Dentizone.Presentaion/Controllers/CartController.cs (1)

11-11: Correct policy name spelling and add null‐safety for user claims

  • The policy "IsPartilyVerified" is registered and consumed in three places:

    • Dentizone.Infrastructure/DependencyInjection/AppIdentity.cs
      options.AddPolicy("IsPartilyVerified", …)
    • Dentizone.Presentaion/Controllers/FavoritesController.cs
      [Authorize(Policy = "IsPartilyVerified")]
    • Dentizone.Presentaion/Controllers/CartController.cs
      [Authorize(Policy = "IsPartilyVerified")]
      Consider renaming it (and the corresponding role constant UserRoles.PARTILY_VERIFIED) to IsPartiallyVerified (and PARTIALLY_VERIFIED) for correct spelling.
  • In both CartController and FavoritesController, any use of
    User.Claims.First(…)
    can throw if the claim isn’t present. Wrap these in a null check or use FirstOrDefault/User.FindFirst(…) and handle the missing‐claim case (e.g., return Unauthorized).

Dentizone.Presentaion/Controllers/WalletController.cs (2)

6-6: LGTM! Appropriate authorization for wallet operations.

The "IsVerified" authorization requirement is well-suited for wallet operations, providing the right balance of security for financial transactions while not being overly restrictive.

Also applies to: 12-12


24-24: Good formatting improvements.

The addition of blank lines improves code readability and follows good formatting practices.

Also applies to: 33-33

Dentizone.Presentaion/Controllers/OrderController.cs (2)

25-25: LGTM: Appropriate authorization policy for order creation.

The "IsVerified" policy ensures only fully verified users can create orders, which is a good security practice for financial transactions.


50-50: LGTM: Proper admin-only authorization for administrative operations.

Both ConfirmOrder and GetAllOrders are correctly restricted to admin users, ensuring only authorized personnel can perform these sensitive operations.

Also applies to: 58-58

Dentizone.Presentaion/Controllers/AdminController.cs (1)

8-8: LGTM: Appropriate class-level admin authorization.

The [Authorize("IsAdmin")] attribute correctly restricts all controller actions to admin users only, which is appropriate since all methods handle sensitive withdrawal operations.

Dentizone.Presentaion/Controllers/FavoritesController.cs (2)

11-11: LGTM: Appropriate authorization policy for favorites functionality.

The "IsPartilyVerified" policy allows both partially and fully verified users to manage favorites, which is appropriate for this non-critical functionality.


26-27: LGTM: Simplified response aligns with service layer refactoring.

The simplified response without returning the created entity is consistent with the centralized authorization approach and reduces unnecessary data transfer.

Dentizone.Infrastructure/DependencyInjection/AppIdentity.cs (1)

39-55: LGTM: Well-designed authorization policies with clear hierarchy.

The authorization policies are well-structured with:

  • Clear documentation explaining each policy's purpose
  • Proper role hierarchy (verified users can access partially verified actions)
  • Appropriate separation of concerns between different verification levels

This provides a solid foundation for the application's role-based access control system.

Dentizone.Presentaion/Controllers/UploadController.cs (2)

11-11: LGTM: Appropriate authorization policy for file uploads.

The "IsVerified" policy ensures only fully verified users can upload files, which is a good security practice for preventing abuse of upload functionality.


50-50: LGTM: Simplified method aligns with centralized authorization.

The removal of user ID extraction and direct service call is consistent with the BaseService authorization approach, where ownership verification is now handled centrally within the service layer.

Dentizone.Presentaion/Controllers/UniversitiesController.cs (2)

29-34: Verify the authorization inconsistency for GET endpoints.

The GetUniversityById endpoint requires admin access while the other GET endpoints (GetAll, GetAllUniversities) allow anonymous access. This inconsistency might be intentional, but please confirm whether individual university details should be publicly accessible or admin-only.


3-3: Authorization pattern looks well-designed.

The implementation correctly applies admin-only access by default while allowing public access to university listing endpoints. This follows security best practices for administrative resources where discovery is public but management requires elevated privileges.

Also applies to: 10-10, 14-14, 22-22

Dentizone.Presentaion/Controllers/ReviewController.cs (1)

11-11: Well-implemented authorization hierarchy.

The authorization pattern correctly implements a tiered access control where verified users can interact with reviews, but only admins can modify or delete them. This follows the principle of least privilege while maintaining necessary administrative control.

Also applies to: 24-24, 32-32

Dentizone.Presentaion/Controllers/CatalogController.cs (2)

34-34: Excellent implementation of role-based access control.

The authorization pattern consistently applies admin-only access to all mutating operations while keeping read operations publicly accessible. This is ideal for a catalog system where browsing should be open but content management should be restricted.

Also applies to: 42-42, 52-52, 80-80, 89-89, 97-97


34-34: Excellent authorization implementation for catalog management.

The admin-only policy for all mutating operations (create, update, delete) on categories and subcategories is appropriate, while maintaining public read access for browsing. This prevents unauthorized catalog modifications while enabling essential functionality for all users.

Also applies to: 42-42, 52-52, 80-80, 89-89, 97-97

Dentizone.Presentaion/Controllers/PostsController.cs (2)

13-13: Well-structured authorization implementation.

The authorization pattern effectively balances public access for browsing with appropriate role requirements for sensitive operations. The tiered approach (anonymous for viewing, verified for creating, admin for management) is well-designed.

Also applies to: 17-17, 25-25, 33-33, 41-41, 57-57, 64-64, 72-72, 80-80


33-33: Verify if GetPostsBySellerId should be publicly accessible.

This endpoint being admin-only prevents users from browsing posts by specific sellers, which might be a desired public feature in a marketplace application.

Please confirm if this endpoint should allow public access for marketplace browsing functionality.

Id { get; set; }

public string FullName { get; set; }
public string Email { get; set; }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add validation attributes to the Email property.

The Email property lacks validation attributes that would ensure data integrity and provide clear validation feedback.

+using System.ComponentModel.DataAnnotations;
+
 namespace Dentizone.Application.DTOs.User
 {
     public class CreateAppUser
     {
         public string Id { get; set; }
         public string FullName { get; set; }
+        [Required]
+        [EmailAddress]
         public string Email { get; set; }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public string Email { get; set; }
using System.ComponentModel.DataAnnotations;
namespace Dentizone.Application.DTOs.User
{
public class CreateAppUser
{
public string Id { get; set; }
public string FullName { get; set; }
[Required]
[EmailAddress]
public string Email { get; set; }
}
}
🤖 Prompt for AI Agents
In Dentizone.Application/DTOs/User/CreateAppUser.cs at line 11, the Email
property is missing validation attributes. Add appropriate data annotation
attributes such as [Required] and [EmailAddress] to the Email property to
enforce that it is not null or empty and that it contains a valid email format,
ensuring data integrity and clear validation feedback.

gitnasr added 5 commits June 29, 2025 13:38
Updated the `IsAdmin` method in the `BaseService` class to use `Enum.TryParse` for safer role checking, enhancing type safety.

Reorganized the using directives in the `UserService` class for improved readability without affecting functionality.
- Removed `IReviewRepository` and `IWithdrawalRequestRepository` from DI in `AddRepositories.cs`.
- Updated `Environment` variable initialization in `GetSecret` constructor of `SecretService.cs` to use environment variables.
- Simplified lambda expression in `GetSecret` method of `SecretService.cs`.
- Removed unused import of `Dentizone.Domain.Exceptions` in `PostsController.cs`.
- Eliminated null check for `userId` in `CreatePost` method of `PostsController.cs`.
- Changed authorization policy for `DeletePost` and `UpdatePost` methods to "IsVerified".
- Made root endpoint redirection permanent in `Program.cs`.
- Commented out role and database seeding logic in `Program.cs`.
- Implemented AutoMapper profiles for Answer, Question, and Withdrawal entities.
- Introduced FluentValidation classes for various DTOs to enforce data integrity.
- Updated IQAService and IWithdrawalService interfaces with new methods.
- Added services and repositories for managing questions and answers.
- Enhanced QAController to handle HTTP requests for questions and answers.
- Updated OrderStatus enum to include a new status and corrected spelling.
- Improved code style and removed unnecessary using directives for better readability.
- Ensured consistency with application architecture and design patterns.
Updated the mapping configurations in the `AnswerProfile` class to use simplified type names instead of fully qualified names. This change improves code readability and reduces verbosity while retaining the existing mapping functionality for `CreateAnswerDto`, `UpdateAnswerDto`, and `AnswerViewDto`.
@sonarqubecloud
Copy link
Copy Markdown

@gitnasr gitnasr merged commit 5a7319c into dev Jun 29, 2025
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants