best way for logging WHO did WHAT in spring web apps?

[maxjar10]

I would tend to agree with vmarcinko.

It seems like if you want to expose your business service easily through some kind of RMI (Webservices, Hessian, Burlap) then you would want to put credentials into each of the service methods that needed authentication/priviledges.

For example, take the PetClinic sample. In the PetClinicManager (or PetClinicService, whatever ) we would probably have something like the following:

interface PetClinicManager
{
     // Everybody has access to see all pets.
     public List getPets();

     // Only a given owner should be able to save their own pet's data.
     public Pet saveOrUpdate( String username, String password, Pet pet );
}

Pros:
1) No assumption is made about the user already being authenticated; The don't trust anyone principal.
2) Very easy to just slap any RMI on top of it and not worry about where authentication takes place (e.g. is it happening in WS-Security?). The authentication will happen in the BL.

Cons:
1) You need to have credentials passed around all the time and you keep needing to revalidate whoever the user is (in the case of most web applications you would have already done this so you are essentially doing it again! YUCK! :evil: ).
2) Potentially a large amount of code duplication (the number one code smell... phew!).

Using ThreadLocal
If we use ThreadLocal then maybe we can eliminate the username and password from being passed to the method. In fact, if we have the ability to set ThreadLocal then we could do the authentication of the user there and only store the username/User object in ThreadLocal as an authenticated user. We would still, however, need to verify that the user who was trying to update a pet in a call to saveOrUpdate( Pet pet ) was the owner of that pet.

Also, if we rely on ThreadLocal then we need to at least modify our RMI interfaces to have a way to set ThreadLocal (maybe using WS-Security in webservices?).

Using AOP
Now, if we use AOP and we stick with the convention that the username and password will be the first two method parameters then we will save ourselves some code duplication in doing authentication of the user and then in our saveOrUpdate() body we would assume that the username that was passed in was authenticated for us. However, we still need to verify that the user is actually the pet's owner .

As was mentioned by vmarcinko:

Using AOP seems as great thing for generic transparent stuff (such as transactioning), but not specific one. And what we have here is clearly very method-specific stuff.

In the example I posed the "saveOrUpdate" method requires a generic step to authenticate the passed in username and password but it requires a specific business logic to verify that the user is the pet's owner.

This is not to say that AOP is all bad. Certainly, if I have business methods that require a user to have a certain priviledge and the context of the method (e.g. what data I am performing logic on) doesn't matter then I believe AOP is a GREAT alternative. It simply breaks down a little bit when the context DOES matter.

My solution

For this example I would probably do the following:

// Please forgive the code, it's a little rought ;)
class PetClinicManagerImpl implements PetClinicManager
{
     ... omissions ...

     // Everybody has access to see all pets.
     public List getPets()
     {
          return petDAO.getAll();
     }

     // Maybe move this to some base class.
     public User authenticateUser( String username, String password )
     {
          User user = userDAO.getUser( username );
          if( user.getPassword().equals( password ) )
          {
                return user;
          }else
          {
                throw new UserAuthenticationFailedException( username );
          }
     }

     // Only a given owner should be able to save their own pet's data.
     public Pet saveOrUpdate( String username, String password, Pet pet )
     {
          Pet pet = null;
          User owner = authenticateUser( username, password );
          if( pet.getOwner().getUserName().equals( username ) )
          {
                pet = petDAO.saveOrUpdate( pet );
          }else
          {
                throw new InvalidAccessException( username, pet );
          }
          return pet;
     }
}

Now again, I am not too happy about the fact that I need to pass in both the username AND the password. An alternative might be to pass in the User object and just verify that the password matched but I don't know how much I have saved here other than saving on parameters (I still can't necessarily trust that the User object is already authenticated).

Enough babbling... Any thoughts?

Thanks in advance. Great discussion!

[Martin Kersten]

For me the answer is quite simple. It simply depends! ;-)

You know it just comes down to the responsibility (again). Where does the sphere of trust begins and where does it ends?

A user uses system interfaces to interact with the system. Ergo the user needs to use an interface in order to trigger a modification of the sate of the system.

So if the sphere of trust surrounds the whole domain layer, the interface is responsible for security issues related to authentication and identification. If more then one interface is used and for some of the interfaces the same user domain applies, the developers will find themselves to build abstract interfaces in order to satisfy the test-suite. Weapon of choice might be security policies and rejection of user requests.

If the user domain is always the same and we build a secure but responsive system (only autentificated users might use the service here), the security becomes mostly a functional requirement of the domain layer.

All the use cases are likely to contain sentences like: "if the user has the required rights, he might do xyz". In that case you have to let the security issues drip even further into the system. The sphere of trust would be quite small. So the security aspect gets omnipotent and explicit.

An example would be a banking system. Before money is moved the trust is checked by the money moving logic. This results mostly in passing certificates obtained by a trusted party (like the login service or the identification service authenticated the user using a certain interface).


For the petclinic example, I would say that the domain should be designed to be completely covered by the sphere of trust. Therefore I the interfaces (web and others) have to protect the system and have to maintain the sphere of trust. So I would most likely use the following sheme:

1. Identify the user. (including authentication).
2. Check if user has the required rights (might include forming classes of actions/commands like read-only vs. modification, affecting privacy rights of other persons or not etc.)
3a. If everything is ok, perform the requested task
3b. If not react to the security violation

So in the pet-clinic case, there would be no need to bother the domain layer with maintaining the sphere of trust. The interface is just responsible to ensure the sphere does not become compromised.

