Cosa si intende per "Un utente non dovrebbe decidere se è un amministratore o meno. Il sistema di privilegi o di sicurezza dovrebbe. "

Pensa all'Utente come chiave e ai permessi come valore.

Diversi contesti potrebbero avere permessi diversi per ogni utente. Poiché le autorizzazioni sono rilevanti per il contesto, è necessario modificare l'utente per ciascun contesto. Quindi è meglio mettere questa logica da qualche altra parte (ad esempio la classe di contesto), e basta cercare le autorizzazioni dove necessario.

Un effetto collaterale è che potrebbe rendere il tuo sistema più sicuro, perché non puoi dimenticare di cancellare la flag admin per i punti in cui non è rilevante.

Quel commento era mal scritto.

Il punto non è se l'oggetto utente "decide" se è un amministratore, un utente di prova, un superutente o qualsiasi dentro o fuori dall'ordinario. Ovviamente non decide nessuna di queste cose, il sistema software in generale, come implementato da te, lo fa. Il punto è se l '"utente" classe dovrebbe rappresentare i ruoli del principale che i suoi oggetti rappresentano, o se questa è una responsabilità di un'altra classe.

Non avrei scelto di esprimere il commento originale nel modo in cui è stato formulato, ma identifica un problema legittimo potenzialmente .

In particolare, le preoccupazioni che giustificano la separazione sono autenticazione rispetto a autorizzazione .

Autenticazione fa riferimento al processo di accesso e acquisizione di un'identità. È come i sistemi sanno chi sei e sono usati per cose come personalizzazione, proprietà di oggetti, ecc.

Autorizzazione si riferisce a che cosa ti è permesso fare , e questo (generalmente) è non determinato da chi sei . Al contrario, è determinato da una politica di sicurezza come ruoli o permessi, a cui non importa nulla come il tuo nome o il tuo indirizzo email.

Questi due possono cambiare ortogonalmente l'un l'altro. Ad esempio, è possibile modificare il modello di autenticazione aggiungendo i provider OpenID / OpenAuth. E potresti cambiare la politica di sicurezza aggiungendo un nuovo ruolo o cambiando da RBAC a ABAC.

Se tutto questo va in una classe o in un'astrazione, allora il tuo codice di sicurezza, che è uno dei tuoi strumenti più importanti per mitigazione del rischio , diventa, ironicamente, ad alto rischio.

Ho lavorato con sistemi in cui l'autenticazione e l'autorizzazione erano troppo strettamente accoppiate. In un sistema, c'erano due database utente paralleli, ciascuno per un tipo di "ruolo". La persona o il team che l'ha progettata apparentemente non ha mai considerato che un singolo utente fisico potrebbe essere in entrambi i ruoli o che potrebbero esserci azioni comuni a più ruoli o che potrebbero esserci problemi con le collisioni ID utente. Questo è sicuramente un esempio estremo, ma era / è incredibilmente doloroso con cui lavorare.

Microsoft e Sun / Oracle (Java) si riferiscono al aggregato delle informazioni di autenticazione e autorizzazione come Security Principal . Non è perfetto, ma funziona abbastanza bene. In .NET, ad esempio, hai IPrincipal , che incapsula il IIdentity - il primo è un oggetto criterio (autorizzazione) mentre il secondo è un identità (autenticazione). Potresti ragionevolmente dubitare della decisione di mettere uno dentro dell'altro, ma la cosa importante è che la maggior parte del codice che scrivi sarà solo per una delle astrazioni che significa che è facile da testare e refactoring.

Non c'è niente di sbagliato in un campo User.IsAdmin ... a meno che non ci sia anche un campo User.Name . Ciò indicherebbe che il concetto di "utente" non è propriamente definito e questo è, purtroppo, un errore molto comune tra gli sviluppatori che sono un po 'bagnati dietro le orecchie quando si parla di sicurezza. In genere, l'unica cosa che deve essere condivisa da identity e policy è l'ID utente, che, non a caso, è esattamente come viene implementato sia in Windows che in * nix modelli di sicurezza.

È completamente accettabile creare oggetti wrapper che incapsulino sia l'identità che la politica. Ad esempio, faciliterebbe la creazione di una schermata del dashboard in cui è necessario visualizzare un messaggio "ciao" oltre a vari widget o collegamenti a cui l'utente corrente può accedere. Finché questo wrapper racchiude solo le informazioni sull'identità e sulla politica e non pretende di possederlo. In altre parole, purché non venga presentato come una radice aggregata .

Un modello di sicurezza semplicistico sembra come una buona idea quando progetti per la prima volta una nuova applicazione, a causa di YAGNI e tutto il resto, ma finisce quasi sempre per tornare a morderti dopo, perché, sorpresa a sorpresa, vengono aggiunte nuove funzionalità!

Quindi, se sai cosa è meglio per te, manterrai le informazioni di autenticazione e autorizzazione separate. Anche se la "autorizzazione" adesso è semplice come un flag "IsAdmin", starai ancora meglio se non fa parte della stessa classe o tabella delle informazioni di autenticazione, in modo che se e quando la tua politica di sicurezza debba cambia, non hai bisogno di fare interventi di chirurgia ricostruttiva sui tuoi sistemi di autenticazione che funziona già bene.

Beh, per lo meno viola il principio di responsabilità singola . Se dovessero esserci dei cambiamenti (più livelli di amministrazione, ancora più ruoli diversi?) Questo tipo di design sarebbe abbastanza disordinato e terribile da mantenere.

Considera il vantaggio di separare il sistema di sicurezza dalla classe utente, in modo che entrambi possano avere un singolo ruolo ben definito. Si finisce con un design più pulito, più facile da mantenere in generale.

Penso che il problema, forse formulato male, sia che mette troppo peso sulla classe User . No, non c'è alcun problema di sicurezza con il metodo User.isAdmin() , come suggerito in quei commenti. Dopo tutto, se qualcuno è abbastanza in profondità nel codice per iniettare codice dannoso per una delle classi principali, si hanno problemi seri. D'altra parte, non ha senso che User decida se è un amministratore per ogni contesto. Immagina di aggiungere ruoli diversi: moderatori per il tuo forum, editor che possono pubblicare post in prima pagina, autori che possono scrivere contenuti da pubblicare per gli editori e così via. Con l'approccio di metterlo sull'oggetto User , finirai con troppi metodi e troppa logica di vita su User che non è direttamente correlata alla classe.

Invece, potresti usare un enum di diversi tipi di permessi e una classe PermissionsManager . Forse potresti avere un metodo come PermissionsManager.userHasPermission(User, Permission) , restituendo un valore booleano. In pratica, potrebbe sembrare (in java):

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

È essenzialmente equivalente al metodo di metodo Rails before_filter , che fornisce un esempio di questo tipo di controllo delle autorizzazioni nel mondo reale.

Object.isX () è usato per rappresentare una chiara relazione tra l'oggetto e X, che può essere espressa come risultato booleano, ad esempio:

Water.isBoiling ()

Circuit.isOpen ()

User.isAdmin () assume un singolo significato di 'Amministratore' in ogni contesto del sistema , che un utente è, in ogni parte del sistema, un amministratore.

Anche se sembra abbastanza semplice, un programma del mondo reale non si adatta quasi mai a questo modello, ci sarà sicuramente un requisito per un utente che amministri la risorsa [X] ma non [Y], o per più tipi distinti di amministratori ( un [progetto] amministratore contro un [sistema] amministratore).

Questa situazione di solito richiede un'indagine sui requisiti. Raramente, se mai, un cliente vorrà la relazione che User.isAdmin () rappresenta in realtà, quindi esiterei a implementare qualsiasi soluzione senza alcun chiarimento.

Il poster aveva semplicemente un design diverso e più complicato in mente. Secondo me, User.isAdmin() va bene . Anche se in seguito introduci qualche complicato modello di autorizzazioni, vedo poco motivo per spostare User.isAdmin() . Successivamente, potresti dividere la classe User in un oggetto che rappresenta un utente che ha effettuato l'accesso e un oggetto statico che rappresenta i dati sull'utente. O forse no. Salva domani per domani.

Non proprio un problema ...

Non vedo un problema con User.isAdmin() . Certamente lo preferisco a qualcosa come PermissionManager.userHasPermission(user, permissions.ADMIN) , che nel sacro nome di SRP rende il codice meno chiaro e non aggiunge nulla di valore.

Penso che l'SRP venga interpretato un po 'alla lettera da alcuni. Penso che vada bene, anche preferibile che una classe abbia un'interfaccia ricca. SRP significa semplicemente che un oggetto deve delegare ai collaboratori tutto ciò che non rientra nella sua responsabilità unica. Se il ruolo di amministratore di un utente è qualcosa di più impegnativo di un campo booleano, potrebbe benissimo avere senso che l'oggetto utente deleghi ad un PermissionsManager. Ma ciò non significa che non sia anche una buona idea per un utente mantenere il suo metodo isAdmin . In realtà significa che quando la tua applicazione passa da semplice a complessa, devi cambiare il codice che utilizza l'oggetto Utente. IOW, il tuo cliente non ha bisogno di sapere cosa serve veramente per rispondere alla domanda se un utente sia o meno un amministratore.

... ma perché vuoi davvero saperlo?

Detto questo, mi sembra che raramente tu abbia bisogno di sapere se un utente è un amministratore eccetto per essere in grado di rispondere ad altre domande, come se un utente sia autorizzato o meno a compiere qualche azione specifica, ad esempio aggiornare un widget. Se questo è il caso, preferirei avere un metodo su Widget, come isUpdatableBy(User user) :

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

