0

Right now I check the objects ownership of users like in code below (I commented important parts of code). And I do the same for every model and every controller method in my application, which completely violates the DRY principle. Maybe you have some ideas how I can avoid duplication?

Besides, I am not sure if it is safe and a user will not be able to access other people's data with some tricks. Is what I am doing is safe?

This is a very important topic regarding the security of personal data, and I'm sure there should be some kind of documentation. Unfortunately, I did not find any documentation, if I did not search well, I would be glad to see any links.

TasksController.cs

public async Task<IActionResult> Index()
        {
            // here I form a selection from objects belonging to the current user
            var Tasks = _context.Tasks.Where(t => t.UserId.Equals(User.Identity.GetUserId()));
            return View(await applicationDbContext.ToListAsync());
        }


public async Task<IActionResult> Edit(int? id)
        {
            if (id == null)
            {
                return NotFound();
            }

            var task = await _context.Tasks.FindAsync(id);

            // here I check if the requested object belongs to the current user
            if (task == null | task.UserId != User.Identity.GetUserId())
            {
                return NotFound();
            }
            return View(task);
        }


    [HttpPost]
    [ValidateAntiForgeryToken]
    public async Task<IActionResult> Create([Bind("Id,Name,TaskTypeId,Status,UserId")] Task task)
    {
        if (ModelState.IsValid)
        {
            // here i am assigning object's UserId to id of current user
            task.UserId = User.Identity.GetUserId();
            _context.Add(task);
            await _context.SaveChangesAsync();
            return RedirectToAction(nameof(Index));
        }
        return View(task);
    }
lleviy
  • 406
  • 5
  • 16

1 Answers1

1

I think the best approach would be to inject a work context object to the controller. Then get the user and other common information from that object which would reduce the repetitive codes. For example:

Controller:

private readonly IWorkContext _workContext;

public MyController(IWorkContext  workContext)
{
 this._workContext = workContext;
}

Action:

Instead of User.Identity.GetUserId() you do _workContext.UserId. This way if your identity logic changes then you do not have to change that in hundreds of places but only in the IWorkContext.

Here is what would be in the IworkContext implementation:

public partial class WorkContext : IWorkContext
{
    ...
    public virtual string UserId
        {
            get
            {
                return User.Identity.GetUserId()
            }
        }
}

You can define the IWorkContext interface accordingly and implement the necessary User based logic in there.

You can also have the Task property in the work context object instead of in different actions.

I have not included the Dependency Injection related code sample above. Which is not part of the question. You can read more about Dependency Injection here.

Rahatur
  • 3,147
  • 3
  • 33
  • 49