Bug 603346

Summary: Review Request: php-voms-admin - Web based interface to control VOMS parameters written in PHP
Product: [Fedora] Fedora Reporter: Mattias Ellert <mattias.ellert>
Component: Package ReviewAssignee: Lev Shamardin <shamardin>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, manf, notting, shamardin
Target Milestone: ---Flags: shamardin: fedora‑review+
limburgher: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: php-voms-admin-0.6-1.el5 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2011-07-08 14:05:21 EDT Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:

Description Mattias Ellert 2010-06-12 10:05:44 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

Description: 
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.
Comment 1 Jason Tibbitts 2010-12-03 15:16:58 EST
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).
Comment 2 Mattias Ellert 2011-05-15 08:34:58 EDT
(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:

SPEC: http://www.grid.tsl.uu.se/review/php-voms-admin.spec
SRPM: http://www.grid.tsl.uu.se/review/php-voms-admin-0.5-1.fc14.src.rpm
Comment 3 Lev Shamardin 2011-06-04 16:34:03 EDT
1. /etc/httpd/conf.d/pva.conf contains %DATADIR% macros inside, must be fixed.

2. I think you should either add 

   Requires: voms-server

   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 items:

- 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
        actual license.
+ 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 items:

+ 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.
Comment 4 Andrii Salnikov 2011-06-06 10:50:22 EDT
(In reply to comment #3)
Hello, Lev!

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:

http://grid.org.ua/development/pva/packages/rpms/php-voms-admin-0.5.1.spec
http://grid.org.ua/development/pva/packages/rpms/php-voms-admin-0.5.1-1.fc14.src.rpm

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.

Regards,
Andrii
Comment 5 Lev Shamardin 2011-06-06 16:52:24 EDT
Hi Andrii,

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.
Comment 6 Andrii Salnikov 2011-06-07 04:08:30 EDT
(In reply to comment #5)
Hi Lev,

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.
Comment 7 Mattias Ellert 2011-06-08 09:29:22 EDT
Thanks Andrii for addressing the issues raised by the reviewer. I have updated the package with your latest changes:

SPEC: http://www.grid.tsl.uu.se/review/php-voms-admin.spec
SRPM: http://www.grid.tsl.uu.se/review/php-voms-admin-0.5.1-1.fc14.src.rpm
Comment 8 Lev Shamardin 2011-06-10 14:27:54 EDT
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.

Setting fedora-review+. 

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.
Comment 9 Lev Shamardin 2011-06-10 14:47:46 EDT
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

<Directory/usr/share/pva/ >

...

  <IfModule mod_ssl.c>
    SSLRequireSSL
    SSLVerifyClient require
    SSLVerifyDepth 10 # or other appropriate number
  </IfModule>
  <IfModule mod_gnutls.c>
    GnuTLSClientVerify require
  </IfModule>
  <IfModule mod_nss.c>
    SSLVerifyDepth 10 # not sure about this directive
  </IfModule>

...
</Directory>
Comment 10 Mattias Ellert 2011-06-10 15:42:51 EDT
(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
Owners: ellert
Branches: f14 f15 el5 el6
InitialCC:
Comment 11 Andrii Salnikov 2011-06-11 06:22:27 EDT
Hi Lev, 
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.

Regards,
Andrii
Comment 12 Jon Ciesla 2011-06-13 08:17:27 EDT
Git done (by process-git-requests).
Comment 13 Fedora Update System 2011-06-18 02:36:27 EDT
php-voms-admin-0.5.1-2.fc14 has been submitted as an update for Fedora 14.
https://admin.fedoraproject.org/updates/php-voms-admin-0.5.1-2.fc14
Comment 14 Fedora Update System 2011-06-18 02:36:42 EDT
php-voms-admin-0.5.1-2.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/php-voms-admin-0.5.1-2.el6
Comment 15 Fedora Update System 2011-06-18 02:36:49 EDT
php-voms-admin-0.5.1-2.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/php-voms-admin-0.5.1-2.el5
Comment 16 Fedora Update System 2011-06-18 02:36:57 EDT
php-voms-admin-0.5.1-2.fc15 has been submitted as an update for Fedora 15.
https://admin.fedoraproject.org/updates/php-voms-admin-0.5.1-2.fc15
Comment 17 Fedora Update System 2011-06-21 13:12:14 EDT
php-voms-admin-0.5.1-2.el6 has been pushed to the Fedora EPEL 6 testing repository.
Comment 18 Fedora Update System 2011-07-08 14:05:11 EDT
php-voms-admin-0.6-1.fc14 has been pushed to the Fedora 14 stable repository.
Comment 19 Fedora Update System 2011-07-08 14:12:06 EDT
php-voms-admin-0.6-1.fc15 has been pushed to the Fedora 15 stable repository.
Comment 20 Fedora Update System 2011-07-14 20:00:34 EDT
php-voms-admin-0.6-1.el6 has been pushed to the Fedora EPEL 6 stable repository.
Comment 21 Fedora Update System 2011-07-14 20:03:27 EDT
php-voms-admin-0.6-1.el5 has been pushed to the Fedora EPEL 5 stable repository.