Qu'entend-on par «Un utilisateur ne devrait pas décider s'il s'agit d'un administrateur ou non. Le système de privilèges ou de sécurité devrait. "

55

L'exemple utilisé dans la question Transmettre les données minimales nues à une fonction permet de déterminer le meilleur moyen de déterminer si l'utilisateur est un administrateur ou non. Une réponse commune était:

user.isAdmin()

Cela a provoqué un commentaire qui a été répété plusieurs fois et voté plusieurs fois:

Un utilisateur ne devrait pas décider s'il s'agit d'un administrateur ou non. Le système de privilèges ou de sécurité devrait. Quelque chose est étroitement lié à une classe ne signifie pas que c'est une bonne idée de faire partie de cette classe.

J'ai répondu,

L'utilisateur ne décide rien. L'objet / la table utilisateur stocke des données sur chaque utilisateur. Les utilisateurs réels ne peuvent pas tout changer d'eux-mêmes.

Mais ce n'était pas productif. Clairement, il existe une différence de perspective sous-jacente qui rend la communication difficile. Quelqu'un peut-il m'expliquer pourquoi user.isAdmin () est mauvais et dessiner un bref aperçu de ce à quoi cela ressemble "bien"?

En réalité, je ne vois pas l'avantage de séparer la sécurité du système qu'il protège. Tout texte sur la sécurité indiquera que la sécurité doit être intégrée dans un système dès le début et prise en compte à chaque étape du développement, du déploiement, de la maintenance et même de la fin de vie. Ce n'est pas quelque chose qui peut être verrouillé sur le côté. Mais 17 votes jusqu'à présent sur ce commentaire indiquent qu'il me manque quelque chose d'important.

GlenPeterson
la source
8
Je ne pense pas que ce soit mauvais, surtout s'il ne s'agit que d'un indicateur dans la table des bases de données des utilisateurs. S'il devait y avoir des changements à l'avenir, changez-les à l'avenir. Vous pouvez également appeler le système de sécurité à partir de l'objet utilisateur. J'ai déjà lu sur le modèle de conception que chaque accès à une base de données / ressource doit passer par l'objet utilisateur actuel, afin de m'assurer que vous n'essayez jamais de faire quelque chose qui n'est pas autorisé. Peut-être un peu cher, mais l'utilisateur pourrait fournir un objet "privilèges" qui couvre tout ce qui concerne la sécurité.
Céphalopode
Je pense qu'il y avait un malentendu. Pour vous, "utilisateur" signifie la personne qui utilise le logiciel. Pour eux, "Utilisateur" signifie le code qui utilise vos fonctionnalités.
Philipp
2
Ce n’est pas que vous ayez demandé, mais ce que je ferais le plus attention dans ce cas est une méthode IsAdmin (), où que vous soyez. Je recommanderais généralement de ne pas coder en dur un "Admin", mais juste un ensemble d'autorisations possibles et qu'un "Admin" se trouve avoir tout l'ensemble. Ainsi, lorsque vous aurez besoin de séparer le rôle d’administrateur ultérieurement, les choses seront beaucoup plus simples. Et cela réduit la logique des cas spéciaux.
psr
1
@Philipp Je ne pense pas ... Les deux sites parlent explicitement d'un Userobjet avec une isAdmin()méthode (quoi qu'il fasse en coulisse)
Izkata
Trop tard pour éditer; Je voulais dire "Les deux côtés", pas "Les deux sites" ... = P
Izkata

Réponses:

12

Pensez à l'utilisateur en tant que clé et aux autorisations en tant que valeur.

Des contextes différents peuvent avoir des autorisations différentes pour chaque utilisateur. Les autorisations étant liées au contexte, vous devez changer d'utilisateur pour chaque contexte. Il est donc préférable de placer cette logique ailleurs (par exemple, la classe de contexte) et de rechercher les autorisations si nécessaire.

Un effet secondaire est que cela pourrait rendre votre système plus sécurisé, car vous ne pouvez pas oublier d'effacer l'indicateur admin pour les endroits où il n'est pas pertinent.

Wilbert
la source
40

Ce commentaire était mal formulé.

Il ne s'agit pas de savoir si l'objet utilisateur "décide" s'il s'agit d'un administrateur, d'un utilisateur test, d'un superutilisateur ou de tout autre élément entrant ou sortant de l'ordinaire. De toute évidence, rien de tout cela n’est décidé, c’est le système logiciel dans son ensemble, tel que vous le mettez en œuvre. Le point est de savoir si la classe "utilisateur" doit représenter les rôles du principal que représentent ses objets ou s'il s'agit d'une responsabilité d'une autre classe.

Kilian Foth
la source
6
Ouch (c'était mon commentaire), mais vous l'exprimez beaucoup mieux que moi.
Marjan Venema
Vous dites modéliser les rôles dans une classe de rôle séparée, pas sur la classe d'utilisateur? Vous ne recommandez pas un système séparé comme LDAP? Que se passe-t-il si chaque décision prise par la classe de rôle nécessite d'interroger l'enregistrement utilisateur approprié et ne nécessite aucune autre information? Devrait-il toujours être séparé de l'utilisateur et si oui, pourquoi?
GlenPeterson
2
@GlenPeterson: pas nécessairement, seulement les classes <> et généralement, il y a beaucoup plus de classes à discerner que de tables. Être orienté données conduit à définir des sous-listes sur des classes, bien que ces listes doivent souvent être définies sur un autre "grand livre" (ordres) ou sur une classe de ce type transmettant l'utilisateur (ou l'ordre) pour ... C'est plus une question de responsabilités que de verbes et de noms.
Marjan Venema
9
Pensez à l'utilisateur en tant que clé et aux autorisations en tant que valeur. Des contextes différents peuvent avoir des autorisations différentes pour chaque utilisateur. Les autorisations étant liées au contexte, vous devez soit changer l'utilisateur pour chaque contexte. Il est donc préférable de placer cette logique ailleurs (par exemple, la classe de contexte).
Wilbert
2
@ Wilbert: Contexte! C'est particulièrement éclairant! Oui, si vous disposez de plus d'un contexte pour ces autorisations, tout est logique pour moi. Habituellement, je vois ces autorisations dans un seul contexte, et les coller sur l'utilisateur est la solution la plus simple dans ce cas. Clairement, elles n’appartiennent pas à elles si elles s’appliquent différemment dans un deuxième contexte. Si vous écrivez cela comme une réponse, je suis susceptible de l'accepter. Je vous remercie.
GlenPeterson
30

Je n'aurais pas choisi de formuler le commentaire original de la manière dont il était libellé, mais cela identifie un problème potentiellement légitime.

Plus précisément, les préoccupations qui justifient la séparation sont l’ authentification contre l’ autorisation .

L'authentification fait référence au processus de connexion et d'obtention d'une identité. C’est ainsi que les systèmes savent qui vous êtes et sont utilisés pour des tâches telles que la personnalisation, la propriété d’objets, etc.

L'autorisation fait référence à ce que vous êtes autorisé à faire , et ceci (généralement) n'est pas déterminé par votre identité . Au lieu de cela, il est déterminé par certaines règles de sécurité, telles que les rôles ou les autorisations, qui ne se soucient pas de choses telles que votre nom ou votre adresse électronique.

Ces deux peuvent changer orthogonalement l'un à l'autre. Par exemple, vous pouvez modifier le modèle d'authentification en ajoutant des fournisseurs OpenID / OpenAuth. Vous pouvez également modifier la stratégie de sécurité en ajoutant un nouveau rôle ou en passant de RBAC à ABAC.

Si tout cela est classé dans une classe ou une abstraction, votre code de sécurité, qui est l’un de vos principaux outils d’ atténuation des risques , devient, paradoxalement, à haut risque.

J'ai travaillé avec des systèmes où authentification et autorisation étaient trop étroitement couplées. Dans un système, il y avait deux bases de données d'utilisateurs parallèles, chacune pour un type de "rôle". La personne ou l'équipe qui l'a conçu ne s'est apparemment jamais rendu compte qu'un seul utilisateur physique pouvait occuper les deux rôles, qu'il pouvait y avoir certaines actions communes à plusieurs rôles ou qu'il pouvait y avoir des problèmes de collisions d'ID utilisateur. C’est certes un exemple extrême, mais c’était / était incroyablement pénible de travailler avec.

Microsoft et Sun / Oracle (Java) font référence à l' ensemble des informations d'authentification et d'autorisation en tant que principal de sécurité . Ce n'est pas parfait, mais cela fonctionne raisonnablement bien. Dans .NET, par exemple, vous avez IPrincipal, qui encapsule le IIdentity- le premier étant une politique objet (autorisation) , alors que celle - ci est une identité (authentification). Vous pouvez raisonnablement remettre en question la décision de placer l'un à l' intérieur de l'autre, mais l'important est que la plupart du code que vous écrivez ne soit utilisé que pour l'une des abstractions, ce qui signifie qu'il est facile de tester et de refactoriser.

Il n'y a rien de mal avec un User.IsAdminchamp ... à moins qu'il y ait aussi un User.Namechamp. Cela indiquerait que le concept "d'utilisateur" n'est pas correctement défini et qu'il s'agit malheureusement d'une erreur très courante parmi les développeurs qui sont un peu mouillés derrière les oreilles en matière de sécurité. Généralement, l' identité et la stratégie ne doivent être partagées que par l'ID utilisateur. Ce n'est pas une coïncidence si c'est exactement comment il est implémenté dans les modèles de sécurité Windows et * nix.

Il est tout à fait acceptable de créer des objets wrapper qui encapsulent à la fois l'identité et la stratégie. Par exemple, cela faciliterait la création d'un écran de tableau de bord dans lequel vous devez afficher un message "hello" en plus de divers widgets ou liens auxquels l'utilisateur actuel est autorisé à accéder. Tant que cette enveloppe juste enveloppe les informations d'identité et de la politique, et ne prétend pas posséder. En d'autres termes, tant que cela n'est pas présenté comme une racine globale .

Un modèle de sécurité simpliste semble toujours être une bonne idée lorsque vous concevez une nouvelle application, à cause de YAGNI et ainsi de suite, mais il finit presque toujours par vous revenir plus tard, car, surprise, de nouvelles fonctionnalités sont ajoutées!

Ainsi, si vous savez ce qui vous convient le mieux, vous allez séparer les informations d'authentification et d'autorisation. Même si "l'autorisation" est aussi simple qu'un indicateur "IsAdmin", vous serez toujours mieux s'il ne fait pas partie de la même classe ou du même tableau que les informations d'authentification. Ainsi, si et quand votre politique de sécurité doit change, vous n’avez pas besoin de faire de chirurgie reconstructive sur vos systèmes d’authentification qui fonctionne déjà bien.

Aaronaught
la source
11
Oh j'aurais aimé avoir le temps d'écrire ceci. Encore une fois, je n'aurais probablement pas pu l'exprimer aussi bien que vous l'avez fait sans plus de réflexion de ma part. Bien souvent, mon instinct me dit d’aller d’une manière ou d’une autre, mais expliquer pourquoi - à moi-même ou à quelqu'un d’autre - nécessite beaucoup plus de réflexion et d’efforts. Merci pour ce post. J'aurais pu en prendre plus, mais SE ne me laissera pas ...
Marjan Venema
Cela ressemble étrangement à un projet sur lequel je travaille et votre réponse m'a donné une autre perspective. Merci beaucoup!
Brandon
"Il n'y a rien de mal avec un champ User.IsAdmin ... sauf s'il existe également un champ User.Name" que vous écrivez. Mais quelle isAdminest une méthode? Il peut simplement déléguer à un objet dont la responsabilité est de garder une trace des autorisations. Quelle est la meilleure pratique dans la conception d'un modèle relationnel (quels sont les champs d'une entité) ne peut pas nécessairement être traduite en une meilleure pratique dans un modèle OO (quels messages un objet peut-il répondre).
KaptajnKold
@KaptajnKold: Je continue à lire ce sentiment dans divers sujets de questions et réponses de ce forum. Si une méthode existe, peu importe si elle effectue directement les opérations ou si elle les délègue , cela reste une responsabilité, car d'autres classes peuvent s'en prévaloir . Vous User.IsAdminpourriez très bien être un one-liner qui ne fait qu’appel PermissionManager.IsAdmin(this.Id), mais s’il est référencé par 30 autres classes, vous ne pourrez ni le modifier ni le supprimer. C'est pourquoi le SRP existe.
Aaronaught
Et, @KaptajnKold, en ce qui concerne les modèles relationnels, je ne sais pas trop où vous voulez en venir; c'est encore pire d'avoir un IsAdmin champ . C'est le genre de domaine qui a tendance à persister longtemps après que le système a évolué en RBAC ou en quelque chose de plus complexe, et se désynchronise progressivement avec les "vraies" autorisations, et les gens oublient qu'ils ne sont plus censés l'utiliser, et ... eh bien, dites simplement non. Même si vous pensez que vous n'avez que deux rôles, "admin" et "non-admin", ne le stockez pas en tant que champ booléen / bit, normalisez-le correctement et disposez d'un tableau des permissions.
Aaronaught
28