About the parameter container BizRequest, the problem is not with the parameter container - or better using a single object representing the informations of all parameters (I wouldn't call it a container, since containers seam to be anemic). The problem exist by reusing the same object in different situations. Therefore such a solution degenerates and an inversion of responsibility is reported by the test-suite instantly. So finally a command driven approach would be more natural, I guess.

[maxjar10]

So, if I understand you correctly you would have, for example, the web interface do the user authentication but you would still have the PetClinicManager check that the logged in user "owned" the pet that they were trying to modify? Essentially the code would become:

class PetClinicManagerImpl implements PetClinicManager 
{ 
     ... omissions ... 

     // Everybody has access to see all pets. 
     public List getPets() 
     { 
          return petDAO.getAll(); 
     } 

     // Only a given owner should be able to save their own pet's data. 
     public Pet saveOrUpdate( String username, Pet pet ) 
     { 
          Pet pet = null;

          // Assume that username is the currently logged in
          // in user and that they are authenticated already.
          User owner = userDAO.getUser( username ); 
          if( pet.getOwner().getUserName().equals( username ) ) 
          { 
                pet = petDAO.saveOrUpdate( pet ); 
          }else 
          { 
                throw new InvalidAccessException( username, pet ); 
          } 
          return pet; 
     } 
}

Or, would you move this check out into the interface tier as well? I can understand having the authentication happen outside of the domain layer but I believe the access control should live outside the interface tier.

Maybe an alternative would be to have an AccessControlManager that encapsulates the "rights" logic. The only question then is who invokes the methods on the AccessControlManager (domain or interface tiers).

For example:

interface AccessControlManager
{
      public bool hasWritePermission( Object object );
}

class AccessControlManagerImpl
{
     ... omissions ...
     public bool hasSavePermission( Object object );
     {
          // Get the username from a ThreadLocal
          username = userContext.get().getUserName();
          
          if( object.getClass() == Pet.getClass() )
          {
                Pet pet = (Pet)object;
                if( pet.getOwner().getUserName().equals( username ) )
                {
                      return true;
                }
          }else
          {
                throw new ClassNotSupportedException( object.getClass() );
          }

          return false;
     }
}

class PetClinicManagerImpl implements PetClinicManager 
{ 
     ... omissions ... 

     // Everybody has access to see all pets. 
     public List getPets() 
     { 
          return petDAO.getAll(); 
     } 

     // Only a given owner should be able to save their own pet's data. 
     public Pet saveOrUpdate( Pet pet ) 
     {
          if( accessControlManager.hasSavePermission( pet ) )
          {
                return petDAO.saveOrUpdate( pet );
          }else
          {
                throw new AccessControlException( pet );
          }
          return null;
     } 
}

It seems you could also use AOP on any method that would save/modify the objects and have it user your AccessControlManager. Is this functionality similar to the Acegi ACL? I haven't found any good tutorials on using Acegi to protect my domain objects like the above.

[Ben Alex]

It seems you could also use AOP on any method that would save/modify the objects and have it user your AccessControlManager. Is this functionality similar to the Acegi ACL? I haven't found any good tutorials on using Acegi to protect my domain objects like the above.

I found this entire thread interesting. Basically everything described has already been done in Acegi Security - in most cases for well over 12 months. The ACL package is entirely pluggable and delivers the type of information you're seeking. Here's the main ACL manager's interface:

public interface AclManager {
    //~ Methods ================================================================

    /**
     * Obtains the ACLs that apply to the specified domain instance.
     *
     * @param domainInstance the instance for which ACL information is required
     *        &#40;never <code>null</code>&#41;
     *
     * @return the ACLs that apply, or <code>null</code> if no ACLs apply to
     *         the specified domain instance
     */
    public AclEntry&#91;&#93; getAcls&#40;Object domainInstance&#41;;

    /**
     * Obtains the ACLs that apply to the specified domain instance, but only
     * including those ACLs which have been granted to the presented
     * <code>Authentication</code> object
     *
     * @param domainInstance the instance for which ACL information is required
     *        &#40;never <code>null</code>&#41;
     * @param authentication the prncipal for which ACL information should be
     *        filtered &#40;never <code>null</code>&#41;
     *
     * @return only those ACLs applying to the domain instance that have been
     *         granted to the principal &#40;or <code>null</code>&#41; if no such ACLs
     *         are found
     */
    public AclEntry&#91;&#93; getAcls&#40;Object domainInstance,
        Authentication authentication&#41;;
&#125;

I would warmly encourage anything with ACL related requirement to try Acegi Security. If it doesn't do what you need, it can easily be customised to do it. :-)

[Martin Kersten]

So, if I understand you correctly you would have, for example, the web interface do the user authentication but you would still have the PetClinicManager check that the logged in user "owned" the pet that they were trying to modify? Essentially the code would become:

For the pet-clinic, I guess no. The PetClinicManager is domain related and therefore I would avoid to add the security responsibility to any domain layer component. I just would add another layer to the onion (the user interface layer).

Or, would you move this check out into the interface tier as well? I can understand having the authentication happen outside of the domain layer but I believe the access control should live outside the interface tier.

What is the outside of the interface tier looking like? How does it called?

Maybe an alternative would be to have an AccessControlManager that encapsulates the "rights" logic. The only question then is who invokes the methods on the AccessControlManager (domain or interface tiers).

In your example you made the domain layer responsible. Another idea would be using a warper implementation of PetClinicManager. This implementation would be interface related as long as it is declared in the interface tier(?).

public class PetClinicManagerImpl implements PetClinicManager {
   PetClinicManager businessDeligate=...;
   public Pet saveOrUpdate(Pet pet ) {
        User user=session.getUser();
        UserRights userRights=UserRightsService.getUserRights(user);
        userRights.ensureModificationRightsAreGranted();
        businessDeligate.saveOrUpdate(pet);
   }
}

Of cause a true domain service shouldn't have a saveOrUpdate method since this method does provide no true service and is just used to interact with the persistence layer. Using such a method is an indication that the domain use-cases are not fully implemented and even fully understood.

It seems you could also use AOP on any method that would save/modify the objects and have it user your AccessControlManager.

I wouldn't use AOP, I guess. I would require to explicitly add a SecurityManager or an AccessControlManager to the interface. If an implicit method would be used (AOP, Interceptor, Listener etc), the security aspect would not be part of the user interface (tier) and up to the configuration, which I wouldn't buy here.

Thinking about it, I would guess that the user interface is just another part of the presentation layer (which I would rather call user interface (ui) layer anyways). It is a specialisation of a generic domain service interface of the domain layer. So it is just a way to limit the affect of the security aspect towards the domain layer. Thinking further about it, the generic domain service interface is also belonging to the presentation layer. (remember the Inversion of Ownership (Interface belongs to the client) principle). But the onion model saves us here, I guess. The domain layer is the client here, where the user is the client in the ui layer. Pui, we are saved ;-).


Cheers,

Martin (Kersten)

[maxjar10]

So you have another layer between your UI tier and your business tier...

Swing or WebUI --> Security Tier (Wrappers) --> Business Tier --> DAO

In your example you do a check to see if a user has can make modifications but you don't associate it with the Pet object at all which was kind of the goal of the exercise (only the owner of the pet can modify the pet). It seems like you would need to do:

userRights.ensureModificationRightsAreGranted( pet );

Also,

Of cause a true domain service shouldn't have a saveOrUpdate method since this method does provide no true service and is just used to interact with the persistence layer. Using such a method is an indication that the domain use-cases are not fully implemented and even fully understood.

Is it that I said "saveOrUpdate" instead of "savePet" (which was just an oversight)? There are many cases when the business service will simply interact with the DAO (for example, getPet( Integer petId ) will call the corresponding method in the DAO).

I am afraid I don't understand what you mean here:

I wouldn't use AOP, I guess. I would require to explicitly add a SecurityManager or an AccessControlManager to the interface. If an implicit method would be used (AOP, Interceptor, Listener etc), the security aspect would not be part of the user interface (tier) and up to the configuration, which I wouldn't buy here.

[Martin Kersten]

In your example you do a check to see if a user has can make modifications but you don't associate it with the Pet object at all which was kind of the goal of the exercise (only the owner of the pet can modify the pet).

Well it just wasn't the goal of the exercise for me, but sure you can add a dependency for pets, too.

Is it that I said "saveOrUpdate" instead of "savePet" (which was just an oversight)? There are many cases when the business service will simply interact with the DAO (for example, getPet( Integer petId ) will call the corresponding method in the DAO).

The DAO shouldn't do any security checking. Security is beyond the scope of a DAO. Also I think that a 'savePet' method is still a method I consider an implementation detail of the domain layer. Therefore it should by no means be exposed to other layers.

For example, you might have some business methods like: renamePet(Pet) or createPet(Pet) but I can't consider a use-case which would be named savePet. Maybe in a dangerous situation when you call in a special doctor to save the life of a pet but that isn't what the questioned savePet method is about, I guess. ;-)

