Red Hat Bugzilla – Bug 603346
Review Request: php-voms-admin - Web based interface to control VOMS parameters written in PHP
Last modified: 2011-07-14 20:03:32 EDT
Spec URL: http://www.grid.tsl.uu.se/review/php-voms-admin-spec
SRPM URL: http://www.grid.tsl.uu.se/review/php-voms-admin-0.5-0.el5.svn18160.src.rpm
PHP VOMS-Admin (PVA) originally implemented the same functions as the
traditional JAVA-based VOMS-Admin (v.2.0.18) interface for Apache
Tomcat. It was designed to be more flexible and stable, provide easy
scalability and minimize resource usage. PVA is fully compatible with
the vomsd mysql backend.
For an example of a server running php-voms-admin installed from this rpm, see http://grid.tsl.uu.se/pva/ and https://grid.tsl.uu.se/pva/ for the unauthenticated and authenticated interface respectively.
The spec URL above is invalid.
Version 0.5 seems to have been released; is there any reason to continue to package a checkout? Also, you have the dist tag in the wrong place in the Release:; it goes at the end (except in one limited case which does not apply here).
(In reply to comment #1)
> The spec URL above is invalid.
Typo - the last dash should of course have been a full stop.
> Version 0.5 seems to have been released
Updated to the released version:
1. /etc/httpd/conf.d/pva.conf contains %DATADIR% macros inside, must be fixed.
2. I think you should either add
or consider adding README.Fedora stating that voms-server package
(possibly on some other host) is required for using this software.
3. Looks like /usr/share/pva/.htaccess is redundant, since pva.conf
already contains these rewrites.
4. After first fix mentioned above, the package does NOT work on my
Fedora 14 x86_64 box, with these errors in ssl_error_log:
[Sun Jun 05 00:30:26 2011] [error] [client 127.0.0.1] PHP Notice: Undefined variable: voms_str_unauth in /usr/share/pva/index.php on line 58
[Sun Jun 05 00:30:26 2011] [error] [client 127.0.0.1] PHP Notice: Undefined variable: voms_title in /usr/share/pva/index.php on line 90
[Sun Jun 05 00:30:26 2011] [error] [client 127.0.0.1] PHP Notice: Undefined variable: regex_digits in /usr/share/pva/index.php on line 112
[Sun Jun 05 00:30:26 2011] [error] [client 127.0.0.1] PHP Warning: preg_match(): Empty regular expression in /usr/share/pva/index.php on line 112
5. As the added bonus, would you consider upgrading to a newer SVN
version, or at least include lang/ru.inc into the package. (I will
not require this, but it would be very nice.)
Now, the formal part:
- MUST: rpmlint errors:
php-voms-admin.noarch: E: htaccess-file /usr/share/pva/.htaccess
+ MUST: package is named according to guidelines.
+ MUST: spec file name is correct.
+ MUST: The package must meet the Packaging Guidelines .
+ MUST: package is licensed under ASL 2.0
+ MUST: The License field in the package spec file does match the
+ MUST: The license text file is included into the source and the package.
+ MUST: The spec file must is written in American English.
+ MUST: The spec file for the package is legible.
+ MUST: Provided sources match the upstream SVN.
+ MUST: The package builds on noarch.
+ MUST: Package does not need any Build dependencies.
+ MUST: Source package does not use gettext locales.
+ MUST: Package does not contain shared libraries.
+ MUST: Package does not bundle copies of system libraries.
+ MUST: Package is not relocatable.
+ MUST: Package owns all directories it creates.
+ MUST: %files listings are fine.
+ MUST: Permissions on files are correct.
+ MUST: Package consistently uses macros.
+ MUST: The package contains code, or permissable content.
+ MUST: Package does not contain large documentation.
+ MUST: Package %doc does not affect the runtime.
+ MUST: Package does not own files or directories owned by other packages.
+ MUST: All filenames in rpm package are valid UTF-8.
+ SHOULD: Source package includes the license text.
+ SHOULD: Package builds in mock.
- SHOULD: At the moment the package functionality seems to be broken,
but this seems to be an upstream issue. Some svn revisions
ago it did work.
- SHOULD: Manual page for /usr/sbin/pva-addvo is missing in upstream.
(In reply to comment #3)
I have made required changes to stable branch and released 0.5.1 to fix drawbacks.
1. %DATADIR% problems occurs in code revision in subject here provided by Mattias due to old spec file that was not updated from SVN.
Spec file from SVN tagged version does not have this issue.
Here you can find latest src.rpm and spec file for 0.5.1:
2. I have added README.Fedora to 0.5.1, because voms-server in not required for PVA operation.
PVA works even without it, but if you want to employ credentials signing you must have voms-server installed.
3. outdated spec file problem
4. outdated spec file problem
5. Upstream 0.6 language files contain several changes, that are incompatible with 0.5 (menu entries especially). Russian translation by Denis Patrikeev was provided for upstream 0.6 and cannot be easily added to 0.5 distribution. This summer I plan to release stable 0.6 and russian translation will be a part of it.
> - SHOULD: Manual page for /usr/sbin/pva-addvo is missing in upstream.
Manual page for pva-addvo has been included in 0.5.1 release.
First of all, thanks for a prompt reply.
I have tested your spec with a checkout of pva-0.5.1 tag, the errors I mentioned in the comment above are still relevant for this version. I suppose, something is broken with php includes, since I can see the sources of the regex.php and langdetect.php in the .../pva/ page source. I'm not a php expert, but I suspect that ini_set directives should be avoided in the production code, and should be moved out to the configuration (probably to /etc/httpd/conf.d/pva.conf).
Second, there is one critical thing missing in your version of the spec-file, namely instructions how to get the sources from the upstream version control system (see http://fedoraproject.org/wiki/Packaging/SourceURL).
And the last question, do you have a FAS account, and, if you do, are you in the 'packager' group? If not, either Mattias should maintain and submit the spec for this package, or you should follow the sponsorship procedure described in http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group.
(In reply to comment #5)
I have forgot about fixes made in upstream for PHP 5.3.3 included into Fedora 14 distribution. When you mentioned ‘sources visibility’ I realized what the problem is.
Now I have finished with PHP 5.3.3 fixes for 0.5.1, update an SVN tagged version and src.rpm mentioned in previous post. You can find updated version following the same URLs.
> And the last question, do you have a FAS account, and, if you do, are you in
> the 'packager' group? If not, either Mattias should maintain and submit the
> spec for this package, or you should follow the sponsorship procedure described
> in http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group.
No, I have not FAS account. My packages, that seems to work for me, was posted for you on bug request. If you confirm that now SVN version is woks on you Fedora 14, I will ask Mattias to make an official commit after his final spec corrections.
Thanks Andrii for addressing the issues raised by the reviewer. I have updated the package with your latest changes:
The package now works correctly and the review issues are fixed.
Review items which were not ok, but are fixed now:
+ MUST: rpmlint errors: no errors, only spelling warnings.
+ SHOULD: Package core functionality appears to work in most cases.
+ SHOULD: Package contains manual pages for binaries/scripts.
Please consider providing a package also for the EPEL tree. I could be a co-maintainer if you wish.
There are some future points which could be done better:
1. The package could provide a SELinux module/subpackage, conforming to https://fedoraproject.org/wiki/SELinux_Policy_Modules_Packaging_Draft. Under SELinux it needs two things to operate properly:
a) setsebool -P httpd_can_sendmail 1
b) proper context on /var/www/pva/mail-copies, setroubleshootd suggests
semanage fcontext -a -t httpd_sys_rw_content_t '/var/www/pva/mail-copies'
restorecon -v '/var/www/pva/mail-copies'
2. Upstream provides no documentation on package configuration. Consider adding notices about editing /etc/pva/addvo.conf and /etc/pva/config.inc after installation into README.Fedora.
I have also found an upstream issue, but I haven't found the appropriate bug tracker, so filing it here:
pva-addvo fails to add a new vo, if there is more than one copy (or symlink) of VO administrator's CA in /etc/grid-security/certificates: this causes an incorrect CAID inside the pva-addvo, which can not only point at a wrong CA, but also to a missing CA id.
And almost forgot another missing documentation/packaing(?) minor, but still important point: this package is useless without SSL with certificates peer authentication, so README.Fedora should include
1. Notice about this requirement.
2. Example configuration for mod_ssl.
Also probably it would be a good idea to include a universal configuration snippet into /etc/http/conf.d/pva.conf, something like
SSLVerifyDepth 10 # or other appropriate number
SSLVerifyDepth 10 # not sure about this directive
(In reply to comment #8)
> The package now works correctly and the review issues are fixed.
> Setting fedora-review+.
Thank you for the review. I'll investigate the SELinux issues.
> I have also found an upstream issue, but I haven't found the appropriate bug
> tracker, so filing it here:
Upstream bug tracker is http://bugzilla.nordugrid.org/
→ Product: Contributed software
→ Component: pva
New Package SCM Request
Package Name: php-voms-admin
Short Description: Web based interface to control VOMS parameters written in PHP
Branches: f14 f15 el5 el6
First of all thanks a lot for deep review and posting hints for PHP VOMS-Admin enhancements.
As for documentation, now I am working on operation manual for 0.6 that already includes configuration description. I need a few weeks to finish it along with 0.6 stable (fixing some minor issues, while writing the documentation). But anyway I think this is a good idea to write manual pages for configuration files, so I’ll implement this too in 0.6.
Link to the upstream bug tracker is available on project web-site and already provided by Mattias. Mentioning the bug tracker in README directly will be also a good idea. I will appreciate if you report any found bugs to bugzilla, so I’ll be able to improve the software.
pva-addvo bug that you described seems to be already reported and fixed in 0.5. I have also made a test with grid-security/certificates directory containing 2 different hash symlinks for one CA (IGTF new-style distribution), and haven’t encountered any problems. Could you point on how to reproduce the bug?
About the SSL notice, I have also put it to documentation for 0.6 but I agree it must be mentioned in README directly. Universal configuration is also a good idea, thank you for hinting. But I think optional VerifyClient will be more useful, because PVA handles unauthenticated operations and allows to view vomses and mkgridmap configuration for a VO without authentication and maybe other things if ACL rules are configured accordingly. A choice to require client certificate is available on demand of site administrator that want to enforce more security restrictions.
Git done (by process-git-requests).
php-voms-admin-0.5.1-2.fc14 has been submitted as an update for Fedora 14.
php-voms-admin-0.5.1-2.el6 has been submitted as an update for Fedora EPEL 6.
php-voms-admin-0.5.1-2.el5 has been submitted as an update for Fedora EPEL 5.
php-voms-admin-0.5.1-2.fc15 has been submitted as an update for Fedora 15.
php-voms-admin-0.5.1-2.el6 has been pushed to the Fedora EPEL 6 testing repository.
php-voms-admin-0.6-1.fc14 has been pushed to the Fedora 14 stable repository.
php-voms-admin-0.6-1.fc15 has been pushed to the Fedora 15 stable repository.
php-voms-admin-0.6-1.el6 has been pushed to the Fedora EPEL 6 stable repository.
php-voms-admin-0.6-1.el5 has been pushed to the Fedora EPEL 5 stable repository.