Eh bien, à tout le moins, cela viole le principe de responsabilité unique . Devrait-il y avoir des changements (plusieurs niveaux d’administrateur, encore plus de rôles différents?), Ce type de conception serait assez compliqué et difficile à maintenir.

Considérez l'avantage de séparer le système de sécurité de la classe d'utilisateurs, afin que les deux puissent avoir un rôle unique et bien défini. Vous vous retrouvez avec plus propre, plus facile à maintenir la conception globale.

Zavior
la source
2
@GlenPeterson: Nope, les utilisateurs peuvent très bien exister sans sécurité. Qu'en est-il de mon nom, de mon nom d'affichage, de mon adresse, de mon adresse de courrier électronique, etc. Où les mettriez-vous? L'identification (authentification) et l'autorisation sont deux bêtes différentes.
Marjan Venema
2
class Userest un composant essentiel du triplet de sécurité Identification, Authentification, Autorisation . Celles-ci sont intimement liées au niveau de la conception, il n'est donc pas déraisonnable que le code reflète cette interdépendance.
MSalters
1
La question est de savoir si la classe User doit contenir toute la logique permettant de manipuler le triplet!
Zavior
3
@MSalters: s'il s'agit d'une API pour une telle logique, elle le sait même si elle délègue ailleurs la logique exacte. L’argument avancé est que l’autorisation (autorisations) concerne la combinaison d’un utilisateur avec autre chose et ne devrait donc pas faire partie de l’utilisateur ou de quelque chose d’autre, mais faire partie des systèmes d’autorisations qui tiennent compte à la fois des utilisateurs et de tout le reste. est donné l'autorisation pour.
Marjan Venema
2
Should there be changes- eh bien, nous ne savons pas qu'il y aura des changements. YAGNI et tout. Est-ce que ces changements quand cela devient nécessaire, non?
Izkata
10

Je pense que le problème, peut-être mal formulé, est que cela donne trop de poids à la Userclasse. Non, il n'y a pas de problème de sécurité avec une méthode User.isAdmin(), comme suggéré dans ces commentaires. Après tout, si votre code est suffisamment profond pour injecter du code malveillant pour l’une de vos classes principales, vous rencontrez de graves problèmes. En revanche, il n’a aucun sens Userde décider s’il s’agit d’un administrateur pour chaque contexte. Imaginez que vous ajoutiez différents rôles: modérateurs pour votre forum, rédacteurs pouvant publier des publications sur la page d'accueil, rédacteurs pouvant rédiger du contenu que les rédacteurs publient, etc. Avec l'approche consistant à le mettre sur l' Userobjet, vous vous retrouverez avec trop de méthodes et trop de logique vivant sur des objets Userqui ne sont pas directement liés à la classe.

Au lieu de cela, vous pouvez utiliser une énumération de différents types d'autorisations et une PermissionsManagerclasse. Peut-être que vous pourriez avoir une méthode comme PermissionsManager.userHasPermission(User, Permission)renvoyer une valeur booléenne. En pratique, cela pourrait ressembler à (en java):

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

C'est essentiellement équivalent à la méthode Rails before_filter, qui fournit un exemple de ce type de vérification des autorisations dans le monde réel.

asthasr
la source
6
En quoi est-ce différent de user.hasPermission(Permissions.EDITOR), si ce n’est plus verbeux et procédural que OO?
Céphalopode
9
@Arian: car attribue user.hasPermissionstrop de responsabilités à la classe d'utilisateurs. Cela nécessite que l'utilisateur connaisse les autorisations, alors qu'un utilisateur (classe) doit pouvoir exister sans cette connaissance. Vous ne souhaitez pas qu'une classe d'utilisateurs sache comment imprimer ou afficher (créer un formulaire) elle-même, vous la laissez à un générateur de rendu d'imprimante ou à une classe de générateur d'affichage.
Marjan Venema
4
user.hasPermission(P)est la bonne API, mais ce n'est que l'interface. La vraie décision doit bien sûr être prise dans un sous-système de sécurité. Pour répondre au commentaire de syrion sur les tests, vous pouvez échanger ce sous-système pendant les tests. Cependant, l’avantage de ce user.hasPermission(P)qui est simple est que vous ne polluez pas tout le code simplement pour pouvoir tester class User.
MSalters
3
@MarjanVenema Selon cette logique, l'utilisateur resterait un objet de données sans aucune responsabilité. Je ne dis pas que toute la gestion des autorisations doit être mise en œuvre dans la classe d'utilisateurs, si un système aussi complexe est nécessaire. Vous dites qu'un utilisateur n'a pas besoin de connaître les autorisations, mais je ne vois pas pourquoi toutes les autres classes doivent savoir comment résoudre les autorisations pour un utilisateur. Cela rend les moqueries et les tests beaucoup plus difficiles.
Céphalopode
6
@Arian: alors que les classes anémiques sont une odeur, certaines classes n'ont tout simplement aucun comportement ... User, à mon humble avis, est une classe de ce type qu'il vaut mieux utiliser comme paramètre pour toute une série d'autres choses (qui veulent faire des choses / prendre des décisions en fonction de la personne qui les demande), puis d’être utilisé comme un conteneur pour tous les types de comportement simplement parce qu’il semble commode de le coder ainsi.
Marjan Venema
9

Object.isX () est utilisé pour représenter une relation claire entre l'objet et X, qui peut être exprimée sous la forme d'un résultat booléen, par exemple:

L'eau bouillonne ()

Circuit.isOpen ()

User.isAdmin () assume un sens unique de "Administrateur" dans chaque contexte du système , qu'un utilisateur est, dans chaque partie du système, un administrateur.

Bien que cela semble assez simple, un programme du monde réel ne correspondra presque jamais à ce modèle, mais il sera certainement nécessaire pour un utilisateur d’administrer une ressource [X] mais pas de [Y], ou pour des types d’administrateurs plus distincts (un [projet]). ] administrateur contre un administrateur [système]).

Cette situation nécessite généralement une enquête sur les exigences. Rarement, voire jamais, un client voudra-t-il la relation que représente réellement User.isAdmin (), aussi j'hésiterais-il à mettre en œuvre une telle solution sans clarification.

FMJaguar
la source
6

L'affiche avait simplement une conception différente et plus compliquée à l'esprit. À mon avis, ça User.isAdmin()va . Même si vous introduisez plus tard un modèle d'autorisations compliqué, je ne vois aucune raison de changer User.isAdmin(). Plus tard, vous voudrez peut-être scinder la classe User en un objet représentant un utilisateur connecté et un objet statique représentant les données relatives à l'utilisateur. Ou vous ne pouvez pas. Économiser demain pour demain.

Kevin Cline
la source
4
Save tomorrow for tomorrow. Ouais. Lorsque vous vous rendez compte que le code étroitement couplé que vous venez d'écrire vous a catalogué, vous devez le réécrire car vous n'avez pas pris les 20 minutes supplémentaires pour le découpler ...
MirroredFate
5
@MirroredFate: Il est tout à fait possible de User.isAdminconnecter une méthode à un mécanisme de permission arbitraire sans coupler la classe User à ce mécanisme spécifique.
Kevin Cline
3
Pour que User.isAdmin soit couplé à un mécanisme d'autorisation arbitraire, même sans couplage à un mécanisme spécifique, nécessite ... bien ... couplage. Le minimum de ce qui serait à une interface. Et cette interface ne devrait tout simplement pas être là. Il donne à la classe d'utilisateurs (et à l'interface) quelque chose dont il ne devrait tout simplement pas être responsable. Aaronaught l' explique beaucoup mieux que moi (tout comme la combinaison de toutes les autres réponses à cette question).
Marjan Venema
8
@MirroredFate: Peu m'importe si je dois réécrire une implémentation simple pour répondre à des exigences plus complexes. Mais j'ai eu de très mauvaises expériences avec des API trop complexes conçues pour répondre à des exigences futures imaginaires qui ne se sont jamais posées.
Kevin Cline
3
@kevincline Alors que les API trop complexes sont (par définition) une mauvaise chose, je ne suggérais pas qu'il faille écrire des API trop complexes. Je disais que Save tomorrow for tomorrowc’est une approche de conception épouvantable parce que cela rend les problèmes qui seraient triviaux si le système était correctement implémenté bien pire. En fait, c'est cette mentalité qui conduit à des API trop complexes, au fur et à mesure de l'ajout d'une couche à l'autre pour corriger la mauvaise conception initiale. Je suppose que ma position est measure twice, cut once.
MirroredFate
5

Pas vraiment un problème ...

Je ne vois pas de problème avec User.isAdmin(). Je le préfère certainement à quelque chose du genre PermissionManager.userHasPermission(user, permissions.ADMIN), qui, dans le saint nom de SRP, rend le code moins clair et n’ajoute rien de valeur.

Je pense que SRP est interprété un peu littéralement par certains. Je pense que c'est bien, même préférable pour une classe d'avoir une interface riche. SRP signifie simplement qu'un objet doit déléguer à des collaborateurs tout ce qui ne relève pas de sa seule responsabilité. Si le rôle d'administrateur d'un utilisateur est plus complexe qu'un champ booléen, il serait peut-être judicieux que l'objet utilisateur délègue une délégation à un Autorisateur. Mais cela ne veut pas dire que ce n’est pas aussi une bonne idée pour un utilisateur de conserver sa isAdminméthode. En fait, cela signifie que lorsque votre application passe du plus simple au plus complexe, vous devez changer le code qui utilise l'objet Utilisateur. IOW, votre client n'a pas besoin de savoir ce qu'il faut vraiment pour répondre à la question de savoir si un utilisateur est un administrateur ou non.

... mais pourquoi voulez-vous vraiment savoir?

Cela dit, il me semble que vous avez rarement besoin de savoir si un utilisateur est un administrateur, sauf pour pouvoir répondre à une autre question, par exemple si un utilisateur est autorisé à effectuer une action spécifique, par exemple la mise à jour d'un widget. Si tel est le cas, je préférerais avoir une méthode sur Widget, comme isUpdatableBy(User user):

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

De cette façon, un widget est responsable de savoir quels critères doivent être satisfaits pour qu'il soit mis à jour par un utilisateur, et un utilisateur est tenu de savoir s'il s'agit d'un administrateur. Cette conception communique clairement sur ses intentions et facilite le passage à une logique métier plus complexe, le cas échéant.

[Modifier]

Le problème de ne pas avoir User.isAdmin()et utiliserPermissionManager.userHasPermission(...)

J'ai pensé ajouter quelque chose à ma réponse pour expliquer pourquoi je préfère fortement appeler une méthode sur l'objet User que d'appeler une méthode sur un objet PermissionManager si je veux savoir si un utilisateur est un administrateur (ou a un rôle d'administrateur).

Je pense qu'il est juste de supposer que vous allez toujours dépendre de la classe User chaque fois que vous devez vous poser la question suivante : cet utilisateur est-il un administrateur? C'est une dépendance dont vous ne pouvez vous débarrasser. Mais si vous devez passer l'utilisateur à un autre objet pour lui poser une question, cela crée une nouvelle dépendance à cet objet en plus de celle que vous avez déjà sur l'utilisateur. Si la question est souvent posée, il y a beaucoup d'endroits où vous créez une dépendance supplémentaire et vous devrez peut-être changer d'endroit si l'une des dépendances l'exige.

Comparez cela avec le déplacement de la dépendance dans la classe User. Maintenant, vous avez soudainement un système où le code client (le code qui doit poser la question à cet utilisateur est un administrateur ) n'est pas associé à la mise en œuvre de la réponse donnée à cette question. Vous êtes libre de changer complètement le système de permission et il vous suffit de mettre à jour une méthode dans une classe pour le faire. Tout le code client reste le même.

Insister sur le fait de ne pas avoir de isAdminméthode sur l'utilisateur, de peur de créer une dépendance de la classe User vis-à-vis du sous-système des autorisations, équivaut, à mon avis, à dépenser un dollar pour gagner un centime. Bien sûr, vous évitez une dépendance dans la classe User, mais au prix de la créer à chaque endroit où vous devez poser la question. Pas une bonne affaire.

KaptajnKold
la source
2
"SRP signifie simplement qu'un objet doit déléguer à des collaborateurs tout ce qui ne relève pas de sa seule responsabilité" - faux . La définition du SRP englobe tout ce qu'un objet fait directement et tout ce qu'il délègue. Une classe qui n'effectue qu'une seule tâche directement, mais effectue 50 autres tâches indirectement via une délégation enfreint (probablement) le SRP.
Aaronaught
KaptajnKold: J'ai +1 parce que je suis d'accord avec vous et je me sens un peu coupable à ce sujet. Votre message ajoute à la discussion, mais il ne répond pas à la question.
GlenPeterson
@GlenPeterson Eh bien, j'ai essayé de répondre à la partie de la question qui était "ce qui semble être fait" correctement "En outre: vous ne devriez absolument pas vous sentir coupable d'être d'accord avec moi :)
KaptajnKold Le
1
@ JamesSnell Peut-être. Si. Mais c’est un détail d’implémentation que je limiterais encore dans la méthode isAdmin sur User. De cette façon, votre code client ne doit pas nécessairement changer lorsque "l'administrateur" d'un utilisateur passe d'un champ booléen à un domaine plus avancé. Vous avez peut-être besoin de savoir si un utilisateur est un administrateur et vous ne voulez pas avoir à les changer chaque fois que votre système d'autorisation change.
KaptajnKold
1
@ JamesSnell Peut-être que nous nous comprenons mal? La question de savoir si un utilisateur est ou non un administrateur n'est pas la même chose que de savoir si un utilisateur est autorisé ou non à effectuer une action spécifique . La réponse à la première question est toujours indépendante du contexte. La réponse à la seconde dépend beaucoup du contexte. C’est ce que j’ai essayé de traiter dans la seconde partie de ma réponse initiale.
KaptajnKold
1

Cette discussion m'a rappelé un billet de blog d'Eric Lippert, qui m'a été signalé lors d'une discussion similaire sur la conception, la sécurité et la correction des classes.

Pourquoi autant de classes-cadres sont-elles scellées?

Eric soulève le point en particulier:

4) sécurisé. le polymorphisme a pour objectif principal de pouvoir faire passer des objets qui ressemblent à des animaux mais qui sont en fait des girafes. Il y a des problèmes de sécurité potentiels ici.

Chaque fois que vous implémentez une méthode qui prend une instance d'un type non scellé, vous DEVEZ écrire cette méthode pour qu'elle soit robuste face à des instances potentiellement hostiles de ce type. Vous ne pouvez pas compter sur des invariants que vous savez être vrais dans VOTRE implémentation, car une page Web hostile peut sous-classer votre implémentation, remplacer les méthodes virtuelles pour effectuer des tâches qui gâchent votre logique et les transmet. Chaque fois que je scelle une classe , Je peux écrire des méthodes qui utilisent cette classe avec la certitude de savoir ce que cette classe fait.

Cela peut sembler une tangente, mais je pense que cela rejoint le point que d'autres affiches ont soulevé à propos du principe de responsabilité unique SOLID . Considérez la classe malveillante suivante comme une raison de ne pas implémenter une méthode ou une propriété.User.IsAdmin

public class MyUser : User
{

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

    // Anything else we can break today?

}

Certes, c'est un peu exagéré: personne n'a encore suggéré de créer IsAmdminune méthode virtuelle, mais cela pourrait arriver si votre architecture utilisateur / rôle devenait étrangement complexe. (Ou peut-être pas. Pensez public class Admin : User { ... }: une telle classe pourrait avoir exactement le code ci-dessus.) La mise en place d’un binaire malveillant n’est pas un vecteur d’attaque courant et a beaucoup plus de potentiel de chaos qu’une obscure bibliothèque utilisateur. pourrait être le bug d' escalade de privilège qui ouvre la porte aux véritables manigances. Enfin, si une instance binaire ou d'objet malveillante se retrouvait dans le temps d'exécution, imaginer le "système de privilège ou de sécurité" remplacé de la même manière ne serait pas trop compliqué.

Mais pour reprendre le point de vue d’Eric, si vous mettez votre code utilisateur dans un type particulier de système vulnérable, vous perdez peut-être tout simplement la partie.

Oh, et pour être précis, je suis d'accord avec le postulat de la question: «Un utilisateur ne devrait pas décider s'il s'agit d'un administrateur ou non. Les privilèges ou le système de sécurité devraient. »Une User.IsAdminméthode est une mauvaise idée si vous exécutez le code sur le système, vous ne gardez pas le contrôle à 100% du code - vous devriez plutôt le faire Permissions.IsAdmin(User user).

Patrick M
la source
Hein? Comment est-ce pertinent? Où que vous placiez la stratégie de sécurité, vous pouvez toujours potentiellement la sous-classer avec un élément non sécurisé. Peu importe qu'il s'agisse d'un utilisateur, d'un responsable, d'une stratégie, d'un PermissionSet ou autre. C'est une question totalement indépendante.
Aaronaught
C'est un argument de sécurité en faveur de la responsabilité unique. Pour répondre directement à votre question, le module de sécurité et autorisations est vraiment une classe scellée, mais certaines conceptions peuvent souhaiter que la classe User soit virtuelle et héritée. C'est le dernier point de l'article du blog (peut-être le moins probable / le plus important?), Et peut-être que je confondre un peu les problèmes, mais étant donné la source, il est clair que le polymorphisme peut être une menace pour la sécurité et donc limiter les responsabilités de une classe / objet donné est plus sécurisé.
Patrick M
Je ne sais pas pourquoi tu dis ça. Beaucoup de cadres de sécurité et d'autorisations sont hautement extensibles. La plupart des types .NET de base liés à la sécurité (IPrincipal, IIdentity, ClaimSet, etc.) ne sont pas seulement non scellés, mais sont en fait des interfaces ou des classes abstraites afin que vous puissiez brancher ce que vous voulez. Ce dont Eric parle, c'est que vous ne voudriez pas que quelque chose System.Stringpuisse être hérité, car toutes sortes de codes de framework critiques émettent des hypothèses très spécifiques sur son fonctionnement. En pratique, la plupart des "codes utilisateur" (y compris la sécurité) peuvent être hérités pour permettre les doublons de test.
Aaronaught
1
Quoi qu'il en soit, le polymorphisme n'est mentionné nulle part dans la question, et si vous suivez le lien de l'auteur à l'endroit où le commentaire original a été fait, il est clair qu'il ne fait pas référence à l'héritage ou même aux invariants de classe en général - il s'agissait de responsabilités.
Aaronaught
Je sais que cela n’a pas été mentionné, mais le principe des responsabilités de classe est directement lié au polymorphisme. S'il n'y a pas de IsAdminméthode de substitution / cooptation, il n'y a pas de faille de sécurité potentielle. Passant en revue les méthodes de ces interfaces système, IPrincipalet IIdentity, il est clair que les concepteurs sont en désaccord avec moi, et je concèdent le point.
Patrick M
0

Le problème ici est:

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

Soit vous avez correctement configuré votre sécurité, auquel cas la méthode privilégiée doit vérifier si l’appelant dispose des droits suffisants - auquel cas la vérification est superflue. Ou bien vous n’avez pas fait confiance à une classe non privilégiée pour prendre ses propres décisions en matière de sécurité.

James Anderson
la source
4
Qu'est-ce qu'une classe non défavorisée?
Brandon
1
Vous ne pouvez vraiment rien faire pour empêcher l’écriture de ce type de code. Quelle que soit la manière dont vous concevez votre application, il y aura quelque part un contrôle explicite comme celui-ci, et quelqu'un pourrait l'écrire pour indiquer qu'il n'est pas sécurisé. Même si votre système de permissions est aussi simple que de décorer une méthode avec un attribut role ou permission, quelqu'un pourrait à tort donner une permission "publique" ou "tout le monde" dessus. Je ne vois donc pas vraiment en quoi cela rend une méthode comme un problème User.IsAdmin spécifique .
Aaronaught