7
\$\begingroup\$

BACKGROUND

I am working on a .Net Core API to drive a lyrics application. Users can sign up, submit artists & lyrics, and earn kudos / XP points in the process. Basically a community-driven lyrics website.

CODE

I have the following ArtistsController.

[Route("api/artists")]
public class ArtistsController : Controller
{
    private readonly IArtistsService _artistsService;
    private readonly UserManager<BbUser> _userManager;

    public ArtistsController(IArtistsService artistsService, UserManager<BbUser> userManager)
    {
      _artistsService = artistsService ?? throw new ArgumentNullException(nameof(artistsService));
      _userManager = userManager;
    }

    [HttpGet]
    public async Task<IActionResult> GetArtists()
    {
      ArtistCardDtoCollection artistCardDtoCollection;
      if (User.Identity.IsAuthenticated)
      {
        var username = HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier);
        var user = await _userManager.FindByNameAsync(username);
        var roles = await _userManager.GetRolesAsync(user);
        artistCardDtoCollection = _artistsService.GetAllArtists(user.Id, roles.Contains("Admin"));
      }
      else
      {
        artistCardDtoCollection = _artistsService.GetAllArtists("", false);
      }

      return Ok( new { artists = artistCardDtoCollection });
    }

    [HttpGet("{slug}")]
    [HttpGet("{slug}/lyrics", Name = "GetArtist")]
    public async Task<IActionResult> GetArtist(string slug)
    {
      if (!_artistsService.ArtistExists(slug)) return NotFound();
      ArtistOverviewDto artistOverviewDto;
      if (User.Identity.IsAuthenticated)
      {
        var username = HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier);
        var user = await _userManager.FindByNameAsync(username);
        var roles = await _userManager.GetRolesAsync(user);
        artistOverviewDto = _artistsService.GetArtistBySlug(slug, user.Id, roles.Contains("Admin"));
      }
      else
      {
        artistOverviewDto = _artistsService.GetArtistBySlug(slug, "", false);
      }

      return Ok(artistOverviewDto);
    }

    [Authorize]
    [HttpPost]
    public async Task<IActionResult> CreateArtist([FromBody] ArtistCreateDto artistCreateDto)
    {
      if (artistCreateDto == null) return BadRequest();
      if (!ModelState.IsValid) return BadRequest(ModelState);
      var username = HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier);
      var user = await _userManager.FindByNameAsync(username);
      var artistOverviewDto = await _artistsService.AddNewArtist(user.Id, artistCreateDto);
      if (artistOverviewDto == null) return NotFound();
      return CreatedAtRoute("GetArtist", new {artistOverviewDto.Slug}, artistOverviewDto);
    }
}

I have 3 methods. One to get a list of artists, one to get a single artist and finally, one to create a new artist (you need to be logged in for that one).

How would you improve the code above? Readability of code is very important to me, and right now I don't feel it particularly reads well.

\$\endgroup\$
6
\$\begingroup\$

Here I will discuss only about your API, I think something may also be changed in your domain model but I don't see enough code to suggest anything really meaningful.

You have some code duplication you can avoid, extract this:

private async (string UserId, IEnumerable<string> Roles) GetCurrentUserAndRole()
{
    if (!User.Identity.IsAuthenticated)
        return ("", Enumerable.Empty<string>());

    var username = HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier);
    var user = await _userManager.FindByNameAsync(username);
    var roles = await _userManager.GetRolesAsync(user);

    return (user.id, roles);
}

Let's introduce an helper method to determine if user is an administrator:

private static bool IsAdministrator(IEnumerable<string> roles)
    => roles.Contains("Admin");

Now API methods might be simplified:

[HttpGet]
public async Task<IActionResult> GetArtists()
{
  (var userId, var roles) = await GetCurrentUserAndRole();      
  var artists = _artistsService.GetAllArtists(userId, IsAdministrator(roles));

  return Ok(new { artists = artists });
}

Exactly same thing may be applied to all the other methods. Note that names can be simplified, the list of artists does not need to be artistCardDtoCollection when simply artists is enough.

Now we have a problem. I don't know how you setup your tests but I usually like to test different things in different layers, controllers are easy to test if I can simply create an instance of an object, call a method and assert about its result. In this way I can verify all the controller logic (that should be pretty small) in simple unit testing and then check for HTTP related stuff in integration tests (slower and often tedious to write). Can I do it here? No, because GetCurrentUserAndRole() needs HttpContext and User and they're not available when I manually created an instance of ArtistsController. It's also hard to try this logic because I need the whole authentication thing to try both code paths. Let's move this logic into a separate IPermissionsService (or similar name). You get an instance of this service from your favorite DI framework and you can mock it as you prefer:

[HttpGet]
public async Task<IActionResult> GetArtists()
{
  var permissions = await _permissionsService.GetPermissions(HttpContext);      

  var artists = _artistsService.GetAllArtists(permissions.UserId,
      permissions.IsAdministrator);

  return Ok(new { artists = artists });
}

Now you can test it:

[TestMethod]
public async void IfUserIsAdministratorThenXyz()
{
    var artistsService = ...;
    var userManager = ...;
    var permissionsManager = new MockPermissionsManager(...);

    var controller = new ArtistsController(artistsService,
        userManager, permissionsManager);
    var response = await controller.GetArtists();

    // Check result and HTTP status code...
}

