Bug 442522

Summary: Review Request: audit-viewer - Audit event viewer
Product: [Fedora] Fedora Reporter: Miloslav Trmač <mitr>
Component: Package ReviewAssignee: Lubomir Rintel <lkundrak>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: fedora-package-review, notting
Target Milestone: ---Flags: lkundrak: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2008-05-03 23:50:42 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:

Description Miloslav Trmač 2008-04-15 10:55:25 UTC
Spec URL: http://people.redhat.com/mitr/packaging/audit-viewer.spec
SRPM URL: http://people.redhat.com/mitr/packaging/audit-viewer-0.2-1.src.rpm
Description: A graphical utility for viewing and summarizing audit events.

Comment 1 Lubomir Kundrak 2008-04-18 13:20:05 UTC
1.) Release: 1

Please include the dist tag
http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-beca3bf84972f19a384cc2e5091ed47c2b3cebc7

2.) You use update-desktop-database and update-mime-database

Please depend on required packages:
Requires(post): desktop-file-utils
Requires(postun): desktop-file-utils

SPEC file seems sane in all other respect. Will continue review once mock build
finishes.

Comment 2 Lubomir Kundrak 2008-04-18 13:20:58 UTC
3.) mock build fails, probably due to lacking dependency on perl(XML::Parser):

checking for XML::Parser... 
configure: error: XML::Parser perl module is required for intltool

Comment 3 Lubomir Kundrak 2008-04-18 13:26:11 UTC
4.) rpmlint:

$ rpmlint -i audit-viewer-0.2-1.src.rpm audit-viewer-0.2-1.x86_64.rpm
audit-viewer-debuginfo-0.2-1.x86_64.rpm
audit-viewer.src: W: no-url-tag
The URL tag is missing.

audit-viewer.x86_64: W: symlink-should-be-relative
/usr/libexec/audit-viewer-server /usr/bin/consolehelper
Absolute symlinks are problematic eg. when working with chroot environments.

audit-viewer.x86_64: W: no-url-tag
The URL tag is missing.

audit-viewer-debuginfo.x86_64: W: no-url-tag
The URL tag is missing.

The URL tag is not a problem for now given what the comment above URL tag says.
However you should correct the absolute symlink. Have you considered using
PolicyKit instread of consolehelper?

Comment 4 Miloslav Trmač 2008-04-18 13:32:26 UTC
(In reply to comment #1)
> 1.) Release: 1
> Please include the dist tag
The linked document states the dist tag is optional.  Is it really necessary?

> 2.) You use update-desktop-database and update-mime-database
> 
> Please depend on required packages:
> Requires(post): desktop-file-utils
> Requires(postun): desktop-file-utils
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets recommends not to
depend on these packages - to allow installing the package in non-GNOME
environments without dragging these packages in.

Comment 5 Lubomir Kundrak 2008-04-18 13:39:57 UTC
(In reply to comment #4)
> (In reply to comment #1)
> > 1.) Release: 1
> > Please include the dist tag
> The linked document states the dist tag is optional.  Is it really necessary?

Common practice is to include it for packages that contains code that is linked
to other packages in the distribution. It is usually ommited in content-only
packages, such as clip art gallery or firmware packages.

Moreover, it simplifies updating the branches with bugfixes, see the paragraph
below.

Given the guidelines do not require the dist tag, this won't block the package
approval. But I encourage you to reconsider its use.

> > 2.) You use update-desktop-database and update-mime-database
> > 
> > Please depend on required packages:
> > Requires(post): desktop-file-utils
> > Requires(postun): desktop-file-utils
> http://fedoraproject.org/wiki/Packaging/ScriptletSnippets recommends not to
> depend on these packages - to allow installing the package in non-GNOME
> environments without dragging these packages in.

Oh, sorry. Ignore that then.

Comment 6 Lubomir Kundrak 2008-04-18 13:42:12 UTC
5.) One more thing:

/usr/share/applications/audit-viewer.desktop: warning: key "Encoding" in group
"Desktop Entry" is deprecated

http://fedoraproject.org/wiki/Packaging/Guidelines#head-d559ee7363418a5840ce63090c608c991cd39ce6
Please consider setting vendor to Fedora

Comment 7 Miloslav Trmač 2008-04-18 21:48:01 UTC
(In reply to comment #3)
> 4.) rpmlint:
> audit-viewer.src: W: no-url-tag
Added, now that audit-viewer is at fedorahosted.org

