11
\$\begingroup\$

I've always seen the IPrincipal and IIdentity interfaces separately, but I haven't seen any compelling reason for it, so I have my own interface that combines the two:

public interface IUser : System.Security.Principal.IIdentity, System.Security.Principal.IPrincipal {
    string Role { get; set; }
}

Then I implement the interface as:

public class User : IUser {
    public string Role { get; set; }

    public System.Security.Principal.IIdentity Identity {
        get { return this; }
    }

    public string AuthenticationType {
        get { throw new System.NotImplementedException(); }
    }

    public bool IsAuthenticated {
        get { throw new System.NotImplementedException(); }
    }

    public string Name {
        get { throw new System.NotImplementedException(); }
    }

    public bool IsInRole(string role) {
        throw new System.NotImplementedException();
    }
}

Does this smell? Do I have a security issue here?

\$\endgroup\$
6
  • \$\begingroup\$ I doubt this is code review. It looks more like a design question. \$\endgroup\$
    – Snowbear
    Mar 25 '11 at 11:35
  • \$\begingroup\$ @Snowbear: Why wouldn't it be? Design is often discussed here anyhow. \$\endgroup\$ Mar 25 '11 at 12:28
  • 3
    \$\begingroup\$ Well... it's code that I wanted reviewed... I figured Code Review was the perfect place. :] \$\endgroup\$ Mar 25 '11 at 12:46
  • 1
    \$\begingroup\$ I mean that the only thing worth discussing I see here is IUser: IPrincipal, IIdentity (we can also review NotImplementedException throwing - I prefer NotSupportedException - but I doubt this is what you want to discuss). And the question should I implement both interfaces in one place? should not be here. \$\endgroup\$
    – Snowbear
    Mar 25 '11 at 14:02
  • \$\begingroup\$ Specific code design is on-topic here. Abstract is not. \$\endgroup\$
    – Michael K
    Mar 25 '11 at 18:14
8
\$\begingroup\$

While there are circumstances where you need to decouple the Principal from the Identity, in many applications this is not the case, and the distinction will only complicate the code. The way security is best handled in an application depends very much on the individual needs of the particular app.

Martin Fowler's article Dealing with Roles provides an in depth analysis on to the different patterns that can be used to implement role-based security, and the indications of necessity for each pattern.

In general, you can't make an assumption that a solution that separates the principal from the identity is the optimal solution for all applications. I have encountered many scenarios where it was unnecessary and added nothing but additional classes. In that case, it is better to merge the two interfaces.

Furthermore, these are interfaces not classes. An interface denotes a "has a" relationship between the instance and the definition, where as a class denotes an "is a" relationship. An instance of a User in the OP's example is an object that has both an Identity and a Principal (Security Context). You can always break things down to further levels of complexity, but if the application functionality doesn't indicate a reasonable need to separate the class, then Why would you do it?

\$\endgroup\$
9
  • \$\begingroup\$ "interface denotes a "has a" relationship"? Here I was thinking interfaces define a 'can/is' relationship, and composition defines a 'has a' relationship. I do agree with your other points, hence my reply in a comment on my answer. \$\endgroup\$ Apr 3 '11 at 0:18
  • \$\begingroup\$ @Steven, a "can do" relationship, is applicable to an interface. I suppose that a class that implements and interface has a "has a" relationship to the interface's properties and a "can do" relationship to the interface's methods. Composition seems to me like part of the implementation as opposed to part of the definition. Check out the answer in this SO post stackoverflow.com/questions/56867/interface-vs-base-class \$\endgroup\$ Apr 3 '11 at 1:56
  • \$\begingroup\$ @smartcaveman: I'm talking about Class Diagram definitions here ... Stating that "interfaces denotes 'has a'" is very confusing and seems incorrect to me. Apparently interfaces should be interpreted as 'realizes'. Isn't it the other way around, composition is part of the definition as opposed to part of the implementation? \$\endgroup\$ Apr 3 '11 at 2:32
  • \$\begingroup\$ P.s.: Definition disagreements or not, the problem stated in my answer remains. In your words: An instance of User has both an Identity and a Principal, and Principal has an Identity. As stated in my comment on my answer, I believe only implementing IPrincipal is the only logical design when you want to bundle the two together. \$\endgroup\$ Apr 3 '11 at 2:44
  • 1
    \$\begingroup\$ @Steven, (1) The class design is not already made. The interface design is. If they were meant to always be separate classes, then they would be abstract classes instead of interfaces. (2) By design, IPrincipal has an IIdentity property. That does not necessarily mean that the Principal is different than the Identity. There is nothing wrong with an IPrincipal implementation that also implements IIdentity, and the implementation of IIdentity IPrincipal.Identity{get{return this;}}. \$\endgroup\$ Apr 3 '11 at 3:08
2
\$\begingroup\$

These interfaces are meant to work with the user’s identity information.

  • IIdentity: An identity object represents the user on whose behalf the code is running.
  • IPrincipal: A principal object represents the security context of the user on whose behalf the code is running, including that user's identity (IIdentity) and any roles to which they belong.

Just from looking at the documentation, your implementation looks weird. You are permanently linking the identity of a user to a fixed role. As far as I understand it, a user might operate on different roles at different times. This provides for better encapsulation. The main 'design' problem I see is your identity now contains an identity, which contains an identity, with inside, ... an identity, and there ... you get the point.

Futhermore, perhaps the default implementations of IIdentity and IPrincipal can already help you? Take a look at the generic implementations.

\$\endgroup\$
4
  • \$\begingroup\$ The stub code above doesn't show it, but my implementation does support a user being in multiple roles; Roles is a better name for the property. In my implementation, I have a User class that is tied directly to the database. I have a struct named UserInfo that implements the IUser interface and is constructed from the User object. The UserInfo is stored in the session (this is a web app), and is assigned to Request.User during Request.BeginRequest. I guess I don't understand why I wouldn't want a class to represent both the user and their current security context. \$\endgroup\$ Mar 25 '11 at 13:51
  • \$\begingroup\$ @Andy: Fair, but isn't that exactly what IPrincipal does, and why IPrincipal contains an IIdentity? \$\endgroup\$ Mar 25 '11 at 14:00
  • 1
    \$\begingroup\$ @Andy: If you really do want it in one class, I suggest only implementing IPrincipal, and initializing the identity in your class instead of passing it as a parameter. \$\endgroup\$ Mar 25 '11 at 14:12
  • \$\begingroup\$ As @Steven Jeuris notes, IPrincipal has a IIdentity. There might be a set of cases where something might both 'is a' and 'has a' IIdentity (duality of identities, perhaps for handling delegation or something of similar fashion and even then I might handle that scenario a bit differently), but I don't think that structure and behavior is what you are aiming for here. \$\endgroup\$
    – JustinC
    Mar 26 '11 at 22:36

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.