svn commit: samba r7058 - in branches/SAMBA_4_0: source/lib source/web_server swat/esptest

[Andrew Tridgell]

Simo,

Thanks for helping out with the esp code! Here are some comments on
your auth patch.

*) you should not use 
        #include "pwd.h"
   Instead, use 
    #include "system/passwd.h"
   as that brings in all the configure checks for different
   system authentication types.

*) The unixAuth() function you added takes only string arguments, so
    you should use espDefineStringCFunction() instead of
    espDefineCFunction(). The espDefineCFunction() call should only be
    used when one of more of the arguments is expected to be something
    other than a string. For example, the ldbSearch() function I added
    takes a hash for the list of attributes to search for, so I needed
    to use espDefineCFunction() so I could get at the hash
    elements. If you look at the other examples I've done (such as
    lpGet()) then you will see I used espDefineStringCFunction() as
    they only took string arguments. This means you can define
    esp_unixAuth() as:
         static int esp_unixAuth(struct EspRequest *ep, int argc, char **argv)
    which makes for considerably simpler code.

*) in esp_unixAuth() you explicitly set session['AUTHENTICATED'],
   session['USERNAME'] etc like this:

     mprSetPropertyValue(&ep->variables[ESP_SESSION_OBJ],
			"PRIVILEGE", mprCreateStringVar("ADMIN", 0));

   I think that is the wrong approach. Instead, I think that
   esp_UnixAuth() should return a ejs object, containing the elements
   you want. Then the esp script can do this:

     auth = esp_UnixAuth(user, pass);
     if (auth.authenticated) {
	/* user was authenticated OK as auth.username */
        session['AUTH'] = auth;
     }

   That makes for a much more flexible function. The auth code can put
   all relevent information (sid? method? token lifetime?) in the
   returned object, and it also allows for esp scripts that test
   authentication without actually becoming authenticated. For
   example, a test script might walk the samdb trying to authenticate
   as each user.

   If you have a look at how I return ldb messages in ldbSearch() you
   should see how to return general ejs objects.

   In general I think its a bad idea for our helper functions to set
   explicit variables in the ejs variable space. Instead they should
   return ejs objects, and let the script logic choose where to put
   the variable.

   I imagine you partly did this because of my comment about allowing
   the web server to check for auth on non-esp pages. I think that
   there are several ways to achieve this without tying the helper
   code to specific variables in this way. For example, we might want
   to have a /scripting/preauth.esp script that the web server always
   runs before any request (even requests for images and plain
   html). If that script produces output then that output is sent to
   the client and the page being asked for is not sent. That gives us
   a way to absolutely guarantee that every URL is authenticated,
   without requiring the error prone inclusion of a auth checking
   script at the top of every page.

*) You have a TODO like this:
   TODO: find out how to pass the real client name/address here 

   If what you want is the client IP address, then have a look at how
   the web server currently fills in request['REMOTE_HOST']. We should
   probably put this in web->input.remote_host to avoid multiple calls
   to socket_get_peer_name().

*) when a failure happens in a esp call, its a good idea to call
   espError() to set a nice error string describing what the caller
   did wrong. See the examples in esp_ldbSearch(). So for example you
   could do this:

   pwd = getpwnam(username);
   if (pwd == NULL) {
	espError(ep, "Unknown username '%s'", username);
	goto failed;
   }

   even better would be to fill in a 'reason' property on the returned
   object, and make esp_unixAuth() never return a failure.

   The problem with returning a failure from a esp helper function is
   that it immediately stops execution of the script. So if the user
   does this:

     res = ldbSearch("foo");

   then the script stops and outputs:

   ldbSearch invalid arguments Error on line 66. Offending line: res = ldbSearch("foo") 

   which is exactly what you want as giving only one argument to
   ldbSearch() is a fatal scripting error (the function must take at
   least 2 arguments). But if you do this:

     res = unixAuth("baduser", "badpass");

   then you don't want a fatal error to be thrown, what you want is
   for the script to be told that the auth failed. So you should do is
   return an object containing something like the following
   properties:

      res = unixAuth("baduser", "badpass");
      if (res.failed) {
	write("auth failed with reason " + res.reason);
      }

   To achieve that you need to return 0 (meaning success) from
   esp_unixAuth() and set the failed and reason properties on the
   returned object. We should probably create some helper functions
   similar to mprList() which make it easy to do this.


*) finally I agree with abartlet that we should not reproduce the pam
   auth code in source/web_server/. Instead, please use the generic
   auth infrastructure, or work with Andrew to add anything it needs
   if it isn't sufficient.

Sorry for such a long comment on such a simple commit! While you
probably know a lot of the above already, I thought it might be useful
for others who may wish to work with the new web server code.

Cheers, Tridge