> audit-viewer.x86_64: W: symlink-should-be-relative
> /usr/libexec/audit-viewer-server /usr/bin/consolehelper
> Absolute symlinks are problematic eg. when working with chroot environments.
There's really no way to use that symlink in a chroot anyway:
- There's no reason to use it to copy the consolehelper binary anywhere.
- If you use the symlink to run audit-viewer-server, then consolehelper
  will access /etc/security/console.apps/audit-viewer-server, check users
  from /etc/passwd and run /usr/libexec/audit-viewer-server-real; this will
  all happen in the "top" root and it will happen the same whether the
  running consolehelper is /usr/bin/consolehelper or
  /chroot/usr/bin/consolehelper.
OTOH using an absolute symlink somewhat protect us in case %{libexecdir} was moved.

> Have you considered using PolicyKit instread of consolehelper?
Briefly.  Right now the privileged part is userhelper and roughly 300 lines of
custom C code, and modifying the code to rely on libpolkit would probably not be
a security improvement (although the PolicyKit features are somewhat compelling).

Comment 8 Miloslav Trmač 2008-04-18 22:03:13 UTC
(In reply to comment #2)
> 3.) mock build fails, probably due to lacking dependency on perl(XML::Parser):
> 
> checking for XML::Parser... 
> configure: error: XML::Parser perl module is required for intltool
Fixed.  Sorry about that - I really should have noticed this myself.

(In reply to comment #6)
> 5.) One more thing:
> 
> /usr/share/applications/audit-viewer.desktop: warning: key "Encoding" in group
> "Desktop Entry" is deprecated
Removed.

>
http://fedoraproject.org/wiki/Packaging/Guidelines#head-d559ee7363418a5840ce63090c608c991cd39ce6
> Please consider setting vendor to Fedora
Fixed.

New package at
http://people.redhat.com/mitr/packaging/audit-viewer-0.2-2.src.rpm ; spec file
updated at the original URL.

Comment 9 Lubomir Kundrak 2008-04-18 22:36:07 UTC
(In reply to comment #7)
> (In reply to comment #3)
> > 4.) rpmlint:
> > audit-viewer.src: W: no-url-tag
> Added, now that audit-viewer is at fedorahosted.org

Not really -- you just added a comment, not changed the URL tag. Given this is
an upstream issue and relation between upstream and package is not relevant
here, it can not block the review.

I encourage you to upload the source to https://fedorahosted.org/releases/ and
change the url tag respectively as described here: https://fedorahosted.org/web/faq

> 
> > audit-viewer.x86_64: W: symlink-should-be-relative
> > /usr/libexec/audit-viewer-server /usr/bin/consolehelper
> > Absolute symlinks are problematic eg. when working with chroot environments.
> There's really no way to use that symlink in a chroot anyway:
> - There's no reason to use it to copy the consolehelper binary anywhere.
> - If you use the symlink to run audit-viewer-server, then consolehelper
>   will access /etc/security/console.apps/audit-viewer-server, check users
>   from /etc/passwd and run /usr/libexec/audit-viewer-server-real; this will
>   all happen in the "top" root and it will happen the same whether the
>   running consolehelper is /usr/bin/consolehelper or
>   /chroot/usr/bin/consolehelper.
> OTOH using an absolute symlink somewhat protect us in case %{libexecdir} was
moved.

I don't believe there's a guideline for these and was not able to find any.

I see no more problems with the package.

APPROVED

Comment 10 Miloslav Trmač 2008-04-18 23:04:35 UTC
(In reply to comment #9)
> I encourage you to upload the source to https://fedorahosted.org/releases/ and
> change the url tag respectively as described here:
https://fedorahosted.org/web/faq
Oh, i didn't know about that.  Thanks for the pointer.


Comment 11 Miloslav Trmač 2008-04-30 13:51:03 UTC
New Package CVS Request
=======================
Package Name: audit-viewer
Short Description: A graphical utility for viewing and summarizing audit events.
Owners: mitr
Branches: F-9
InitialCC: 
Cvsextras Commits: yes


Comment 12 Kevin Fenzi 2008-04-30 16:59:59 UTC
cvs done.

Comment 13 Miloslav Trmač 2008-05-17 12:47:43 UTC
Package Change Request
======================
Package Name: audit-viewer
New Branches: F-8


Comment 14 Kevin Fenzi 2008-05-17 20:32:36 UTC
cvs done.