RE: user.c, user.h

[Andrey Aristarkhov]

> -----Original Message-----
> From: bug-cvs-admin@gnu.org [mailto:bug-cvs-admin@gnu.org] On Behalf
Of
> Derek Robert Price
> Sent: Tuesday, August 13, 2002 5:57 PM
> To: Andrey Aristarkhov
> Cc: bug-cvs@gnu.org
> Subject: Re: user.c, user.h
> 
> Hrm.  I should probably point out that the changes I'm requesting
won't
> guarantee that your patch is accepted, but they will make it more
> likely.  Please read the `HACKING' file in the top level of the CVS
> source distribution for more:
> <http://ccvs.cvshome.org/source/browse/ccvs/HACKING>.
> 
> Also, just the standard reminder, sumbission of your work to
> bug-cvs@gnu.org grants the right to use your code in CVS and
distribute
> it under the GNU GPL.
I know that.

> Please see the `HACKING' file in the top level of the CVS source
> distribution for instructions on the submission of patches.  You are
> missing documentation (`cvs.texinfo') changes and test suite
> (`src/sanity.sh') changes.  These are required for every change to the
> CVS source because otherwise those sections of the code tend to
> deteriorate rapidly.
I've never used TeX. I'll try to write.

> 
> [ . . . snip . . . ]
> 
> >static int
> >extendline(char **buf, int * buflen, int needed)
> >{
> >     if (needed > *buflen) {
> >             char    *tmp = realloc(*buf, needed);
> >             if (tmp == NULL)
> >                     return -1;
> >             *buf = tmp;
> >             *buflen = needed;
> >     }
> >     return *buflen;
> >}
> >
> >static int
> >extendarray(char ***buf, int * buflen, int needed)
> >{
> >     if (needed > *buflen) {
> >             char    **tmp = realloc(*buf, needed * sizeof(char *));
> >             if (tmp == NULL)
> >                     return -1;
> >             *buf = tmp;
> >             *buflen = needed;
> >     }
> >     return *buflen;
> >}
> >
> >
> 
> extendline() has an analogue in `src/subr.c'.  I'll grant that
> expand_string() may allocate more space than you really wanted, but
> please at least use xrealloc() from `src/subr.c' in favor of the
system
> realloc() in both these functions if you keep extendline().
It's ok. I'll make changes. 

> 
> >static char * get_password(const char * user) {
> >  static char pwd[MAX_STRING_LEN];
> >  char line[MAX_STRING_LEN];
> >  FILE * fpw=NULL;
> >  char * token;
> >  fpw = fopen (getpwpath(), "r");
> >  if (fpw == NULL) {
> >     return NULL;
> >  }
> >  memset(pwd,0,MAX_STRING_LEN);
> >  while (!(getline (line, MAX_STRING_LEN, fpw))) {
> >    if (line[0] == '#') {
> >      continue;
> >    }
> >    token = strtok(line,":");
> >    if (token == NULL) {
> >      continue;
> >    }
> >    if (strcmp (user, token) != 0) {
> >      continue;
> >    }
> >    /* User found */
> >    token = strtok(NULL,":");
> >    strcpy(pwd,token);
> >
> >    fclose(fpw);
> >    return pwd;
> >  }
> >  fclose(fpw);
> >  return NULL;
> >}
> 
> This is being done in `login.c'.  This functionality should be
factored
> into a common function.
I guess. I suppose there is a number of code duplication in user.c, but
it's because originally it was written from scratch as a standalone
program. I'll take into consideration your comments similar to this one
bellow.

[skipped]

> Oops, I almost missed getpwpath().  I deleted your code already, but
> there is already a construct_cvspass_filename() function in `login.c'
as
> well.
construct_cvspass_filename() constructs path to user's password file. My
{set,get}pwpath functions is used to store path to CVSROOT/passwd file.

> 
> [ . . . snip . . . ]
> 
> >/*
> > * Reads and verifies user's password from an input
> > * Return password for the user
> > */
> >#define safe_string(a) a ? a : "NULL"
> >
> >static char * read_user_password(const char * user, int
pwd_read_mode) {
> >  char prompt[128];
> >  char * pwd, * pwd_verify;
> >  static char password[_PASSWORD_LEN];
> >
> >  memset(password,0,_PASSWORD_LEN);
> >  switch (pwd_read_mode) {
> >    case PRM_NEW:
> >      sprintf(prompt,"Enter New password for user '%s':",user);
> >      /* We need to strdup because GETPASS uses static object */
> >      pwd_verify = GETPASS(prompt);
> >      verify_password_phrase(pwd_verify,TRUE);
> >      pwd = strdup(pwd_verify);
> >      memset(pwd_verify,0,strlen(pwd_verify));
> >      sprintf(prompt,"Re-type New password for user '%s':",user);
> >      pwd_verify = GETPASS(prompt);
> >      verify_password_phrase(pwd_verify,FALSE);
> >      if (strcmp(pwd,pwd_verify)) {
> >        free(pwd);
> >        error(1,0,"Passwords are different");
> >      } else {
> >        free(pwd);
> >        strcpy(password,pwd_verify);
> >        memset(pwd_verify,0,strlen(pwd_verify));
> >      }
> >    break;
> >
> >   case PRM_CHECK:
> >    pwd = get_password(user);
> >    sprintf(prompt,"Enter current password for user '%s': ",user);
> >    pwd_verify = GETPASS(prompt);
> >    verify_password_phrase(pwd_verify,FALSE);
> >    if (strcmp(pwd,crypt(pwd_verify,pwd))==0) {
> >      strcpy(password,pwd);
> >    } else {
> >      error(1,0,"Wrong password",user);
> >    }
> >   break;
> >
> >   case PRM_ADMIN:
> >    pwd = get_password(CVS_ADMIN_USER);
> >    if (pwd == NULL)
> >      error(1,0,"Administrator's user accout not found. Please
contact
> your CVS admin.");
> >    if (strcmp(pwd,"*")==0)
> >      error(1,0,"Password for Administrator's user accout is not set.
> Please contact your CVS admin.");
> >    pwd_verify = GETPASS("Enter CVS Administrator password: ");
> >    verify_password_phrase(pwd_verify,FALSE);
> >    if (strcmp(pwd,crypt(pwd_verify,pwd))==0) {
> >      strcpy(password,pwd);
> >    } else {
> >      error(1,0,"Administrator password is invalid");
> >    }
> >   break;
> >  }
> >  return password;
> >}
> >
> >
> 
> Again, much of this happens in `server.c'.  Please reuse code.  And
> unless I'm mistaken, this asks for the admin password - that shouldn't
> be happening.  If the user is an administrator they should already
have
> authenticated.
I've investigated server.c and login.c before writing this code. There
is no reusable code over there. The only thing I've taken from login.c
is GETPASS macro definition. Suppose it should be placed in cvs.h header
file.

> As for allowing any user's password to be changed if you know the
user's
> password, I'm against that.  It's unneeded overhead.  Go ahead and
> verify a user's password, but make other users log in to change their
> own password or let the admin do it.
I've implemented (I hope) UNIX-like behavior of passwd command.
Try to login and run 'passwd' under UNIX. First you will be asked for
current user password. It's not overhead. If 'cvs pass' will rely on
current user password anyone can change it having physical access to
user workstation.
At the same time 'cvs user' command needs admin privileges, except it
emulates 'cvs passwd' command like this: cvs user -m $USER -p.

Regards,
Andrey