[Martin Kersten]

I am afraid I don't understand what you mean here:

There are usally two ways, things can be done. One way is explicit by saying exactly what should happen and the another way is implicit. Using the implicit way, there is no garantee that things really happen and a form of a written contract using normal language is mostly needed to specify what is needed to make the given solution correct.

Written contracts using normal language act against agility of the source code (like extense and unnecessary documentation do) and mostly require that contexts etc. have to be tested also (acceptance/integration tests).

Everything that is explicitly expressed by any use-case should be also explicitly expressed by the source code, to force the test-suite to cover and document the use-case as a whole.

Using declarative AOP is an implicit way. The advised object itself is not responsible (or even aware) for anything that the advised aspect does. That's why AOP is often referred as being over-used (or abused) in certain situations. The responsibilities are just taken away from the object (and most importantly from its test-code) and is put into aspects. To ensure that a system using AOP (exspecially declarative AOP) is correct this requires acceptance and integration tests to test this details explicitly.

So ensuring correct transactional behaviour becomes a burden when you want to formulate acceptance (or integration) tests. There exist some good ways to limit the negative effects of declarative AOP towards the test-suite but often the way used is to ignore the issue and hope for the best. I survived with this ignorance for a small project, too. But I always had a bad feeling about it... .


Cheers,

Martin (Kersten)

[maxjar10]

Martin,

Thanks for all your time in explaining your opinion. There are a number of things that I agree with but there are still some points that I would differ on.

The DAO shouldn't do any security checking. Security is beyond the scope of a DAO.

I agree. But I don't think that I ever implied the DAO should do security checking. I did state that the business tier over the DAO should do the security checking.

For example, you might have some business methods like: renamePet(Pet) or createPet(Pet) but I can't consider a use-case which would be named savePet.

I'll buy that. But I would probably save myself from creating a bunch of methods and make it updatePet( Pet ) and insertPet( Pet ). Also, I should be able to set/get the pet's name in the Pet object itself. However, this can also lead to problems if certain things are lazily loaded from the DAO where again you wind up with some unfortunate cases where certain things in your domain layer affect your implementation at a higher level (you can mitigate the lazy loading with OpenSessionInView pattern but this also has some deficiencies). This is an issue that does bother me as well... Anyway, that is a tangent that is somewhat outside the scope of this topic anyway so I digress...

