fisharebest / webtrees

Online genealogy
https://webtrees.net
GNU General Public License v3.0
429 stars 294 forks source link

Privacy settings for GEDCOM exports are not correctly applied to certain facts in HEAD (e.g. SUBM) #4883

Open Jefferson49 opened 10 months ago

Jefferson49 commented 10 months ago

Observed with webtrees 2.1.17

Example A: If a GEDCOM export is started with privacy settings less than manager (e.g. visitor), and the privacy for certain facts in HEAD (e.g. SUBM) is set to " Show to managers", the fact is still exported, e.g. SUBM is included in the GEDCOM export.

Prerequesite to reproduce: Create a new tree

Privacy restrictions (for HEAD:SUBM, "Show to managers"): privacy_restrictions

Export privacy settings ("Visitor"): export preferences

Example B: If a GEDCOM export is started as "Manager" with the custom module DownloadGedcomWithURL, and no "Manager'" is logged in, and the privacy settings for submittors is set to " Show to managers", the fact is not exported, e.g. SUBM is not included in the GEDCOM export. The reason is that - if no manager is logged in - the code in GedcomExportService.php does not apply the "Manager" setting to the export.

While debugging in the webtrees code, I recognized the following background:

Jefferson49 commented 10 months ago

Related pull request #4884 with a proposal for a fix.

fisharebest commented 10 months ago

HEAD:SUBM is a mandatory field. Every valid GEDCOM has at least one SUBM record.

If we don't have one that we can safely include, then we will need to create an empty one.

Jefferson49 commented 10 months ago

O.k., I understand; I did not know that SUBM is mandatory. This explains the behavior in Example A. However, it is kind of confusing that a newly created tree is created with a default privacy setting "All records - Submission - Show to managers". This is kind of contradictionary to the rule that submitters are mandatory in the export.

Example B still does not work correctly, because no SUBM is exported. On the first view, it is very difficult to understand. Debugging shows that after some function calls, the canShow method in GedcomRecord.php is called with the following behavior:

The mentioned code leads to the behavior that always the access level of the logged in user is applied and not the access level requested for the GEDCOM export. If using DownloadGedcomWithURL, there is no guarantee that a user is logged in. If no (Manager) user is logged in, canShowRecord returns false, and SUBM is not exported.

I think the current code contains a hidden assumption that during export from the control panel, there is always a (Manager) user logged in. If using the button in the control panel, this is always the case.

Jefferson49 commented 10 months ago

If SUBM is never ruled out by privacy rules, it might be possible to simply add AUTH::PRIV_HIDE as an access level to the calls of ->facts() in createHeader:

i.e. line 324ff:

if ($header instanceof Header) {
    foreach ($header->facts(['COPR', 'LANG', 'PLAC', 'NOTE'], false, AUTH::PRIV_HIDE) as $fact) {
        $gedcom .= "\n" . $fact->gedcom();
    }

    if ($include_sub) {
        foreach ($header->facts(['SUBM', 'SUBN'], false, AUTH::PRIV_HIDE) as $fact) {
            $gedcom .= "\n" . $fact->gedcom();
        }
    }
}
Jefferson49 commented 9 months ago

After further considerations, I addressed the issue of Example B by overwriting the createHeader method in the DownloadGedcomWithURL custom module.

Jefferson49 commented 9 months ago

HEAD:SUBM is a mandatory field. Every valid GEDCOM has at least one SUBM record. If we don't have one that we can safely include, then we will need to create an empty one.

At the moment, a new tree in webtrees is created without a submitter. The current createHeader method only finds and adds a submitter if it has already been created by the user. As a consequence, a newly created default tree is exported with non standard GEDCOM.

Maybe, it would make sense to generate a default submitter; it could have the same name like the default individual, which is generated for a new tree. If you think this makes sense, I can open a new issue for creating a default submitter.

Jefferson49 commented 9 months ago

One additional poiint, which I identified while working on the topic: For submissions (SUBN), the createHeader method also overrules privacy settings, although SUBN is not mandatory. If the privacy restrictions for submissions are set to "Show to managers", the submission is still exported if "Apply privacy settings" are set to "Visitor" during export.

fisharebest commented 9 months ago

Maybe, it would make sense to generate a default submitter;

I think this is a good idea.

The submitter record could be empty - or it could contain the details of the user who created the tree.

We could also create a submitter record when importing a tree.

We could also prevent the final record from being deleted.

But I can think of one useability issue. Users would now see the submitter list in the menu - since they now have submitters in their tree.

For many users, submitters are fairly pointless. This "pointless" menu item will annoy people. Modules will be created to suppress it ;-)

fisharebest commented 9 months ago

At present, we include all records in the export. Private records have all their details removed.

To remove private records, every other record will need to have links to private records removed,

As far as I can tell, this was done for performance reasons in phpGedView, and we have inherited this logic. I'm not sure the performance arguments still apply.

Jefferson49 commented 9 months ago

For many users, submitters are fairly pointless. This "pointless" menu item will annoy people. Modules will be created to suppress it ;-)

Maybe, GEDCOM 7 has considered this and does not require submitters to be mandatory any more. Submissions have even been removed.

On the other hand, it makes sense to get some information about the author/editor of GEDCOM data.

Jefferson49 commented 9 months ago

From my point of view, the issue can be closed: