Co oznacza „użytkownik nie powinien decydować, czy jest administratorem, czy nie. System uprawnień lub bezpieczeństwa powinien. ”

55

Przykład użyty w pytaniu przekazuje absolutną minimalną ilość danych do funkcji, która dotyczy najlepszego sposobu ustalenia, czy użytkownik jest administratorem, czy nie. Jedną z powszechnych odpowiedzi było:

user.isAdmin()

To skłoniło do komentarza, który został kilkakrotnie powtórzony i wielokrotnie głosowany:

Użytkownik nie powinien decydować, czy jest administratorem, czy nie. System uprawnień lub bezpieczeństwa powinien. Coś ściśle związanego z klasą nie oznacza, że ​​dobrym pomysłem jest włączenie jej do klasy.

Odpowiedziałem,

Użytkownik nic nie decyduje. Obiekt / tabela użytkownika przechowuje dane o każdym użytkowniku. Faktyczni użytkownicy nie mogą zmienić wszystkiego o sobie.

Ale to nie było produktywne. Oczywiście istnieje zasadnicza różnica w perspektywie, która utrudnia komunikację. Czy ktoś może mi wyjaśnić, dlaczego user.isAdmin () jest zły, i narysować krótki szkic tego, jak to wygląda „zrobione”?

Naprawdę nie widzę korzyści z oddzielenia bezpieczeństwa od chronionego systemu. W każdym tekście dotyczącym bezpieczeństwa powiedziano, że zabezpieczenia muszą być projektowane od początku w systemie i rozważane na każdym etapie rozwoju, wdrażania, konserwacji, a nawet pod koniec okresu użytkowania. To nie jest coś, co można przykręcić z boku. Ale jak dotąd 17 głosów za tym komentarzem mówi, że brakuje mi czegoś ważnego.

GlenPeterson
źródło
8
Nie sądzę, że jest źle, zwłaszcza jeśli jest to tylko flaga w tabeli db użytkownika. Jeśli w przyszłości będą zmiany, zmień je w przyszłości. Równie dobrze możesz wywołać system bezpieczeństwa z obiektu użytkownika. Przeczytałem kiedyś o wzorcu projektowym, że każdy dostęp do bazy danych / zasobów powinien przechodzić przez obiekt bieżącego użytkownika, aby upewnić się, że nigdy nie spróbujesz zrobić czegoś, co jest niedozwolone. Może to być trochę za dużo, ale użytkownik może zapewnić obiekt „uprawnień”, który obejmuje wszystkie rzeczy związane z bezpieczeństwem.
Głowonogi
Myślę, że było nieporozumienie. Dla Ciebie „Użytkownik” oznacza osobę, która korzysta z oprogramowania. Dla nich „Użytkownik” oznacza kod, który korzysta z Twojej funkcjonalności.
Filip
2
Nie to, że pytałeś, ale w tym przypadku najbardziej uważam na metodę IsAdmin (), gdziekolwiek ją umieścisz. Zwykle zalecałbym, aby „Administrator” nie był zakodowany na stałe, ale miał tylko zestaw możliwych uprawnień, a „Administrator” po prostu ma cały zestaw. W ten sposób, gdy będziesz musiał później podzielić rolę administratora, wszystko jest znacznie łatwiejsze. Ogranicza to logikę specjalnych przypadków.
psr
1
@Philipp Nie sądzę ... Obie strony wyraźnie mówią o Userobiekcie isAdmin()metodą (cokolwiek robi za kulisami)
Izkata,
Za późno na edycję; Miałem na myśli „obie strony”, a nie „obie strony” ... = P
Izkata

Odpowiedzi:

12

Pomyśl o użytkowniku jako kluczu, a uprawnienia jako wartości.

Różne konteksty mogą mieć różne uprawnienia dla każdego użytkownika. Ponieważ uprawnienia dotyczą kontekstu, musisz zmienić użytkownika dla każdego kontekstu. Lepiej więc umieścić tę logikę gdzie indziej (np. Klasę kontekstową) i po prostu sprawdzić uprawnienia w razie potrzeby.

Efektem ubocznym jest zwiększenie bezpieczeństwa systemu, ponieważ nie można zapomnieć o usunięciu flagi administratora dla miejsc, w których nie ma to znaczenia.

Wilbert
źródło
40

Ten komentarz był źle sformułowany.

Nie chodzi o to, czy obiekt użytkownika „decyduje”, czy jest administratorem, użytkownikiem próbnym, superużytkownikiem, czy czymś niezwykłym. Oczywiście to nie decyduje o żadnej z tych rzeczy, tak jak cały system oprogramowania, tak jak Ty. Chodzi o to, czy klasa „user” powinna reprezentować role nazwy głównej, którą reprezentują jej obiekty, czy też jest to odpowiedzialność innej klasy.

Kilian Foth
źródło
6
Ojej (to był mój komentarz), ale stawiasz to o wiele lepiej niż ja.
Marjan Venema
Mówisz o modelowaniu ról w osobnej klasie roli, a nie w klasie użytkownika? Nie polecasz osobnego systemu, takiego jak LDAP? Co się stanie, jeśli każda pojedyncza decyzja klasy Role wymaga zapytania do odpowiedniego rekordu użytkownika i nie wymaga żadnych innych informacji? Czy nadal powinien być odrębny od użytkownika, a jeśli tak, to dlaczego?
GlenPeterson
2
@GlenPeterson: niekoniecznie, tylko tabele klas <> i zwykle jest o wiele więcej klas do rozróżnienia niż tabele. Bycie zorientowanym na dane prowadzi jednak do zdefiniowania podlist na klasach, gdy wiele razy listy te powinny być zdefiniowane w innej „księdze głównej” (zleceniach) lub niektórych takich klasach przekazujących użytkownika (lub zlecenie), dla których ... To bardziej kwestia odpowiedzialności niż czasowników i rzeczowników.
Marjan Venema
9
Pomyśl o użytkowniku jako kluczu, a uprawnienia jako wartości. Różne konteksty mogą mieć różne uprawnienia dla każdego użytkownika. Ponieważ uprawnienia dotyczą kontekstu, musisz zmienić użytkownika dla każdego kontekstu. Lepiej więc umieścić tę logikę gdzie indziej (np. Klasa kontekstu).
Wilbert
2
@Wilbert: Kontekst! To jest szczególnie pouczające! Tak, jeśli masz więcej niż jeden kontekst dla tych uprawnień, wszystko ma dla mnie sens. Zwykle widzę te uprawnienia w jednym kontekście, a naklejenie ich na użytkownika jest w tym przypadku najprostszym rozwiązaniem. Oczywiście nie należą do nich, jeśli stosują się inaczej w drugim kontekście. Jeśli napiszesz to jako odpowiedź, prawdopodobnie to zaakceptuję. Dziękuję Ci.
GlenPeterson
30

Nie zdecydowałbym się wypowiedzieć oryginalnego komentarza w sposób, w jaki został sformułowany, ale identyfikuje on potencjalnie uzasadniony problem.

W szczególności obawy, które uzasadniają separację, to uwierzytelnianie vs. autoryzacja .

Uwierzytelnianie odnosi się do procesu logowania i uzyskiwania tożsamości. W ten sposób systemy wiedzą, kim jesteś , i są wykorzystywane do personalizacji, własności obiektu itp.

Autoryzacja odnosi się do tego, co wolno ci robić , a to (ogólnie) nie zależy od tego, kim jesteś . Zamiast tego zależy to od niektórych zasad bezpieczeństwa, takich jak role lub uprawnienia, które nie dbają o takie rzeczy, jak imię i nazwisko lub adres e-mail.

Ci dwaj mogą zmieniać się ortogonalnie. Na przykład możesz zmienić model uwierzytelniania, dodając dostawców OpenID / OpenAuth. Możesz także zmienić politykę bezpieczeństwa, dodając nową rolę lub zmieniając RBAC na ABAC.

Jeśli wszystko to zostanie ujęte w jedną klasę lub abstrakcję, wówczas kod bezpieczeństwa, który jest jednym z najważniejszych narzędzi ograniczania ryzyka , staje się, jak na ironię, wysokim ryzykiem.

Pracowałem z systemami, w których uwierzytelnianie i autoryzacja były zbyt ściśle powiązane. W jednym systemie istniały dwie równoległe bazy danych użytkowników, każda dla jednego rodzaju „roli”. Osoba lub zespół, który ją zaprojektował, najwyraźniej nigdy nie uważał, że pojedynczy użytkownik fizyczny może pełnić obie role lub że mogą istnieć pewne działania, które były wspólne dla wielu ról lub że mogą występować problemy z kolizjami ID użytkownika. Jest to wprawdzie skrajny przykład, ale praca z nim była / jest niezwykle bolesna.

Microsoft i Sun / Oracle (Java) określają agregację informacji o uwierzytelnianiu i autoryzacji jako Principal Security . Nie jest idealny, ale działa dość dobrze. W .NET, na przykład, masz IPrincipal, który obudowujeIIdentity - dawna będąc polityka (autoryzacja) przedmiot, a drugi jest tożsamość (uwierzytelnianie). Można rozsądnie zakwestionować decyzję o umieszczeniu jednego w drugim, ale ważne jest to, że większość pisanego kodu będzie dotyczyła tylko jednej z abstrakcji, co oznacza, że ​​łatwo go przetestować i zmienić.

Nie ma nic złego w User.IsAdminpolu ... chyba że jest też User.Namepole. Oznaczałoby to, że koncepcja „użytkownika” nie jest właściwie zdefiniowana i jest to niestety bardzo częsty błąd programistów, którzy są nieco mokrzy za uszami, jeśli chodzi o bezpieczeństwo. Zazwyczaj jedyną rzeczą, która powinna być współdzielona przez tożsamość i zasady, jest identyfikator użytkownika, który nieprzypadkowo jest dokładnie taki, jak jest implementowany zarówno w modelach bezpieczeństwa Windows, jak i * nix.

Całkowicie dopuszczalne jest tworzenie obiektów opakowujących, które zawierają zarówno tożsamość, jak i zasady. Ułatwi to na przykład utworzenie ekranu deski rozdzielczej, na którym oprócz różnych widżetów lub łączy, do których bieżący użytkownik ma dostęp, należy wyświetlić komunikat „cześć”. Tak długo, jak to opakowanie po prostu otacza informacje dotyczące tożsamości i zasad i nie rości sobie prawa do ich posiadania. Innymi słowy, o ile nie jest prezentowany jako zagregowany katalog główny .

Uproszczony model bezpieczeństwa zawsze wydaje się dobrym pomysłem przy pierwszym projektowaniu nowej aplikacji, ze względu na YAGNI i tak dalej, ale prawie zawsze wraca, by cię ugryźć później, ponieważ, niespodzianka, nowe funkcje są dodawane!

Jeśli więc wiesz, co jest dla Ciebie najlepsze, informacje dotyczące uwierzytelniania i autoryzacji będą oddzielone. Nawet jeśli „autoryzacja” jest teraz tak prosta jak flaga „IsAdmin”, nadal będziesz lepiej, jeśli nie jest częścią tej samej klasy lub tabeli co informacje uwierzytelniające, więc jeśli i kiedy twoja polityka bezpieczeństwa musi zmiana, nie musisz wykonywać operacji rekonstrukcyjnych w systemach uwierzytelniania, które już działają dobrze.

Aaronaught
źródło
11
Och, szkoda, że ​​nie miałem czasu tego napisać. Z drugiej strony, prawdopodobnie nie byłbym w stanie tego powiedzieć tak dobrze, jak ty, bez większej ilości przemyśleń z mojej strony. Wiele razy mój instynkt jelitowy każe mi iść w jedną lub drugą stronę, ale wyjaśniam, dlaczego - sobie lub komuś innemu - potrzeba dużo więcej myśli i wysiłku. Dzięki za ten post. Byłoby wiele plusów, ale SE nie pozwala mi ...
Marjan Venema
Brzmi to szokująco podobnie do projektu, nad którym pracuję, a twoja odpowiedź dała mi inną perspektywę. Dziękuję bardzo!
Brandon
„Nie ma nic złego w polu User.IsAdmin ... chyba że istnieje również pole User.Name”, które piszesz. Ale jaka isAdminjest metoda? Może po prostu delegować obiekt, którego zadaniem jest śledzenie uprawnień. To, co jest najlepszą praktyką w projektowaniu modelu relacyjnego (jakie pola posiada jednostka), niekoniecznie przekłada się na najlepszą praktykę w modelu OO (na jakie komunikaty obiekt może odpowiedzieć).
KaptajnKold
@KaptajnKold: Czytam ten sentyment w różnych wątkach Q&A na tym forum. Jeśli metoda istnieje, nie ma znaczenia, czy wykonuje ona bezpośrednio operacje, czy je deleguje , nadal liczy się jako odpowiedzialność, ponieważ inne klasy mogą na niej polegać . Twój User.IsAdminmoże równie dobrze być jedno-liner, że nic nie robi, ale rozmowy PermissionManager.IsAdmin(this.Id), ale jeśli robi się odwoływać przez 30 innych klas, nie można zmienić lub usunąć. Właśnie dlatego istnieje SRP.
Aaronaught
I, @KaptajnKold, jeśli chodzi o modele relacyjne, nie jestem pewien, do czego zmierzasz; jeszcze gorzej jest mieć IsAdmin pole . Jest to rodzaj pola, które ma tendencję do pozostawania na długo po tym, jak system ewoluuje w RBAC lub coś bardziej złożonego i stopniowo nie synchronizuje się z „prawdziwymi” uprawnieniami, a ludzie zapominają, że nie powinni go już używać, i ... cóż, po prostu powiedz nie. Nawet jeśli uważasz, że masz tylko dwie role: „admin” i „nieadministrator”, nie przechowuj go jako pola logicznego / bitowego, poprawnie normalizuj i posiadaj tabelę uprawnień.
Aaronaught
28

Cóż, przynajmniej narusza zasadę pojedynczej odpowiedzialności . Czy powinny nastąpić zmiany (wiele poziomów administratora, jeszcze więcej różnych ról?) Tego rodzaju projekt byłby dość niechlujny i okropny w utrzymaniu.

Rozważ zaletę oddzielenia systemu bezpieczeństwa od klasy użytkownika, aby oba mogły mieć jedną, dobrze zdefiniowaną rolę. W efekcie powstaje czystszy, łatwiejszy w utrzymaniu projekt.

Zavior
źródło
2
@GlenPeterson: Nie, użytkownicy mogą bardzo dobrze istnieć bez zabezpieczeń. Co z moim imieniem, nazwą wyświetlaną, moim adresem, moim adresem e-mail itp. Gdzie je umieścisz? Identyfikacja (uwierzytelnienie) i autoryzacja to różne bestie.
Marjan Venema
2
class Userjest kluczowym składnikiem trójki zabezpieczeń: identyfikacji, uwierzytelnienia i autoryzacji . Są one ściśle powiązane na poziomie projektu, więc nie jest nierozsądne, aby kod odzwierciedlał tę współzależność.
MSalters
1
Pytanie brzmi, czy klasa User powinna zawierać całą logikę do obsługi trypletu!
Zavior,
3
@MSalters: jeśli jest to interfejs API do takiej logiki, wie o niej, nawet jeśli deleguje gdzie indziej dla dokładnej logiki. Chodzi o to, że Autoryzacja (uprawnienia) dotyczy połączenia użytkownika z czymś innym i dlatego nie powinna być częścią ani użytkownika, ani czymś innym, ale częścią systemów uprawnień, które słusznie znają zarówno użytkowników, jak i cokolwiek innego. otrzymuje pozwolenie na.
Marjan Venema,
2
Should there be changes- cóż, nie wiemy, że będą zmiany. YAGNI i wszyscy inni. Czy zmieniają się, kiedy stają się konieczne, prawda?
Izkata,
10

Myślę, że problemem, być może źle sformułowanym, jest to, że kładzie on zbyt duży nacisk na Userklasę. Nie, nie ma problemu z bezpieczeństwem posiadania metody User.isAdmin(), jak sugerowano w tych komentarzach. W końcu, jeśli ktoś jest wystarczająco głęboko w twoim kodzie, aby wstrzyknąć szkodliwy kod dla jednej z twoich podstawowych klas, masz poważne problemy. Z drugiej strony nie ma sensu Userdecydować, czy jest administratorem w każdym kontekście. Wyobraź sobie, że dodajesz różne role: moderatorzy forum, redaktorzy, którzy mogą publikować posty na stronie głównej, autorzy, którzy mogą pisać treści, które redaktorzy mogą publikować, i tak dalej. Podejście polegające na umieszczeniu go na Userobiekcie Userspowoduje powstanie zbyt wielu metod i zbyt dużej logiki, która nie będzie miała bezpośredniego związku z klasą.

Zamiast tego można użyć wyliczenia różnych typów uprawnień i PermissionsManagerklasy. Być może możesz mieć metodę typu PermissionsManager.userHasPermission(User, Permission)zwracającą wartość logiczną. W praktyce może wyglądać następująco (w java):

if (!PermissionsManager.userHasPermission(user, Permissions.EDITOR)) {
  Site.renderSecurityRejectionPage();
}

Zasadniczo jest to metoda before_filtermetody Rails , która stanowi przykład tego typu sprawdzania uprawnień w świecie rzeczywistym.

asthasr
źródło
6
Czym to się różni od tego user.hasPermission(Permissions.EDITOR), że jest bardziej gadatliwe i proceduralne niż OO?
Głowonóg
9
@Arian: ponieważ user.hasPermissionsnakłada zbyt wiele obowiązków na klasę użytkowników. Wymaga od użytkownika znajomości uprawnień, podczas gdy użytkownik (klasa) powinien istnieć bez takiej wiedzy. Nie chcesz też, aby klasa użytkownika wiedziała, jak samemu drukować lub wyświetlać (budować formularz), pozostawiasz to rendererowi / konstruktorowi drukarki lub klasie renderera / konstruktora wyświetlania.
Marjan Venema
4
user.hasPermission(P)jest poprawnym API, ale to tylko interfejs. Prawdziwa decyzja musi oczywiście zostać podjęta w podsystemie bezpieczeństwa. Aby odpowiedzieć na komentarz syrion dotyczący testowania, podczas testowania można było zamienić ten podsystem. Zaletą tego prostego rozwiązania user.hasPermission(P)jest to, że nie zanieczyszczasz całego kodu tylko po to, abyś mógł go przetestować class User.
MSalters
3
@MarjanVenema Zgodnie z tą logiką użytkownik pozostanie obiektem danych bez żadnych zobowiązań. Nie twierdzę, że całe zarządzanie uprawnieniami powinno być zaimplementowane w klasie użytkownika, jeśli istnieje potrzeba tak złożonego systemu. Mówisz, że użytkownik nie musi wiedzieć o uprawnieniach, ale nie rozumiem, dlaczego każda inna klasa musi wiedzieć, jak rozwiązać uprawnienia dla użytkownika. To sprawia, że ​​drwiny i testy są znacznie trudniejsze.
Głowonogi
6
@Arian: podczas gdy anemiczne klasy są zapachem, niektóre klasy po prostu nie mają żadnego zachowania ... Użytkownik, imho, jest jedną z takich klas, która lepiej nadaje się jako parametr do całej masy innych rzeczy (którzy chcą robić różne rzeczy / podejmuj decyzje w oparciu o to, kto o nie prosi), niż używać go jako kontenera dla wszelkiego rodzaju zachowań tylko dlatego, że wygodnie jest go tak kodować.
Marjan Venema
9

Object.isX () służy do przedstawienia wyraźnego związku między obiektem a X, który można wyrazić jako wynik boolowski, na przykład:

Water.isBoiling ()

Circuit.isOpen ()

User.isAdmin () zakłada jedno znaczenie „Administrator” w każdym kontekście systemu , że użytkownik jest, w każdej części systemu, administratorem.

Choć brzmi to dość prosto, rzeczywisty program prawie nigdy nie będzie pasował do tego modelu, z pewnością będzie wymóg, aby użytkownik administrował zasobem [X], ale nie [Y], lub bardziej wyraźnymi typami administratorów ([projekt ] administrator kontra administrator [systemowy]).

Ta sytuacja zazwyczaj wymaga zbadania wymagań. Rzadko, jeśli w ogóle, klient będzie chciał relacji, którą faktycznie reprezentuje User.isAdmin (), więc wahałbym się wdrożyć takie rozwiązanie bez wyjaśnienia.

FMJaguar
źródło
6

Plakat miał na myśli jedynie inny i bardziej skomplikowany projekt. Moim zdaniem User.isAdmin()jest w porządku . Nawet jeśli później wprowadzisz skomplikowany model uprawnień, nie widzę powodu, by się poruszać User.isAdmin(). Później możesz podzielić klasę użytkownika na obiekt reprezentujący zalogowanego użytkownika i obiekt statyczny reprezentujący dane o użytkowniku. A może nie. Zaoszczędź jutro na jutro.

Kevin Cline
źródło
4
Save tomorrow for tomorrow. Tak. Kiedy odkryjesz, że ściśle powiązany kod, który właśnie napisałeś, zaszufladkował cię i musisz go przepisać, ponieważ nie poświęciłeś dodatkowych 20 minut na jego oddzielenie ...
MirroredFate
5
@MirroredFate: Całkowicie możliwe jest User.isAdminpołączenie metody z dowolnym mechanizmem uprawnień bez łączenia klasy User z tym konkretnym mechanizmem.
kevin cline
3
Aby User.isAdmin był sprzężony z dowolnym mechanizmem uprawnień, nawet bez sprzężenia z określonym mechanizmem, wymaga ... no cóż ... sprzężenia. Minimalna z tego byłaby dla interfejsu. Interfejs ten po prostu nie powinien istnieć. Daje to klasie użytkownika (i interfejsowi) coś, za co po prostu nie powinien być odpowiedzialny. Aaronaught wyjaśnia to znacznie lepiej niż potrafię (podobnie jak kombinacja wszystkich innych odpowiedzi na to pytanie).
Marjan Venema,
8
@MirroredFate: Nie dbam o to, czy muszę przepisać prostą implementację, aby spełnić bardziej złożone wymagania. Ale miałem naprawdę złe doświadczenia z nadmiernie złożonymi interfejsami API zaprojektowanymi w celu spełnienia wyobrażonych przyszłych wymagań, które nigdy nie powstały.
kevin cline
3
@kevincline Podczas gdy nadmiernie złożone interfejsy API są (z definicji) złą rzeczą, nie sugerowałem, że należy pisać zbyt skomplikowane interfejsy API. Stwierdziłem, że Save tomorrow for tomorrowjest to okropne podejście projektowe, ponieważ powoduje problemy, które byłyby trywialne, gdyby system został poprawnie wdrożony znacznie gorzej. W rzeczywistości ta mentalność powoduje nadmiernie złożone interfejsy API, ponieważ warstwa po warstwie jest dodawana, aby załatać początkowy kiepski projekt. Myślę, że moja postawa jest taka measure twice, cut once.
MirroredFate
5