In questo modo un Widget è responsabile della conoscenza dei criteri che devono essere soddisfatti per essere aggiornati da un utente e un utente è responsabile di sapere se si tratta di un amministratore. Questo design comunica chiaramente le sue intenzioni e semplifica il passaggio a una logica di business più complessa se e quando arriverà quel momento.

[Modifica]

Il problema con non avendo User.isAdmin() e usando PermissionManager.userHasPermission(...)

Ho pensato di aggiungere alla mia risposta per spiegare perché preferisco di gran lunga chiamare un metodo sull'oggetto User per chiamare un metodo su un oggetto PermissionManager se voglio sapere se un utente è un amministratore (o ha un ruolo di amministratore) .

Penso sia giusto presumere che dipenderete sempre dalla classe User ovunque sia necessario porre la domanda questo utente è un amministratore? Questa è una dipendenza che non potete eliminare. Ma se devi passare l'utente a un oggetto diverso per fare una domanda al riguardo, crea una nuova dipendenza su quell'oggetto sopra quella che hai già sull'utente. Se la domanda viene posta molto, ci sono molti posti in cui crei una dipendenza extra, e ci sono molti posti che potresti dover cambiare se una delle dipendenze lo richiede.

Confronta questo con lo spostamento della dipendenza nella classe User. Ora, improvvisamente hai un sistema in cui il codice client (il codice che deve porre la domanda questo utente è un amministratore ) non è accoppiato all'implementazione di come questa domanda abbia una risposta. Sei libero di cambiare completamente il sistema di autorizzazione e devi solo aggiornare un metodo in una classe per farlo. Tutto il codice cliente rimane lo stesso.

Insistere nel non avere un metodo isAdmin sull'utente per timore di creare una dipendenza nella classe User sul sottosistema delle autorizzazioni, è a mio avviso simile alla spesa di un dollaro per guadagnare un centesimo. Certo, si evita una dipendenza nella classe User, ma al costo di crearne una in ogni posto in cui è necessario porre la domanda. Non è un buon affare.

Questa discussione mi ha ricordato un post sul blog di Eric Lippert, che è stato portato alla mia attenzione in una discussione simile sul design della classe, sulla sicurezza e sulla correttezza.

Perché molte delle classi quadro sono sigillate?

In particolare, Eric solleva il punto:

4) Secure. the whole point of polymorphism is that you can pass around objects that look like Animals but are in fact Giraffes. There are potential security issues here.

Every time you implement a method which takes an instance of an unsealed type, you MUST write that method to be robust in the face of potentially hostile instances of that type. You cannot rely upon any invariants which you know to be true of YOUR implementations, because some hostile web page might subclass your implementation, override the virtual methods to do stuff that messes up your logic, and passes it in. Every time I seal a class, I can write methods that use that class with the confidence that I know what that class does.

Questo può sembrare una tangente, ma penso che combaci con il punto che altri poster hanno sollevato riguardo al SOLID principio di responsabilità singola . Considera la seguente classe dannosa come una ragione per non implementare un metodo o una proprietà User.IsAdmin .

public class MyUser : User
{

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

    // Anything else we can break today?

}

Certo, è un po 'troppo lungo: nessuno ancora ha suggerito di rendere IsAmdmin un metodo virtuale, ma potrebbe accadere se l'architettura utente / ruolo finisse per essere stranamente complessa. (O forse no. Considera public class Admin : User { ... } : una classe di questo tipo potrebbe avere esattamente quel codice sopra.) Ottenere un binario malevolo in posizione non è un vettore di attacco comune e ha molto più potenziale per il caos rispetto a una libreria utente oscura - quindi di nuovo, questo potrebbe essere il bug Escalation dei privilegi che apre la porta ai veri imbrogli. Infine, se un'istanza di binario o oggetto malevolo ha trovato la sua strada nel tempo di esecuzione, non è molto più di un tratto immaginare il "Privilege o sistema di sicurezza" che viene sostituito in modo simile.

Ma prendendo a cuore il punto di vista di Eric, se inserisci il tuo codice utente in un particolare tipo di sistema vulnerabile, forse hai appena perso il gioco.

Oh, e per essere precisi per la cronaca, sono d'accordo con il postulato nella domanda: "Un utente non dovrebbe decidere se è un amministratore o meno. Il sistema di privilegi o sicurezza dovrebbe. "Un metodo User.IsAdmin è una cattiva idea se si sta eseguendo il codice sul sistema non si rimane nel controllo al 100% del codice - si dovrebbe invece fare Permissions.IsAdmin(User user) .

Il problema qui è:

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

O hai impostato correttamente la sicurezza, nel qual caso il metodo privilegiato dovrebbe verificare se il chiamante ha un'autorità sufficiente, nel qual caso il controllo è superfluo. Oppure non hai e ti fidi di una classe non privilegiata per prendere le proprie decisioni in materia di sicurezza.