[Freeciv-Dev] Re: client/server authentication (PR#1767)

["Raimar Falke" <rf13@xxxxxxxxxxxxxxxxx>]

On Wed, May 07, 2003 at 07:54:20AM -0700, Mike Kaufman wrote:
> On Wed, May 07, 2003 at 06:32:57AM -0700, Raimar Falke wrote:
> > 
> > What if the user enters the password in the connect dialog? IMHO it
> > belongs there.
> 
> You're confusing me. We do this?!? Please compile the client and test this
> so we can be on the same wavelength.

I get gtk errors and a core dump with "./civ -a"


Gtk-CRITICAL **: file gtkwidget.c: line 3073 (gtk_widget_grab_focus): assertion 
`widget != NULL' failed.

Gtk-WARNING **: invalid cast from (NULL) pointer to `GtkEntry'

Gtk-CRITICAL **: file gtkentry.c: line 438 (gtk_entry_set_text): assertion 
`entry != NULL' failed.

Gtk-WARNING **: invalid cast from (NULL) pointer to `GtkButton'


#0  0x0808e574 in handle_authentication_request (packet=0x831f6d0)
    at connectdlg.c:94
94        gtk_set_label(GTK_BUTTON(connw)->child, _("Next"));
(gdb) bt
#0  0x0808e574 in handle_authentication_request (packet=0x831f6d0)
    at connectdlg.c:94
#1  0x0806544c in handle_packet_input (packet=0x831f6d0, type=135192641)
    at civclient.c:261
#2  0x08068a59 in input_from_server (fd=5) at clinet.c:330
#3  0x4018ca66 in gdk_io_invoke () from /usr/lib/libgdk-1.2.so.0
#4  0x401bdf9e in g_io_unix_dispatch () from /usr/lib/libglib-1.2.so.0
#5  0x401bf773 in g_main_dispatch () from /usr/lib/libglib-1.2.so.0
#6  0x401bfd39 in g_main_iterate () from /usr/lib/libglib-1.2.so.0
#7  0x401bfeec in g_main_run () from /usr/lib/libglib-1.2.so.0
#8  0x400dc2e3 in gtk_main () from /usr/lib/libgtk-1.2.so.0
#9  0x0809e488 in ui_main (argc=1, argv=0xbffff204) at gui_main.c:952
#10 0x08064bf0 in main (argc=1, argv=0xbffff204) at civclient.c:238
#11 0x403301c4 in __libc_start_main () from /lib/libc.so.6
(gdb) 

No problems without "-a".

For "-a" and also some other cases the user should _be able_ to save
his password in his freecivrc and the client should read it from this
location. The client shouldn't save it there. At least not without
asking the user.

The extra step where you type your password should be removed.

The server should sent a message text what the policy is. Something
like "Welcome foobar. Please enter your new password. It has to be at
least 4 characters and must contain at least 2 digits."

> > > yes we do if the server code checks the password.
> > > alternatively we might do pconn->server->user->password, but that's kinda
> > > ugly and probably not worth it.
> > 
> > struct user already contains a password field. Another one for
> > connection doesn't seem to be needed.
> 
> well, this is dependent on what we do below.
>  
> > > this is not double negation, but would you feel better if they were
> > > NEW_USERS_NOT_ALLOWED and GUESTS_NOT_ALLOWED?
> > 
> > IMHO "undef NEW_USERS_DISALLOWED" is double negation because it is the
> > same as "define NEW_USERS_ALLOWED". It should be NEW_USERS_ALLOWED and
> > GUESTS_ALLOWED.
> 
> The issue here is that these features are disabled by default. So you should 
> have to _define_ them to enable them rather than _undef_ them.

I disagree for the reasons outlined above. Reading the code should be easy.

> > > > > -#define MAX_NUM_CONNECTIONS (MAX_NUM_PLAYERS+MAX_NUM_BARBARIANS)
> > > > > +#define MAX_NUM_CONNECTIONS (2 * (MAX_NUM_PLAYERS + 
> > > > > MAX_NUM_BARBARIANS))
> > > > 
> > > > Reason?
> > > 
> > > People should be able to log in (to observe or multiconnect or whatever)
> > > even if all the players have been taken.
> > 
> > But isn't the same true for the current code with observers?! Except
> > the fact that it isn't used.
> 
> Yes, it's true. Please don't tell me you want a separate patch for this.

No. But _maybe_ you can do seperate commits:
 $ cvs commit server/setnet.h -m "increase ... because players want to ..."
 $ cvs commit # the rest

> > Server and userdb implementation both need user.h and user_db.h. IMHO
> > both should be in server/. The user_db.c should be renamed to
> > user_db_file.c and be put in server/userdb. For more complex
> > implementation we can also add subdirs under server/userdb.
> 
> no. you didn't read or remember previous posts. more complex
> implementations will do './autogen.sh --enable-auth=/path/to/libmyuserdb.a'
> which will replace the native user libuserdb.a. 

> There's no point whatsoever in including glue for a mysql database
> (for example) that 0.0001% of our users are going to want.

About what glue are we talking here?

> I can see user.h in server/ but for chrissakes, we've got a userdb/
> directory, user_db.h should be in it.

This may be an indication that the userdb/ directory isn't needed.

As a side note: I think we should seperate between the user_db
interface (user.h and user_db.h) and the implementation
(user_db.c). So user_db.c should be renamed so that it contains a
"file", "reg" or "simple".

> > > that's right. now let me tell you why we must have a callback for
> > > user_db_load and user_db_save. It is simply because in the industrial
> > > strength version of this, libuserdb.a is going to fork a process which 
> > > does
> > > lookup into a real database. We cannot have the server hang while this is
> > > happening. For various reasons, the lookup might take a while to return.
> > > Should a running game freeze while this is happening? In my opinion, no it
> > > shouldn't. Hence the callback. Yes there's going to need to be some more
> > > apparatus to stop "waiting" for a lookup to return (some check in
> > > sniff_packets probably), but damned if I'm going to entirely rewrite the
> > > connecthand code in the meantime.
> > 
> > What is so bad about waiting till the db returns the data? Do you have
> > data how long a SQL-DB lookup take?
> > 
> > If we do what you suggest we would have to disable somehow all players
> > which have an outstanding DB query from all actions. Also we would
> > have listen to these extra fds in the main-loop.
> 
> well, it's certainly not this complex, this since connections which have an
> outstanding query can't do anything anyway (although they can send more
> authentication requests, which I hadn't thought of, so I'll need to do
> something about this).
>  
> > I'm for waiting. It is easier to program. If there however are systems
> > which need 5s to retrieve the data and people want to use these as
> > user-db I will change my mind.
> 
> After I wrote this last night I thought about it for a while. I was on
> crack. I just did a bit of googling, and I see that databases ought to be
> faster (at least for the size of databases we're talking about --- I don't
> see our pubserver database going much beyond 20000 entries do you? ;) than
> the times we're worried about. There's also no reason to fork processes, we
> can put our db API right in the lib. Or if we do fork, we can put our
> timers in the lib and return after a suitable time.

Yes. What we want to do ('select * from users where name="foobar"') is
the best possible case for the DB. Because it only effects one row in
one table and name is the primary key or has an index added by
hand. If a DB can't do this query quickly it will needs eons to do
other stuff.

So there will be a nicer interface in the freeciv code in the next
patch?!

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Programming today is a race between software engineers striving to
  build bigger and better idiot-proof programs, and the Universe trying
  to produce bigger and better idiots. So far, the Universe is winning."
    -- Rich Cook