Bug#495542: RFS: libcas-php
Hi,
[No need to CC me as long as -mentors is]
Olivier Berger wrote:
> Hi.
>
> Thanks for your review. I'll respond bellow.
>
> Le lundi 17 novembre 2008 à 15:20 -0600, Raphael Geissert a écrit :
>> Olivier Berger wrote:
>> [...]
>> >
>> > The corresponding ITP is found at
>> > http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=495542
>> >
>> > Note that this package would allow the dependency of a number of
>> > packages such as moodle and glpi on that package instead of shipping a
>> > copy of phpCAS (http://packages.debian.org/sid/all/moodle/filelist and
>> > http://packages.debian.org/sid/all/glpi/filelist).
>>
>> While you are at it, as your package also uses domxml-php4-php5.php why don't
>> you also package it? (it would be better if the code was finally ported
>> instead of using a wrapper, though).
>>
>
> I can see a couple of reasons why packaging domxml-php4-php5.php may not
> be needed :
> * it's a one file library, so I'm not sure it's really a habit of
> packaging individual files ?
> * it may be included in Debian packages only by those which include a
> copy of phpCAS (but confirmation would be welcome)
> * It will probably be no longer needed in phpCAS when future releases
> will be rewritten (as you suggest) to only support php5, thus using the
> php5 DOM API instead of the old PHP4 one. But that's not something that
> would be done only for Debian, and I'll wait until a decision is taken
> upstream (although I may also again contribute to upstream development
> for that). More details here :
> http://www.ja-sig.org/issues/browse/PHPCAS-25 about a rewrite to get rid
> of that wrapper.
>
> So until usptream phpCAS releases a new version which allows to get rid
> of domxml-php4-php5.php, I'll stick with it in.
Yeah, I forgot that all the matches I found of the dom lib were because of the
embedded copy of phpCAS
>
>> debian/control:
>> > Depends: ${misc:Depends}, php5, php5-curl, php-db
>>
>> Does it really need a web SAPI?
>
> Uh, SAPI ?
SAPI: Server Application Programming Interface
E.g. apache, apache2, cgi, cli, embedded, roxen, etc.
>
>> or can it be used with php5-cli? in the latter
>> case add an ORed dependency on php5-cli in addition to php5.
>>
>
> I have no clue.
> Typical use of CAS is traditionally on web apps, and the protocol
> involves redirections between the CAS server and clients, so I'm not so
> sure a non web use is something interesting.
I saw that, but CAS.php looked like a more general pourpose script, although I
didn't examine it very well.
>
> I suppose most of the use cases would be satisfied as is... unless in
> CGI mode, where php5-cgi would be ORed too ?
No, php5-cgi is already covered by php5.
>>
>> debian/README.Debian:
>> > This packaging needs testing as I'm not fully sure there ain't
>> > regressions introduced by this upgrade of the DOM parsing library.
>>
>> Have you at lest tested the basic functionality of the library with the newer
>> version of domxml-php4-php5.php or are you blindly packaging stuff?
>
> No I have tested it, but you never exactly know for corner cases like
> error messages hard to test.
>
> Btw, my main motivation for packaging it is to use it on a production
> server, so I'd better have it tested of course.
>
> In any case I preferred some explicit mention that it's not exactly
> upstream's version, to let people know who's to blame in case of
> problem.
>
> Will rephrase as :
>
> Differences from upstream version :
> -----------------------------------
>
> This package includes a Debian specific patch, to replace
> domxml-php4-php5.php version 1.5.5 (shipped with version 1.0.1
> of
> phpCAS) by version 1.20a of domxml-php4-to-php5.php, which is
> now
> under LGPL, making it DFSG-free.
>
> Best care was taken by the Debian packager to make sure there
> ain't
> regressions introduced by this upgrade of the DOM parsing
> library. Please report to the Debian BTS if you experience
> problems
> related to that change.
>
Ok, looks better now.
>>
>> CAS.php:
>> > define("CAS_PGT_STORAGE_FILE_DEFAULT_PATH",'/tmp');
>> ..
>> > define("CAS_PGT_STORAGE_FILE_FORMAT_PLAIN",'plain');
>>
>> Doesn't look good at all.
>
> Hmmm... I guess that needs to be fixed indeed. Thanks for spotting that.
>
>>
>> CAS/client.php:
>> > if ( $this->isProxy() ) {
>> > // pass the callback url for CAS proxies
>> > $validate_url .=
>> > '&pgtUrl='.$this->getCallbackURL();
>> > }
>>
>> Improper sanitation of getCallbackURL which uses data like REQUEST_URI; and
>> there's more about this, but I'm better going to test it and send the info to
>> bugtraq.
>
> Feel free to notify me and/or phpcas-devel@esup-portail.org please (or have a
> look at http://www.ja-sig.org/wiki/display/CASC/phpCAS+bug+reports .
>
>>
>> What about also shipping the api docs?
>>
>
> Well maybe in a separate -doc package, then ?
IIRC it was pretty small, so there's no need, IMHO, to ship it in another
package.
>
>> >
>> > I would be glad if someone uploaded this package for me.
>> >
>> > Kind regards
>> > Olivier Berger
>>
>> Cheers,
>> - --
>> Raphael Geissert - Debian Maintainer
>> www.debian.org - get.debian.net
>
> Thanks alot for the detailed review.
>
> Regards,
Cheers,
--
Raphael Geissert - Debian Maintainer
www.debian.org - get.debian.net
Reply to: