Sprawdzenie uprawnień przed zmianą rekordów?

Cześć, nie wiem, czy to bardziej .NET, czy Inżynieria oprogramowania. Już mówię o co mi chodzi.
Mam aplikację w Blazor (chociaż równie dobrze może to być WebApi).

Teraz tak. Klient się loguje. Wciska guzik: "Pokaż moje firmy".

Teraz w serwisie idzie zapytanie do bazy w stylu:

public async Task<List<Company>> GetCompaniesForUser(Guid userId)
{
  return await dbContext.Companies.Where(c => c.OwnerId == userId).ToListAsync();
}

I teraz pytanie, czy powinienem w serwisie zrobić sprawdzenie, czy zalogowany user może pobrać te dane? Np:

public async Task<List<Company>> GetCompaniesForUser(Guid userId)
{
  if(loggedUserProvider.GetLoggedUserId() == userId || loggedUserProvider.LoggedIsAdmin())  
    return await dbContext.Companies.Where(c => c.OwnerId == userId).ToListAsync();
  else
    throw new AuthorizationException();
}

Czy jest różnica, czy robię to w Blazor (bez udziału GET), czy w WebApi? Jak duże znaczenie ma, że kluczem jest GUID a nie int?

Jak zależy Ci żeby jeden user nie podejrzał danych drugiego to jak najbardziej sprawdzaj. Czy to guid czy int to nie ma wiekszego znaczenia. I sprawdzalym to juz kontrolerze bez rzucania wyjatkiem, ale to ja, nie mówie ze tak trzeba robic :)

kzkzg napisał(a):

Jak zależy Ci żeby jeden user nie podejrzał danych drugiego

Najstarsi Indianie mówią, że szansa na podejrzenie bardzo spada, jak kluczem nie jest integer a UUID/GUID

Juhas napisał(a):

Jak duże znaczenie ma, że kluczem jest GUID a nie int?

To pewnie zależy jak masz dużą bazę. https://stackoverflow.com/questions/11938044/what-are-the-best-practices-for-using-a-guid-as-a-primary-key-specifically-rega/11938495

A co do pytania gdzie to robić to właśnie dlatego uwielbiam mediatora. Bo jest elastyczny i można zawsze taką autoryzacje gdzieś zapakować
https://medium.com/@austin.davies0101/creating-a-basic-authorization-pipeline-with-mediatr-and-asp-net-core-c257fe3cc76b

Dorzucę kilka best practice z mojej strony:

• nigdy nie powinno się ufać parametrom z api i trzeba je sprawdzać (acz z reguły w intranetowych apkach w korpo się tego nie robi, must have w apkach internetowych).
• po if'ie otwieraj klamry, nawet jak to jest jedno linijkowiec
• staraj unikać się zagnieżdżeń - domyślna logika niech będzie na głównym wcięciu metody, a nie w if'ie czy elsie

Osobiście bym bardziej w coś takiego poszedł, gdzie zwykły user nie podaje w ogóle userId

public Task<List<Company>> GetCompaniesForUser(Guid? userId = null)
{
  if (userId != null && !loggedUserProvider.LoggedIsAdmin())
  {
    throw new AuthorizationException();
  }

  userId ??= loggedUserProvider.GetLoggedUserId();

  var result = dbContext.Companies.Where(c => c.OwnerId == userId).ToListAsync();
  return result;
}

• obsługa przez rzucanie wyjątku to też niezbyt dobry practice, acz dla czegoś co jest faktycznie wyjątkową akcją to jeszcze ujdzie
• w domyślnym flow nie musisz dodatkowo sprawdzać czy user jest adminem, skoro nic tego nie wymaga
• zawsze resulty zwracam sobie przez var result, dzięki temu od razu w debuggerze mogę podejrzeć jej wynik, który nie zawsze można podejrzeć w pseudozmiennej $returnvalue
• na co komu await i async skoro i tak zwracasz Taska ? Niech kod nadrzędny sobie awaituje i może ktoś wyżej wykorzysta wiele wątków wykonując coś faktycznie równolegle używając np. await Task.WhenAll(twojeRównoległeTaski);

Wiem, że robimy offtop, ale o kilku rzeczach chcę podyskutować

• po if'ie otwieraj klamry, nawet jak to jest jedno linijkowiec

Czemu? To imho brudzi kod. Tak jak nadmiarowe komentarze. Co jest w tym lepszego? Czy to kwestia tylko gustu?

• staraj unikać się zagnieżdżeń - domyślna logika niech będzie na głównym wcięciu metody, a nie w if'ie czy elsie

Widzę różnicę w moim i Twoim kodzie. No... dobra. Przyznaję rację. Twój wygląda czyściej pod tym względem.

• obsługa przez rzucanie wyjątku to też niezbyt dobry practice, acz dla czegoś co jest faktycznie wyjątkową akcją to jeszcze ujdzie

No tak. Tutaj podszedłem bardziej "przykładowo". Chociaż nie wiem, co jest w takim wypadku lepsze. Rzucenie wątku i złapanie go np. w kontrolerze, czy zwrócenie czegoś w stylu IdentityResult. Nie mniej jednak jest to CHYBA wyjątkowa sytuacja, że ktoś próbuje pobrać dane, do których nie powinien mieć dostępu.

• w domyślnym flow nie musisz dodatkowo sprawdzać czy user jest adminem, skoro nic tego nie wymaga

Admin może pobrać wszystkie zasoby. I to jest ten wymóg. Dlatego sprawdzenie, czy pytasz o swoje, czy jesteś adminem.

• na co komu await i async skoro i tak zwracasz Taska ? Niech kod nadrzędny sobie awaituje i może ktoś wyżej wykorzysta wiele wątków wykonując coś faktycznie równolegle używając np. await Task.WhenAll(twojeRównoległeTaski);

Gdzieś kiedyś przeczytałem, że async/await jest jak rak. Jak już się gdzieś pojawi, to powinien być wszędzie. Stąd to podejście.

Ja korzystam z ról przypisanych użytkownikom i przy każdej akci która wymaga autoryzacji mam nagłówek "authorize" z rolą jaką jest wymagana. Według mnie jest to lepsze niż sprawdzanie ifem. https://docs.microsoft.com/pl-pl/aspnet/core/security/authorization/roles?view=aspnetcore-5.0

@Juhas async i await rozrasta się, gdy potrzebujesz operować na jego wynikach, przez co rozrasta się w górę, a nie w dół wywołań, gdzie możesz przekazywać same Taski. Niestety wiele razy widziałem używanie awaita jak w Twoim kodzie, a jest to błąd, bo taki await niczego nie daje - czekasz na wynik Taska, by go znowu zapakować w Taska i wysłać wyżej. Co innego jakbyś jakoś na tych danych operował.

o if i klamrach @abrakadaber już przytoczył historię i sam miałem podobną. Niby więcej kodu ale jak np taki Ci się trafi, to znalezienie takowego błędu nie jest takie proste. Inaczej zaczną pojawiać się cuda typu else w tej samej linijce, a od tego już niedaleka droga do średnika na końcu ifa.

@gswidwa1 podał fajny przykład z używaniem authorize i też w swoich projektach z tego korzystam, acz sam temat autentyfikacji i autoryzacji jest dosyć szerokim i sam dawno temu wolałem to uprościć sobie flagą dla admina.

Co do zwracania exceptiona, to nie raz widziałem np walidację na Exceptionach, co jest błędem i głównie z tego powodu o tym wspomniałem. Tworzenie obiektu Exceptiona, z całym stacktraceem jest czasochłonne i pamięciożerne i nadaje się tylko do faktycznie wyjątkowej sytuacji. Niestety kod w którym co chwila jest try i catch jest mało czytelny, więc na pewno nie powinno się robić try i catch w kontrolerze. Od tego jest globalny error handler, a try i catche to zaraz przy miejscu gdzie faktycznie przewidujesz, że coś się zdarzy i chcesz to obsłużyć (np przy pracy na plikach). Jeśli robi się pustego catch albo catch(Exception ex) to raczej używa się tego błędnie, catch powinien łapać konkretny wyjątek.

Tak jak wspomniałeś kod z wieloma komentarzami też nie jest dobry. Nazwy metod i zmiennych powinny z reguły wystarczać. Co mi po komentarzach, jak jedna metoda ma 1000+ linii kodu, z czego połowa to komentarze lub ify w ifie i zanim dojdziesz do środka to nie pamiętasz początku. Jednak literka S w SOLID jest bardzo ważna, a w brew pozorów bardzo dużo programistów zupełnie o tym zapomina, bo po co tworzyć osobną klasę dla kilku linijek kodu... a potem to się rozrasta do potworów.

Do zwracania wyniku spodziewanej ale nawet i błędnej akcji najlepiej się nadaje klasa z tym wynikiem. Jeśli dobrze zaprojektuje się klasę resulta i jego obsługę to wtedy prosto każdy przypadek zwrócić i wyświetlić użytkownikowi.

@gswidwa1: No tak, tylko

gswidwa1 napisał(a):

Ja korzystam z ról przypisanych użytkownikom i przy każdej akci która wymaga autoryzacji mam nagłówek "authorize" z rolą jaką jest wymagana. Według mnie jest to lepsze niż sprawdzanie ifem. https://docs.microsoft.com/pl-pl/aspnet/core/security/authorization/roles?view=aspnetcore-5.0

No tak, tylko to mi nie załatwia mojego problemu: jeśli użytkownik pobiera WŁASNY rekord lub jest adminem.

A zwrócenie authorization exception jest jakimś ważnym wymaganiem biznesowym?
Bo ja bym odfiltrował na poziomie warstwy DAO i po prostu nie znalazł tego rekordu.

Nie jest to żadne wymaganie, tylko przykład.

A przy okazji... Załóżmy, że uda mi się stworzyć taką politykę, która przepuści konkretnego użytkownika lub admina. Ale jak to później testować? Chciałbym móc testować, kto może pobrać/usunąć rekord. Wtedy sytuacja ze sprawdzeniem tego w serwisie wydaje mi się najlepsza.

No jeśli koniecznie chcesz mieć to sprawdzanie uprawnień, to wypadałoby to zrobić w warstwie aplikacji, czyli np. w serwisie.