Repository navigation
Added a private 'package' media type for zip and 7z files that only Site Administrator and Content API service can download. - #350
Merged
Conversation
…adRolesWithPermission()'.
… 'access package media' and to keep private file responses uncached.
…e file system for Site Administrator and Content API service.
…oad routes while keeping their global providers.
…and API key downloads.
…ccessTest' and renamed the injected account in 'PackageAccessHook'.
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #350 +/- ##
===========================================
- Coverage 87.23% 85.54% -1.69%
===========================================
Files 29 33 +4
Lines 658 761 +103
===========================================
+ Hits 574 651 +77
- Misses 84 110 +26 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ries for accounts without 'access package media'.
…uses them, instead of leaving them on disk.
…deleting a package together with its file.
This comment has been minimized.
This comment has been minimized.
…geFileTest::testOtherBundleUnaffected()'.
|
Code coverage (threshold: 80%) Per-class coverage |
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A new
packagemedia type storeszipand7zfiles infield_do_m_packageunderprivate://packages/[date:custom:Y]-[date:custom:m], and the newdo_packagemodule makes every operation on it - view, download, create, edit, delete, the media listings and the/admin/content/filesoverview - depend on theaccess package mediapermission, which only thecivictheme_site_administratoranddo_content_apiroles hold. Deleting a package deletes its files on the spot (thefile_managedrow, the usage rows and the file on disk), and so does replacing its file without a new revision or deleting a revision that held the only reference.A private-scheme file field doesn't restrict anything by itself on this site: core's
FileAccessControlHandlergrants a private file to whoever canviewthe media that references it, and media view is the single site-wideview mediapermission, which the anonymous and authenticated roles both hold, so a logged-out visitor would have been able to download any package. The editorial roles also hold the cross-bundleupdate any media,delete any mediaandcreate media. 2 more gaps sat on the download routes:key_authisn't a global authentication provider andsystem.filesandsystem.private_file_downloaddeclare no_authoption, so the content API key never authenticated on a download URL, and an anonymous 403 there went out asCache-Control: max-age=900, public, which Cloudflare caches for.zipand.7zby extension - 1 logged-out request (a chat app unfurling a pasted link, say) could leave a 403 at the edge for authorised users to receive. Deleting the media never removed the file either:file.settings:make_unused_managed_files_temporaryisfalsehere, so core leaves an unused file permanent and cron's garbage collection never touches it, and core releases usage only for the default revision's file, so a zip replaced in a new revision kept a usage row pointing at a media item that no longer exists. Meanwhile/admin/content/fileslisted every package file name to content authors and approvers, who holdaccess files overview.After merge,
/system/files/packages/...answers 200 to Site Administrators and to the Content API service (with itsapi-keyheader) and 403 to content authors, approvers, other authenticated users and anonymous visitors; the anonymous 403 carriesCache-Control: must-revalidate, no-cache, private, the on-disk path under/sites/default/files/private/is a 404, and/jsonapi/media/packagereturns an emptydatafor anyone without the permission. Deleting a package through its delete form, the bulkDelete mediaaction, JSON:API or code removes every file its revisions held unless another entity still uses it, after which the download URL is a 404, and/admin/content/filesshows package files only to roles with the permission. Nothing else moves: no theme or front-end change, media of every other type keeps core's delete behaviour (its file stays on disk), no documentation changes,standalone_urlstays off, and the built-inadministratorrole and user 1 keep access by definition.Before / After
Checklist before requesting a review
[#123] Verb in past tense.#123added to descriptionChangedsectionChanged
web/modules/custom/do_package/src/Hook/PackageAccessHook.php-mediaAccess()andmediaCreateAccess()forbid every operation on apackagemedia, including its creation, for accounts withoutaccess package media, and return neutral for everything else. The hook never returns allowed, so core's own operation checks still decide for accounts holding the permission: the Content API service can view published packages and create them through its existingcreate media, but can't edit or delete them, while Site Administrators manage everything. Unpublishing a package withdraws it from the API.queryMediaAccessAlter()(hook_query_media_access_alter) - addsbundle <> 'package'on the basemediaormedia_field_datatable for accounts without the permission, so JSON:API collections return an emptydatawith noomittedUUIDs, and the media overview and media library show no packages to authors and approvers.queryFileAccessAlter()(hook_query_file_access_alter) - addsuri NOT LIKE 'private://packages/%'on the basefile_managedtable for the same accounts, so the Files overview, its usage page and JSON:API file collections skip package files.restrictedBaseTableAlias()holds the account and base table checks both alters share, andPackageConfigTest::testFileDirectory()pins the field's upload directory underPackageAccessHook::DIRECTORY.web/modules/custom/do_package/src/Hook/PackageFileHook.php-hook_media_predeletecollects the file of every revision of a package before core drops the revision rows, thenhook_media_delete,hook_media_updateandhook_media_revision_deletedelete each file that no package revision references and no other entity records usage of. Saving with a new revision keeps the previous file, because the older revision still references it and a revert needs it. Entity hooks rather than a form alter cover the bulk action, JSON:API and code as well as the delete form, with no file permission needed beyond the package's own delete access.web/modules/custom/do_package/src/Routing/RouteSubscriber.php- setsno_cacheonsystem.filesandsystem.private_file_download. Core'sDenyNoCacheRoutesresponse policy then sends every response on those 2 routes asmust-revalidate, no-cache, private, and the page cache never stores any of them. A denied download otherwise carries the public pagemax-age, so a cache keyed on the URL alone would serve that 403 to allowed accounts.web/modules/custom/do_content_api/src/Routing/RouteSubscriber.phpanddo_content_api.services.yml- the subscriber now coverssubrequests.front-controller,system.filesandsystem.private_file_download, and takesauthentication_collectoras an argument. A route with no_authoption is seeded with the global providers beforekey_authis added, so Site Administrators keep cookie access to private files while the API key authenticates there too.config/default/- thepackagemedia type, itsfield_do_m_packagestorage (uri_scheme: private) and field (required, directorypackages/[date:custom:Y]-[date:custom:m], extensionszip 7z), the default andmedia_libraryform and view displays, anddo_packageincore.extension.yml.media.type.packagedeclares an enforced dependency ondo_package, so uninstalling the module removes the type instead of leaving packages readable by everyone holdingview media.packageisn't added to thecivictheme_editorialworkflow, because a moderated package would start as a draft the Content API service can't view. The config was built on a running site with a throwaway script and exported withdrush cex.config/default/user.role.civictheme_site_administrator.ymlandconfig/default/user.role.do_content_api.yml- Site Administrator receivesaccess package mediaplus the per-bundlecreate package media,edit any package media,edit own package media,delete any package mediaanddelete own package mediait holds for every other media type; Content API service receivesaccess package mediaonly.web/modules/custom/do_base/tests/src/Traits/ExportedConfigTrait.php-loadRolesWithPermission()now holds the role scan thatPreviewLinkConfigTesthad inline, including the rule that a role flaggedis_admincounts as granted, andPackageConfigTestreuses it to pin which roles hold each of the 4 package permissions.PackageAccessHookTest(including the file query alter),PackageConfigTest, thedo_packageanddo_content_apiRouteSubscriberTestclasses andServiceRolePermissionsTest; kernel:PackageAccessTestruns a real private file through core's file download access and access-checked file queries for Site Administrator, Content API service, content author and anonymous, andPackageFileTestcovers deleting a package with 2 revisions, a file still used by another package or module, replacing a file with and without a new revision, revision deletes, and adocumentbundle whose files survive an update, a revision delete and a delete; both extend the newPackageKernelTestBase. Behat:tests/behat/features/package_media.featurewith thepackage.zipandpackage.7zfixtures.do_packagekernel group passes (38 cases) andpackage_media.featurepasses (20 scenarios) through nginx - the download matrix (anonymous, authenticated, author and approver get 403; Site Administrator and Content API service get 200), an API key download (200), the on-disk path/sites/default/files/private/...(404), an empty anonymous JSON:API collection,.zipand.7zuploads accepted while.jpgis rejected,/admin/content/fileshiding package files from authors and approvers, and a package deleted from its delete form leaving no Files entry and a 404 download URL. A manual check returned200 application/zipwithCache-Control: privatefor the API key download, andCache-Control: must-revalidate, no-cache, privatefor the anonymous 403.Contrib modules considered before writing the gate:
media_private_accessprivate_files_download_permission3.1.3field_permissions1.5file_entity_permsfile_entitypatch, no security coveragemedia_entity_download2.4/media/{id}/downloadroute with no access model of its ownContrib modules considered for deleting the file together with the package:
media_file_delete1.3.2$file->delete(). Its 2 settings are site-wide, so making it automatic changes every media type; it swaps form classes only, so JSON:API and programmatic deletes leave the file; it reads only the current revision's file; and it needs core'sdelete any filefor a file another user uploadedfile_delete3.0.3fancy_file_delete2.0.11auditfiles4.2.4Screenshots
Summary by CodeRabbit