[freeside-devel] Re: Major patch
ivan
ivan at 420.am
Thu Dec 14 19:08:33 PST 2000
On Mon, Oct 09, 2000 at 08:13:56PM -0700, Jason Spence wrote:
> On Sat, Oct 07, 2000 at 05:19:24AM -0700, ivan developed
> a new theory of relativity and:
>
> > > Javascript to do input validation, client-side password generation,
> > > username capitalization, etc. in CGI.pm
> >
> > There should be a configuration option to toggle all javascript off/on
> > (defaulting to on is fine) as well as configurtaion options to turn off
> > anything that people might not want (for example, all installations might
> > not want username capitalization).
> >
> > I'll probably outright reject client-side password generation, that sounds
> > like a bad idea.
>
> I agree. One thing I've been thinking about is being able to
> customize the location of the directory that FS::Conf uses. Really
> useful for running two copies of freeside simultaneously, or running a
> copy in your home directory on a machine where you don't have (or
> want) root access for freeside. Problem is that I only see two
> options for being able to customize that location:
>
> 1) Define the location during installation of freeside, or
>
> 2) Have it in a global config file in a static set of locations, such
> as @config_files =
> qw(/etc/freeside.conf $ENV{'HOME'}/.freeside/freeside.conf);
>
> What do you think?
I'm not interested in support non-root installs. Either of these is fine.
Note that FS::Conf already does some tricky stuff via mapsecrets to
support running man copies simultaneously, so don't break that.
> > Yes, you could database the Apache auth and put everything in the
> > database. For an application such as yours, with a large number of
> > employees, this is probably the way to go. Apache::AuthDBI etc. I can
> > provide some help on this.
>
> Ah, excellent. It's listed under ApacheDBI on CPAN for some odd
> reason. This looks like exactly what we need.
>
> > > Commissions report
> >
> > Very interested.
> >
> > > We've been using a jailed perl, so we have a separate patch to go in
> > > and change the shabang in every perl script to something like
> > > #!/home/freeside/j/bin/perl
> >
> > Aren't you using mod_perl?! The shebang line should be irrelevant on all
> > the htdocs/ stuff in that case.
>
> I've never used mod_perl. Is that best practice in the CGI community?
> Lemme go poke around on google...
>
> It does WHAT?! Stuffs a Perl interpreter in Apache? Hoi, what a job
> those maintainers must have :)
>
> I don't understand why the shebang is irrevelant in this case. Could
> you please enlighten me?
Because the scripts aren't executed by the shell, which needs to know what
interpreter to run them with - they're loaded up as subroutines
directly by mod_perl (specifically, Apache::Registry).
> Thinking about it, this seems like a very
> good solution to a lot of the problems that CGI scripts have. From
> what I'm reading over here in this window, it looks like you can
> create persistent CGI objects that are not subject to the usual
> one-shot execution problems that I've been frustrated with every time
> I do CGI programming. Am I on the right track here?
Well, you get to load in and compile your code only once, but HTTP is
still transation based - there's no guarantee one click will be handled by
the same Apache child as another, so you still need to do much the same
magic to support sessions as usual. See Apache::Session and the mod_perl
guide, etc.
> > About the only thing here useful for integration into the base release is
> > maybe a patch to rewrite the shebang line in all the scripts at
> > compile-time or something. Doesn't seem terribly important to me; no
> > worries if it isn't worth your time.
>
> You mean in Makefile.PL? I don't know very much about MakeMaker, but
> I've been thinking that there ought to be a Makefile in the root of
> the distribution that...
>
> Hey, we could use autoconf to see if Perl et al is installed and then
> have that create the Makefile which would in turn call "perl
> FS/Makefile.PL"! What do you think?
Sure.
> > > Rewired "shellmachine" which allows many shell machines and autocalc
> > > of MD5 password hash for useradd -p
> >
> > Hmm. :/ I'd like to see this.
>
> Yeah, I saw that there were a few hooks in there to do this already,
> but you were throwing the password together by hand:
>
> <quote>
> [thalakan at host30 freeside]$ find | xargs grep -is salt *
> ./FS/FS/svc_acct.pm: @saltset @pw_set);
> ./FS/FS/svc_acct.pm:@saltset = ( 'a'..'z' , 'A'..'Z' , '0'..'9' , '.' , '/' );
> ./FS/FS/svc_acct.pm: # crypt($3,$saltset[int(rand(64))].$saltset[int(rand(64))]
> ./bin/svc_acct.export:my(@saltset)= ( 'a'..'z' , 'A'..'Z' , '0'..'9' , '.' , '/' );
> ./bin/svc_acct.export: $saltset[int(rand(64))].$saltset[int(rand(64))]
> ./fs_passwd/fs_passwd_server: my $salt = substr($old_password,0,2);
> ./fs_passwd/fs_passwd_server: my $cold_password = crypt($old_password,$salt);
> ./fs_webdemo/register.cgi: #@pw_set @saltset
> ./fs_webdemo/register.cgi:#@saltset = ( 'a'..'z' , 'A'..'Z' , '0'..'9' , '.' , '/' );
> ./fs_webdemo/register.cgi:#$crypt_pw = crypt($user_pw,$saltset[int(rand(64))].$saltset[int(rand(64))]);
> ./fs_webdemo/registerd: @pw_set @saltset
> ./fs_webdemo/registerd:@saltset = ( 'a'..'z' , 'A'..'Z' , '0'..'9' , '.' , '/' );
> ./fs_webdemo/registerd: crypt($user_pw,$saltset[int(rand(64))].$saltset[int(rand(64))]);
> </quote>
>
> I used the Crypt::MD5 module, which I think makes the code cleaner.
MD5 is not the same as classic UNIX crypt() encryption. The code should
support *both*.
>
> <quote file="FS/svc_acct.pm">
> $itoa64 = "./01234567890ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
> $buf = "";
> for($i = 0; $i < 2; $i++) {
> $buf .= substr $itoa64, rand(length($itoa64)), 1;
> }
> $password = unix_md5_crypt($password, $buf);
>
> if ( $username
> && $uid
> && $dir
> && $password
> && ! $nossh_hack ) {
> #one way (note the new loop)
> foreach $i (@shellmachine) {
> ssh("root\@$i",
> "useradd -g users -d $dir -m -p '$password' -s $shell -u $uid $username && chmod 750 $dir"
> );
> }
> #another way
> #ssh("root\@$shellmachine","/bin/mkdir $dir; /bin/chmod 711 $dir; ".
> # "/bin/cp -p /etc/skel/.* $dir 2>/dev/null; ".
> # "/bin/cp -pR /etc/skel/Maildir $dir 2>/dev/null; ".
> # "/bin/chown -R $uid $dir") unless $nossh_hack;
> }
>
> ''; #no error
> </quote>
>
> I really should make those parameters to useradd configurable, but
You're out of date. Current code has all remote commands configurable.
> but they get the job done for adding users to our mail and web servers.
> I don't see a clear difference between the "shellmachine" and
> "shellmachines" config files.
I believe you do now.
> The documentation says that one is used
> for manipulating remote home directories, and the other is used for
> manipulating password file entries. I don't understand why there
> isn't a more flexible mechanism for defining an array of systems which
> have flags that specify whether you want home directories created,
> password entries created, where to get the GECOS field from, whether
> the system is using LDAP, etc, etc.
Because no one wrote it yet.
> > > Patches to the RADIUS export script to make it work better with
> > > cistron radiusd when there is no ACP stuff installed. What is this
> > > ACP stuff, anyway? AIX Certified Professional? Another Command
> > > Processor? The Bay Networks ACP radius-alike?
> >
> > The last. Xircom's actually IIRC, they were swallowed by Bay. Of
> > questionalbe usefullness, but it doesn't break anything, so no reason to
> > remove it. Why is it causing you problems?
>
> Because in svc_acct.export, there's stuff like this:
>
> er, I can't seem to find it. I remember that the 1.2.2 version you
> originally installed had the users and acp files done at the same time
> or something, but I think I was mistaken. I do remember having to
> hack the file to copy the password entries manually, but then I did
> the shellmachine thing like I explained above. Hmm. Nevermind.
>
> > In an ideal world, it would be against the current CVS, and you'd
> > either:
> >
> > -feed it to me in small pieces, broken up logically
> > -check it into a branch in cvs and help merge it onto the trunk
>
> Yeah, I'm porting the patch to the latest version of Freeside. You
> should have seen me check it out Wednesday last week or so. I'm not
> very good at merging trunks, so I think I'll make lots of little
> patches instead.
>
> > It would be better if you could port it against current CVS. A 1.2.4
> > release is kind of overdue. :)
>
> Heh. No problem. Fry's has glacial release schedules for their POS
> software, so I'm used to it.
>
> > Notwithstanding any problems with my style (I'm sure there are), doesn't
> > this make it extrememly difficult for you to track my source, and
> > vice-versa?
>
> We made a branch internally that I used to play around with. We then
> port the patches by hand once we have the changes right in the branch.
>
> > > A few people over here have pointed out the inconsistent
> > > tabbing in the code, which they say makes it very difficult to read.
> > > How do you feel about this?
> >
> > *sigh* I don't have much of an opinion, really. I mean, I don't think
> > the indentation style is particulary difficult, but it's my code.
> >
> > I do want it be as clear as possible for other folks and am amenable to
> > patches that format all the code according to a common style. I'd want to
> > keep this separate from any functionality patches as it is likely to take
> > longer, and I'd want to see at least a loosely-defined style guide that
> > doesn't conflict too much with perlstyle(1).
>
> I agree. One of the major problems I had while I was reading your
> code initally is better explained by the man page than I:
>
> o Just because you CAN do something a particular way doesn't mean that you
> SHOULD do it that way. Perl is designed to give you several ways to do
> anything, so consider picking the most readable one. For instance
>
> open(FOO,$foo) || die "Can't open $foo: $!";
>
> is better than
>
> die "Can't open $foo: $!" unless open(FOO,$foo);
>
> ... and you do that all over the place.
>
> <quote file="FS/svc_acct.pm" line="101">
> return "Unknown pkgnum"
> unless ! $self->pkgnum
> || qsearchs( 'cust_pkg', { 'pkgnum' => $self->pkgnum } );
> </quote>
>
> Huh? "Return 'unknown pkgnum'. Unless the pkgnum method of
> $self instance of the FS::cust_svc class returns false. However, if
> that returns false, then try searching the pkgnum dat-"
Ah, well, TMTOWTDI.
> See what I mean? One of the things that I had to have beat into me
> when I started coding was that, just like in English, you stick the
> most important thing in the sentence at the beginning of the line. I
> still screw it up sometimes and it gives me headaches when I read code
> I wrote when I was sleepy or inebriated. I think this fragment looks
> better as:
>
> if not ( $self->pkgnum() ) {
> qsearchs( 'cust_pkg', { 'pkgnum' => $self->pkgnum } );
> }
> else {
> return "Unknown pkgnum";
> }
Except that it's broken. This is *exactly* why I'm wary of making style
changes for the sake of style changes.
> "If the pkgnum method of the $self instance of the svc_acct class
> returns false, then try searching the database for it. If THAT
> returns false, then and only then do we return the message 'Unknown
> pkgnum'".
>
> However, every time I point this out to LISP progammers, they mutter
> something about FORTH-like syntax taking over the world and RPL being
> a natural way for math geeks to think, which doesn't make any sense to
> me. *I* personally don't care, but I have to worry about the
> efficiency of the people who are going to read this code in the
> future, and I want to make their lives as easy as possible.
>
> Ah, I'm ranting now. Don't worry about it; I'm used to the error
> message coming first by now.
>
> > > I'm personally don't care when people give me patches as long as they
> > > use my tabbing style,
> >
> > I don't care too much one way or the other; I'd probably accept a patch
> > with a different "style" as long as it was clean code. OTOH it's often
> > hard to differentiate signal (code changes) from noise (formatting
> > changes) if someone just reformats for the sake of reformating.
> >
> > > but no one has
> > > been able to figure out what your tabbing style is; you seem to use
> > > many different ones.
> >
> > I do? It seems consistant to me... :(
>
> ... boy did I do something stupid. Well, I read some stuff on the web
> about Perl tabbing, and figured out what's going on: when you extend a
> line over multiple lines, you usually indent it in to the beginning of
> the first token inside the innermost enclosing block. Oops. Other
> than that, everything is two spaces (I did fine one exception at
> cust_pay.pm at line 117, though).
Horrors! An extra space! :)
> > > It's tough to help emacs figure out what to do
> > > to the tab variables when it sees -*- febo-freeside -*-.
> >
> > Eek! You need to use a custom emacs mode? The standard perl or cperl
> > modes don't work? That does seem problematic. :/
>
> After a little tweaking, I think I fixed the standard perl mode to
> make code that looks mostly like yours. Forget I mentioned it.
>
> > The future direction for Freeside trouble ticketing is integration with
> > RT: <http://www.fsck.com/projects/rt/> - or with anything really, although
> > RT is the first target I'd like to make sure the interface/API is
> > well-docuemtned.
>
> Ah, crap. I went and implemented a whole TT database for nothing :(
> This looks perfect. Are you st-
>
> Arrgh! There's a fscking skunk of the front page of fsck.com!
Ferret.
> It
> looks just like that damn skunk my girlfriend has that took a chunk
> out of my left forefinger when I tried to pet it. I couldn't type
> straight for a week solid. Fscking skunks can all-
>
> Right, now this is getting silly. You know, I just realized that this
> email is 304 lines so far, so I'm going to hurry it up and get it over
> with. Besides, it's a weasel, not a skunk. Looks awfully like a
> skunk, though *grumble*
>
> > I'm amenable to including trouble ticketing stuff in the base Freeside
> > source in the short term; in the long term you should plan on moving to RT
> > or maintaining this ticketing system as a separate project (which would
> > interface with the same API as RT, above). Does that makes sense?
>
> Hmm. I have this idea about using my project as a lightweight trouble
> ticket database that people can use if they're not willing to install
> RT, or don't need email, but I think I'll worry about that some other
> time. RT looks like a wonderful solution for the now.
>
> > WRT FS::Record usage, it was always intended as a generally-useful rather
> > than Freeside-specific database interface. Feel free to use it as such;
> > although the name will change when it becomes a CPAN module (Real Soon
> > Now), the functionality should be the same.
>
> Cool.
>
[Text::Template advocacy snipped - hopefully you understand why it's
backwards by now]
--
meow
_ivan
More information about the freeside-devel
mailing list