To naprawdę nie problem ...

Nie widzę problemu z User.isAdmin(). Z pewnością wolę to od czegoś takiego PermissionManager.userHasPermission(user, permissions.ADMIN), co w świętym imieniu SRP czyni kod mniej przejrzystym i nie dodaje nic wartościowego.

Myślę, że SRP jest interpretowany przez niektórych nieco dosłownie. Myślę, że to dobrze, nawet lepiej, jeśli klasa ma bogaty interfejs. SRP oznacza po prostu, że obiekt musi przekazać współpracownikom wszystko, co nie wchodzi w zakres jego pojedynczej odpowiedzialności. Jeśli rola administratora użytkownika jest czymś bardziej zaangażowanym niż pole boolowskie, bardzo dobrze może być delegowanie obiektu użytkownika na menedżera uprawnień. Ale to nie znaczy, że nie jest dobrym pomysłem dla użytkownika, aby zachować tę isAdminmetodę. W rzeczywistości oznacza to, że kiedy twoja aplikacja przechodzi z prostej do złożonej, musisz zmienić kod, który używa obiektu użytkownika. IOW, twój klient nie musi wiedzieć, co tak naprawdę jest potrzebne, aby odpowiedzieć na pytanie, czy użytkownik jest administratorem.

... ale dlaczego tak naprawdę chcesz to wiedzieć?

To powiedziawszy, wydaje mi się, że rzadko musisz wiedzieć, czy użytkownik jest administratorem, z wyjątkiem możliwości udzielenia odpowiedzi na inne pytanie, na przykład, czy użytkownik może wykonać określone działanie, na przykład zaktualizować widżet. W takim przypadku wolałbym mieć metodę na Widget, taką jak isUpdatableBy(User user):

boolean isUpdatableBy(User user) {
    return user.isAdmin();
}

W ten sposób widżet odpowiada za to, jakie kryteria musi spełnić, aby użytkownik mógł go zaktualizować, a użytkownik odpowiada za to, czy jest administratorem. Ten projekt jasno komunikuje o swoich zamiarach i ułatwia przejście do bardziej złożonej logiki biznesowej, jeśli i kiedy nadejdzie ten czas.

[Edytować]

Problemem nie mający User.isAdmin()i korzystaniaPermissionManager.userHasPermission(...)

Pomyślałem, że dodam do mojej odpowiedzi, aby wyjaśnić, dlaczego zdecydowanie wolę wywoływać metodę na obiekcie użytkownika niż wywoływać metodę na obiekcie PermissionManager, jeśli chcę wiedzieć, czy użytkownik jest administratorem (lub pełni rolę administratora).

Sądzę, że można założyć, że zawsze będziesz zależał od klasy User, gdziekolwiek musisz zadać pytanie, czy ten użytkownik jest administratorem? Zależności, której nie możesz się pozbyć. Ale jeśli musisz przekazać użytkownika do innego obiektu, aby zadać pytanie na ten temat, tworzy to nową zależność od tego obiektu w stosunku do tego, który już masz na Użytkowniku. Jeśli pytanie jest często zadawane, jest wiele miejsc, w których tworzysz dodatkową zależność, i jest wiele miejsc, które możesz zmienić, jeśli którakolwiek z zależności tego wymaga.

Porównaj to z przeniesieniem zależności do klasy User. Teraz nagle masz system, w którym kod klienta (kod, który musi zadać pytanie, to ten użytkownik jest administratorem ) nie jest powiązany z implementacją odpowiedzi na to pytanie. Możesz całkowicie zmienić system uprawnień i musisz tylko zaktualizować jedną metodę w jednej klasie, aby to zrobić. Cały kod klienta pozostaje taki sam.

Naleganie, by nie mieć isAdminmetody użytkownika z obawy przed stworzeniem zależności w klasie użytkownika od podsystemu uprawnień, jest moim zdaniem podobne do wydawania dolara na zarobienie grosza. Jasne, unikasz jednej zależności w klasie User, ale kosztem stworzenia jednej zależności w każdym miejscu, w którym musisz zadać pytanie. Nie jest to dobra okazja.