Everything that is explicitly expressed by any use-case should be also explicitly expressed by the source code, to force the test-suite to cover and document the use-case as a whole.

Hmmm... there are parts of that I agree with but... AOP will help address cross-cutting concerns in one's design. Things like transactions, logging, security may all be considered cross-cutting in some cases but in others maybe not?

In this particular example let's say the use cases are:
1) A user must be able to insert/create pets in the system.
2) A user must be able to rename a pet.
3) A user must be able to add another owner to a pet.
4) A user must be able to neuter their pet. :lol:
5) A user must be able to delete a pet. (how sad :wink: )

Now, a cross-cutting concern might be:
1) User's should only be able to modify their own pets.

We could, as you mentioned, put a security wrapper over the business service and create a service to inform us of user rights. As long as this wrapper is properly engineered in can be reused regardless of the presentation (Swing, WebUI, RMI). If we do this then for each of the above use cases (save maybe insert/create) we need to check the user rights service to verify the user can modify this pet.

If we use AOP to accomplish the above then we can potentially avoid code duplication (e.g. having code in all our security wrapper methods to check the user's rights on the pet). In this case we are essentially letting Spring create our security wrapper for us (because it is really just a proxy) and provide pointcuts on use cases 2 through 5.

As far as testability is concerned I will concede that it is probably easier to unit test the hand coded security wrapper because it can easily be taken outside the context of the rest of the system and you can use mock objects to simulate the delegate/user rights management. In the case of AOP you will be doing integration testing but it is possible through the miracle of spring to actually configure your AOP proxy to use mock objects as well. I believe you can even create your AOP proxy with a target that is either a test object or a mock.

The advised object itself is not responsible (or even aware) for anything that the advised aspect does. That's why AOP is often referred as being over-used (or abused) in certain situations. The responsibilities are just taken away from the object (and most importantly from its test-code) and is put into aspects.

I think when you put a security wrapper over the object you are still removing responsibility from the advised object anyway. You are moving it to the security wrapper who is responsible for this part of the system.

From the Acegi (http://acegisecurity.sourceforge.net/faq.html) FAQ:

1) Separation of concerns: Authorization is a crosscutting concern and should be implemented as such. MVC controllers or views implementing authorization code makes it more difficult to test both the controller and authorization logic, more difficult to debug, and will often lead to code duplication.

2) Support for rich clients and web services: If an additional client type must ultimately be supported, any authorization code embedded within the web layer is non-reusable. It should be considered that Spring remoting exporters only export service layer beans (not MVC controllers). As such authorization logic needs to be located in the services layer to support a multitude of client types.

3) Layering issues: An MVC controller or view is simply the incorrect architectural layer to implement authorization decisions concerning services layer methods or domain object instances. Whilst the Principal may be passed to the services layer to enable it to make the authorization decision, doing so would introduce an additional argument on every services layer method. A more elegant approach is to use a ThreadLocal to hold the Principal, although this would likely increase development time to a point where it would become more economical (on a cost-benefit basis) to simply use a dedicated security framework.

4) Authorisation code quality: It is often said of web frameworks that they "make it easier to do the right things, and harder to do the wrong things". Security frameworks are the same, because they are designed in an abstract manner for a wide range of purposes. Writing your own authorization code from scratch does not provide the "design check" a framework would offer, and in-house authorization code will typically lack the improvements that emerge from widespread deployment, peer review and new versions.

Thanks,
maxjar10

[Martin Kersten]

Also, I should be able to set/get the pet's name in the Pet object itself.

So everyone obtaining a pet may change its name? The setting of a pet or renaming of a pet I would never expose to the outside of a domain layer. I had bad some bad moments with this. Implementations of the Pet object might have such a method to ease overall implementation but this is transparent for the user of a Pet object.

This is an issue that does bother me as well... Anyway, that is a tangent that is somewhat outside the scope of this topic anyway so I digress...

I think that the presentation layer of a web application is most likely to contain parts, I would consider to be domain layer related. Even like to defining and populating a view model for a certain view (tile). So it is more like adding an adapter to the real 'generic' domain layer here. But I am not sure since the context of each domain part seam to be related but have some abnormality, I am not understanding fully.

As far as testability is concerned I will concede that it is probably easier to unit test the hand coded security wrapper because it can easily be taken outside the context of the rest of the system and you can use mock objects to simulate the delegate/user rights management. In the case of AOP you will be doing integration testing but it is possible through the miracle of spring to actually configure your AOP proxy to use mock objects as well.

Seams like I wasn't that clear. I wasn't meant to talk about ease of implementing a certain test but about the overall design of the test-suite. Using mocks or not isn't that issue I wanted to talk about.

The problem comes on trying to test a contract written using natural language. There must be hints in the documentation how and why to advise certain objects. Also the different context definitions become part of the test-suite/documentation as well. And that makes sometimes not much sense. (but sometimes it does of cause)

I think when you put a security wrapper over the object you are still removing responsibility from the advised object anyway.

The scenario was about formulating a real interface. I really think that the use of the keyword interface is a big problem in the Java world. The interface I ment was a specification of a communication channel both parties have to agree on in order to interact properly. But this specification must be expressed by the documentation (and documentation is best done by plain source code). And this specification has to be ensured by the test-suite. A good test-suite would make extense documentation (either in source code or separated documentation) unnecessary.

In the scenario I talked about, the interface was responsible for the security checking. That's why I wouldn't use AOP here. But if you adapt security issues to an existing interface, AOP would be a helper (but I would rather go with a explicit specialisation, anyways).

Authorization is a crosscutting concern

Sometimes yes, but sometimes not. I think that mostly AOP is used to cover more objects by the sphere of trust because it is so easy to reuse an aspect. But if you analyse such scenarios, it often would be better to do it explicit or at least using a single aspect for a single object to narrow the sphere of trust and to simplify the test suite or even allow some testing of the security related parts, afterall.

Take the example of only allowing modification of pets the current user owns. The problem is that the aspect has to be aware of the particular pet to do the checking. Using declarative AOP would be not looking that natural for me. It is a functional requirement of the business interface object, I guess. So I would try to make it explicit, which often suites great. But maybe injecting a security manager as a dependency is a great way to do it. By injecting a security manage you can also reuse your security managers. (which often works like a charm).

2) Support for rich clients and web services: If an additional client type must ultimately be supported, any authorization code embedded within the web layer is non-reusable. It should be considered that Spring remoting exporters only export service layer beans (not MVC controllers). As such authorization logic needs to be located in the services layer to support a multitude of client types.

So we have two different clients (web client and rich client) in this scenario. The question is, if the special interfaces both clients use to interact with the domain layer has to be secured independently. (AOP solutions most likely do so, since it is so easy to advise a method). Most likely securing the domain layer by pushing the security requirements even more toward the domain layer objects, might suite better. (this would also narrow the sphere of trust).

4) Authorisation code quality: It is often said of web frameworks that they "make it easier to do the right things, and harder to do the wrong things". Security frameworks are the same, because they are designed in an abstract manner for a wide range of purposes. Writing your own authorization code from scratch does not provide the "design check" a framework would offer, and in-house authorization code will typically lack the improvements that emerge from widespread deployment, peer review and new versions.

I like to use all kinds of frameworks or 3rd-party API. Everything that you don't have to write but you can rely to, is a relief. I also use Acegi myself. It seam to do a fine job. (I am still learning here, I just got into it some weeks ago). But there is no different in thinking. Before I use declarative AOP in production code, I like to do a bit more of a thinking. I need really good reasons to stick to AOP. I find declarative AOP to be just very expensive in terms of the impact it has on the test-suites and the documentation.

But never the less, I use declarative AOP myself, while I implement something. For example when doing transactional work but not knowing where things might end. It is like logging. Great helper during the developing phase, but quite harmful in terms of production code. If you need logging there are mostly special requirements where common loggings may fail.

But this really depends on the way someone is used to program or the amount of documentation being expressed by the test-suite/source-code. I think it often acts against agility, but this is an opinion.