This is not the only (or the best) way to solve the problem but definitely something you should think about.

We're creating an anonymous object here, I strongly suggest to add a proper model object such as ArtistListModel. In .NET Core default naming conversion for serialized JSON objects changed, now you can name properties with PascalCase and they get serialized in camelCase.

When you create a new artist and AddNewArtist() fails you're returning 404 Not Found, it's not exactly the expected HTTP status code if something went wrong.

You're not catching any exception. I do not think that this code won't ever fail in any circumstance (and a generic 500 Server Error is somehow unprofessional if it can be avoided with a more meaningful error message).

To create an object you might consider to use PUT instead of POST. I say consider because it's an opinionated guideline which I won't discuss here. Also a redirect as result of an API call isn't completely clear to me, leave this design decision to Presentation. An API call should return 201 Created and caller has the responsibility to read the object content (if interested); this is presentation logic, not domain logic. If you want to inform the client about the location of the newly created object then you can manually add Location HTTP header in your response (even if it has been originally intended to be served only with 3xx responses, it is somehow common in API interface):

201 Created
Location: http://example.com/api/v1/artists/1234

As I said I won't discuss these issues here but someone suggests to return resource content in the body of the request when returning 201 Created (in this case you do not even need to add Location header) or an empty body when returning 202 Accepted. Think about it and stick to one convention everywhere.

For documentation purposes I'd also add [ProducesResponseType] attributes to your API methods. It will help you to understand the code later and it'll make automatically generated documentation (for example with Swagger) much more complete and useful.

\$\endgroup\$
  • \$\begingroup\$ First of all, I'd like to thank you @AdrianoRepetti. I learnt more in this single post of yours than the previous two weeks I spent on StackOverflow :) I have some more questions (if you don't mind). Where would you put the IPermissionsService.cs and its implementation? Would they have to sit in my .Api project? I have the following organisation for my solution. Finally, where would you recommend I put caching and what sort of caching? I may have 1000 artists, and do not want to always query the Db for them \$\endgroup\$ – J86 Aug 17 '17 at 8:59
  • 1
    \$\begingroup\$ I think I'd put in the API project. They might split to separate projects but IMO only when application is complex enough. Caching? First I'd introduce pagination (in the controller). Do not assume you will need caching (and to keep it in sync is much more complex than you may expect). Where to put cache...well it depends which ORM you're using. I'd say in the Controller (or in the Repository object, if you have one) but ORM support and hosting context play an important role (for example if you're using Azure and/or Entity Framework there are extension points for a 2nd level cache) \$\endgroup\$ – Adriano Repetti Aug 17 '17 at 9:23
  • \$\begingroup\$ Thank you, Adriano. Am I right that the PermissionsService.cs has to take in the ClaimsPrincipal ControllerBase.User in its constructor along with the HttpContext? \$\endgroup\$ – J86 Aug 17 '17 at 10:21
  • \$\begingroup\$ User isn't required (for common scenarios you get it with HttpContext.User) then you can go with HttpContext only. I didn't check (yet) how things look like in ASP.NET Core there but I'd beat it's now much easier to also mock an HttpContext (without all those static stuff we had in ASP.NET...) \$\endgroup\$ – Adriano Repetti Aug 17 '17 at 10:29
  • 1
    \$\begingroup\$ With HttpContext.User.Identity.IsAuthenticated \$\endgroup\$ – Adriano Repetti Aug 17 '17 at 10:32
5
\$\begingroup\$
if (User.Identity.IsAuthenticated)
{
    var username = HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier);
    var user = await _userManager.FindByNameAsync(username);
    var roles = await _userManager.GetRolesAsync(user);
    artistCardDtoCollection = _artistsService.GetAllArtists(user.Id, roles.Contains("Admin"));
}

This and the accepted answer is not how user authorization and role checking should be implemented. With this techinique you need to put the same boilerplate code in each and every action method.

Instead you should implement the IAuthorizationFilter that you can use to decorate either the controller or the particular action methods and set the permisstions.

You can read how to do it for example here: Custom Authorization in MVC or just use the Role based Authorization

\$\endgroup\$
  • 2
    \$\begingroup\$ AFAIK if you're using the user role to filter out available data from EF then you can't use role based authorization (can't use = unrelated), it's not code to handle authorization/access permissions according to external rules but to restrict access to a subset of available data. \$\endgroup\$ – Adriano Repetti Aug 17 '17 at 14:26
  • \$\begingroup\$ @AdrianoRepetti I find it's better to have two action methods with clear authorization each (and do the actual filtering there) then hide this and doing some kind of a fake authorization in addition to retrieving the data. \$\endgroup\$ – t3chb0t Aug 18 '17 at 19:42
  • \$\begingroup\$ @AdrianoRepetti I see we're even and both got downvoted ;-] \$\endgroup\$ – t3chb0t Aug 18 '17 at 19:43
  • \$\begingroup\$ Hmmmmmmmm I didn't think to use it in that way but hmmmmm maybe it's not a bad idea,at first sight it looks overcomplex (to me) for this task but it might be better when things got complicated. I have a simple project I wrote in the other way, next week I'll try to rewrite one controller in this way. \$\endgroup\$ – Adriano Repetti Aug 18 '17 at 19:54
  • \$\begingroup\$ Without a comment in one way or another. That's useless... \$\endgroup\$ – Adriano Repetti Aug 18 '17 at 19:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.