KaptajnKold
źródło
2
„SRP oznacza po prostu, że obiekt musi przekazać współpracownikom wszystko, co nie wchodzi w zakres jego pojedynczej odpowiedzialności” - fałsz . Definicja SRP obejmuje wszystko, co obiekt robi bezpośrednio i wszystko, co deleguje. Klasa, która wykonuje tylko jedno zadanie bezpośrednio, ale wykonuje 50 innych zadań pośrednio za pośrednictwem delegacji, nadal (prawdopodobnie) narusza SRP.
Aaronaught
KaptajnKold: Dałem +1, ponieważ zgadzam się z tobą i czuję się trochę winny z tego powodu. Twój post dodaje do dyskusji, ale nie odpowiada na pytanie.
GlenPeterson
@GlenPeterson Cóż, próbowałem odnieść się do części pytania, która brzmiała: „jak to wygląda zrobione„ dobrze ”” Ponadto: Absolutnie nie powinieneś czuć się winny z powodu zgodzenia się ze mną :)
KaptajnKold
1
@JamesSnell Może. Jeśli. Ale to szczegół implementacji, który nadal ograniczałbym w ramach metody isAdmin na User. W ten sposób kod klienta nie musi się zmieniać, gdy „administracja” użytkownika zmienia się z pola logicznego w coś bardziej zaawansowanego. Możesz mieć wiele miejsc, w których musisz wiedzieć, czy użytkownik jest administratorem i nie chcesz ich zmieniać przy każdej zmianie systemu autoryzacji.
KaptajnKold
1
@JamesSnell Być może źle się rozumiemy? Pytanie, czy użytkownik jest administratorem, nie jest tym samym, co pytanie, czy użytkownik może wykonać określone działanie . Odpowiedź na pierwsze pytanie jest zawsze niezależna od kontekstu. Odpowiedź na drugie pytanie zależy w dużym stopniu od kontekstu. Do tego chciałem się odnieść w drugiej połowie mojej pierwotnej odpowiedzi.
KaptajnKold
1

Dyskusja ta przypomniała mi post na blogu autorstwa Erica Lipperta, na który zwróciłem uwagę w podobnej dyskusji na temat projektu klasy, bezpieczeństwa i poprawności.

Dlaczego tak wiele klas ramowych jest zapieczętowanych?

W szczególności Eric podnosi kwestię:

4) Bezpieczne. sedno polimorfizmu polega na tym, że możesz ominąć przedmioty, które wyglądają jak Zwierzęta, ale w rzeczywistości są Żyrafami. Istnieją tutaj potencjalne problemy z bezpieczeństwem.

Za każdym razem, gdy implementujesz metodę, która pobiera instancję typu nieopieczętowanego, MUSISZ napisać tę metodę, aby była odporna na potencjalnie wrogie instancje tego typu. Nie możesz polegać na niezmiennikach, o których wiesz, że są prawdziwe w TWOICH implementacjach, ponieważ niektóre wrogie strony internetowe mogą podklasować twoją implementację, zastępować wirtualne metody robienia rzeczy, które psują twoją logikę i przekazują ją. Za każdym razem, gdy zamykam klasę , Potrafię pisać metody, które używają tej klasy z pewnością, że wiem, co robi ta klasa.

Może się to wydawać styczną, ale myślę, że zgadza się z punktem, w którym inni plakaty poruszali kwestię SOLIDNEJ zasady pojedynczej odpowiedzialności . Weź pod uwagę następującą złośliwą klasę jako przyczynę niewdrożenia User.IsAdminmetody lub właściwości.

public class MyUser : User
{

    public new boolean IsAdmin()
    {
        // You know it!
        return true;
    }

    // Anything else we can break today?

}

To prawda, że ​​jest to trochę IsAmdminskomplikowane : nikt jeszcze nie zaproponował stworzenia metody wirtualnej, ale może się to zdarzyć, jeśli twoja architektura użytkowników / ról stanie się dziwnie złożona. (A może nie. Zastanów się public class Admin : User { ... }: taka klasa może mieć dokładnie ten kod powyżej). Umieszczenie złośliwego pliku binarnego na miejscu nie jest częstym wektorem ataku i ma o wiele większy potencjał do chaosu niż niejasna biblioteka użytkowników - to znowu może to być błąd eskalacji uprawnień, który otwiera drzwi do prawdziwych shenaniganów. Wreszcie, jeśli złośliwe wystąpienie binarne lub obiektowe przedostało się do środowiska wykonawczego, wyobrażenie sobie, że „system przywilejów lub system bezpieczeństwa” został zastąpiony w podobny sposób, nie stanowi większego problemu.

Ale biorąc pod uwagę punkt Erica, jeśli umieścisz swój kod użytkownika w określonym systemie podatnym na ataki, być może po prostu przegrałeś.

Aha, a ściślej mówiąc, zgadzam się z postulatem w pytaniu: „Użytkownik nie powinien decydować, czy jest administratorem, czy nie. System przywilejów lub zabezpieczeń powinien. ” User.IsAdminMetoda jest złym pomysłem, jeśli uruchamiasz kod w systemie, nie masz 100% kontroli nad kodem - powinieneś to zrobić Permissions.IsAdmin(User user).

Patrick M.
źródło
Co? Jak to ma znaczenie? Bez względu na to, gdzie umieścisz politykę bezpieczeństwa, zawsze możesz potencjalnie podzielić ją na klasy z czymś niepewnym. Nie ma znaczenia, czy jest to użytkownik, zleceniodawca, polityka, zestaw uprawnień lub cokolwiek innego. Jest to całkowicie niezwiązany problem.
Aaronaught
Jest to argument bezpieczeństwa na rzecz pojedynczej odpowiedzialności. Aby bezpośrednio odpowiedzieć na twoje pytanie, moduł bezpieczeństwa i uprawnień powinien być zapieczętowaną klasą, ale niektóre projekty mogą wymagać, aby klasa User była wirtualna i dziedziczona. Jest to ostatni punkt w artykule na blogu (a więc być może najmniej prawdopodobny / ważny?), A może trochę komplikuję te kwestie, ale biorąc pod uwagę źródło, jasne jest, że polimorfizm może stanowić zagrożenie dla bezpieczeństwa, a zatem ogranicza odpowiedzialność dana klasa / obiekt jest bezpieczniejszy.
Patrick M,
Nie jestem pewien, dlaczego tak mówisz. Wiele ram zabezpieczeń i uprawnień jest wysoce rozszerzalnych. Większość podstawowych typów .NET związanych z bezpieczeństwem (IPrincipal, IIdentity, ClaimSet itp.) Nie tylko nie jest zapieczętowanych, ale w rzeczywistości jest interfejsami lub klasami abstrakcyjnymi, dzięki czemu można podłączać cokolwiek chcesz. Eric mówi o tym, że nie chciałbyś, aby coś takiego System.Stringbyło dziedziczne, ponieważ wszelkiego rodzaju krytyczny kod frameworku przyjmuje bardzo konkretne założenia dotyczące jego działania. W praktyce większość „kodu użytkownika” (w tym bezpieczeństwa) jest dziedziczna, aby umożliwić podwójne testowanie.
Aaronaught,
1
W każdym razie, polimorfizm nie jest nigdzie wspomniany w pytaniu, a jeśli podążasz za linkiem autora do miejsca, w którym został napisany oryginalny komentarz, jasne jest, że nie odnosiło się to do dziedziczenia, a nawet niezmienników klasowych w ogóle - chodziło o obowiązki.
Aaronaught,
Wiem, że nie wspomniano o tym, ale zasada odpowiedzialności klasowej jest bezpośrednio związana z polimorfizmem. Jeśli nie ma IsAdminmetody na zastąpienie / dokooptowanie, nie ma potencjalnej luki w zabezpieczeniach. Przeglądając metody tych interfejsów systemowych, IPrincipali IIdentityjasne jest, że projektanci się ze mną nie zgadzają, więc przyznaję.
Patrick M,
0

Problem tutaj:

if (user.isAdmin()) {
   doPriviledgedOp();
} else {
   // maybe we can anyway!
   doPriviledgedOp();
}

Albo poprawnie skonfigurowałeś zabezpieczenia, w którym to przypadku uprzywilejowana metoda powinna sprawdzić, czy osoba dzwoniąca ma wystarczające uprawnienia - w takim przypadku sprawdzenie jest zbędne. Albo nie masz zaufania do klasy nieuprzywilejowanej i ufasz jej, że podejmie własne decyzje dotyczące bezpieczeństwa.

James Anderson
źródło
4
Co to jest klasa bez uprawnień?
Brandon
1
Naprawdę nic nie można zrobić, aby zapobiec pisaniu tego rodzaju kodu. Bez względu na to, jak zaprojektujesz swoją aplikację, gdzieś tam będzie wyraźna kontrola, a ktoś może napisać ją, aby była niepewna. Nawet jeśli Twój system uprawnień jest tak usprawniony, jak dekorowanie metody atrybutem roli lub uprawnienia - ktoś może niepoprawnie rzucić na nią uprawnienie „publiczne” lub „wszyscy”. Więc tak naprawdę nie rozumiem, jak to sprawia, że ​​metoda taka stanowi User.IsAdmin konkretny problem.
Aaronaught