Added 'view' permissions for media folders. #15173

gvkries · 2024-01-26T15:30:23Z

This pull request adds a "Secure Media" feature to the Media module, enhancing security and control over media files. The key features include:

Restricted Access to Media Folders: A view permission is created for the root media folder and for each first-level folder within the media root, allowing administrators to restrict access based on user roles.
Enhanced Viewing Permissions: Introduces permissions to view one's own media files and/or those of others, expanding upon the existing ManageOwnMedia permission.
Consistent Access Rules for Media and Content Items: Media attached to content items will adhere to the ViewContent permission of the respective content item. This alignment ensures consistent access rules between media and content items.
Protection for Temporary Attached Media Files: Secures temporary attached media files in a manner similar to personal user files.
Improved Management Permissions in Admin: Refines the manage media permissions to allow media management only when viewing permissions are also granted. This prevents users from managing media they cannot view. Additionally, the creation and deletion buttons in the admin interface are disabled for folders that are not accessible post-creation or for special folders like "_Users" and "mediafields".
Handling Unauthorized Access: Introduces a middleware that returns a 404 NotFound response for unauthenticated access attempts to secured media files. This approach not only restricts access but also conceals the existence of the file.
Configurable Cache-Control for Secured Files: Sets the Cache-Control header of secured files to no-store by default, preventing their caching. This setting is configurable to suit different needs.
Bearer Token Authentication for API Access: Enables bearer token authentication for media files, aligning with OrchardCore's API capabilities. This feature is particularly useful for headless CMS scenarios and external application integrations.

This implementation addresses most ideas outlined in issue #9369. It focuses on introducing simple view permissions for media files, avoiding an excessive number of permissions in the interface. This approach also resolves issues like #12057 by enabling folder access restrictions for users without viewing permissions.

Fixes #3590, fixes #9369, fixes #12057.

github-actions · 2024-02-02T21:27:47Z

This pull request has merge conflicts. Please resolve those before requesting a review.

hishamco · 2024-02-02T22:44:07Z

Please fix the merge conflict

github-actions · 2024-02-09T04:23:09Z

This pull request has merge conflicts. Please resolve those before requesting a review.

hishamco · 2024-02-09T15:28:44Z

Again we have a merge conflict :(

Piedone

Awesome! This is a pretty old feature request: #3590. Also see #6027 (comment) for some implementation ideas.

When addressing review feedback, please adhere to the following:

Apply code suggestions directly so the reviewer doesn't have to eyeball the changes. These resolve themselves after applying them, that's fine.
Don't resolve other conversations so it's easier to track for the reviewer. Then, the reviewer will resolve them.
Feel free to mark conversations that you addressed to keep track of them with an emoji or otherwise, just don't resolve them.
Please keep conversations happening in line comment in those convos, otherwise communication will be a mess. If you have trouble finding them, see this video.
Please click "Re-request review" in the top-right corner for each reviewer when you're ready for another round of review, so they know that you're done.

src/OrchardCore/OrchardCore.Media.Abstractions/MediaOptions.cs

src/OrchardCore.Modules/OrchardCore.Media/Manifest.cs

src/OrchardCore.Modules/OrchardCore.Media/Processing/MediaImageSharpConfiguration.cs

src/OrchardCore.Modules/OrchardCore.Media/Services/MediaOptionsConfiguration.cs

src/OrchardCore.Modules/OrchardCore.Media/Services/ViewMediaFolderAuthorizationHandler.cs

...Core.Tests/Modules/OrchardCore.Media/SecureMedia/ViewMediaFolderAuthorizationHandlerTests.cs

src/OrchardCore.Modules/OrchardCore.Media/Services/ViewMediaFolderAuthorizationHandler.cs

…eSharpConfiguration.cs Co-authored-by: Zoltán Lehóczky <[email protected]>

…sConfiguration.cs Co-authored-by: Zoltán Lehóczky <[email protected]>

Co-authored-by: Zoltán Lehóczky <[email protected]>

…ns.cs Co-authored-by: Zoltán Lehóczky <[email protected]>

…troller.cs Co-authored-by: Zoltán Lehóczky <[email protected]>

…lderAuthorizationHandler.cs Co-authored-by: Zoltán Lehóczky <[email protected]>

Piedone

Great! When this is merged, with 3 issues closing, this will be a combo breaker :). I've asked for feedback under the issues; if nothing else happens, I'll merge this in a week.

github-actions · 2024-04-18T17:20:39Z

This pull request has merge conflicts. Please resolve those before requesting a review.

hishamco · 2024-04-18T23:51:42Z

Please merge the conflict and I will do a quick review, then we can merge ASAP before another merge conflict shows up

Piedone · 2024-04-19T00:07:30Z

You just need to rebuild the frontend assets, which is now a more reliable and automatically verified process: #15735. But it's still just a temporary band-aid before we have a proper development story for assets, see #15740.

I'll merge this on the 25th, but not before that, waiting for possible feedback under the linked issues. This is quite a large change that many people awaited, so I want to give them a chance to look at it before merge.

src/OrchardCore.Modules/OrchardCore.Media/Controllers/AdminController.cs

hishamco · 2024-04-19T22:04:37Z

src/OrchardCore.Modules/OrchardCore.Media/SecureMediaPermissions.cs

+        // Note: The ManageMediaFolder permission grants all access, so viewing must be implied by it too.
+        public static readonly Permission ViewMedia = new("ViewMediaContent", "View media content in all folders", new[] { Permissions.ManageMediaFolder });
+        public static readonly Permission ViewRootMedia = new("ViewRootMediaContent", "View media content in the root folder", new[] { ViewMedia });
+        public static readonly Permission ViewOthersMedia = new("ViewOthersMediaContent", "View others media content", new[] { Permissions.ManageMediaFolder });
+        public static readonly Permission ViewOwnMedia = new("ViewOwnMediaContent", "View own media content", new[] { ViewOthersMedia });
+
+        private static readonly Permission _viewMediaTemplate = new("ViewMediaContent_{0}", "View media content in folder '{0}'", new[] { ViewMedia });
+
+        private static Dictionary<ValueTuple<string, string>, Permission> _permissionsByFolder = new();
+        private static readonly char[] _trimSecurePathChars = ['/', '\\', ' '];
+        public static readonly ReadOnlyDictionary<string, Permission> PermissionTemplates = new(new Dictionary<string, Permission>()
+        {
+            { ViewMedia.Name, _viewMediaTemplate },
+        });


Suggested change

// Note: The ManageMediaFolder permission grants all access, so viewing must be implied by it too.

public static readonly Permission ViewMedia = new("ViewMediaContent", "View media content in all folders", new[] { Permissions.ManageMediaFolder });

public static readonly Permission ViewRootMedia = new("ViewRootMediaContent", "View media content in the root folder", new[] { ViewMedia });

public static readonly Permission ViewOthersMedia = new("ViewOthersMediaContent", "View others media content", new[] { Permissions.ManageMediaFolder });

public static readonly Permission ViewOwnMedia = new("ViewOwnMediaContent", "View own media content", new[] { ViewOthersMedia });

private static readonly Permission _viewMediaTemplate = new("ViewMediaContent_{0}", "View media content in folder '{0}'", new[] { ViewMedia });

private static Dictionary<ValueTuple<string, string>, Permission> _permissionsByFolder = new();

private static readonly char[] _trimSecurePathChars = ['/', '\\', ' '];

public static readonly ReadOnlyDictionary<string, Permission> PermissionTemplates = new(new Dictionary<string, Permission>()

{

{ ViewMedia.Name, _viewMediaTemplate },

});

// Note: The ManageMediaFolder permission grants all access, so viewing must be implied by it too.

public static readonly Permission ViewMedia = new("ViewMediaContent", "View media content in all folders", new[] { Permissions.ManageMediaFolder });

public static readonly Permission ViewRootMedia = new("ViewRootMediaContent", "View media content in the root folder", new[] { ViewMedia });

public static readonly Permission ViewOthersMedia = new("ViewOthersMediaContent", "View others media content", new[] { Permissions.ManageMediaFolder });

public static readonly Permission ViewOwnMedia = new("ViewOwnMediaContent", "View own media content", new[] { ViewOthersMedia });

public static readonly ReadOnlyDictionary<string, Permission> PermissionTemplates = new(new Dictionary<string, Permission>()

{

{ ViewMedia.Name, _viewMediaTemplate },

});

private static readonly Permission _viewMediaTemplate = new("ViewMediaContent_{0}", "View media content in folder '{0}'", new[] { ViewMedia });

private static readonly char[] _trimSecurePathChars = ['/', '\\', ' '];

private static Dictionary<ValueTuple<string, string>, Permission> _permissionsByFolder = new();

Not possible due to initialization order...

I will check

src/OrchardCore.Modules/OrchardCore.Media/SecureMediaPermissions.cs

src/OrchardCore.Modules/OrchardCore.Media/Services/AttachedMediaFieldFileService.cs

src/OrchardCore.Modules/OrchardCore.Media/Services/MediaOptionsConfiguration.cs

src/OrchardCore.Modules/OrchardCore.Media/Services/ViewMediaFolderAuthorizationHandler.cs

src/OrchardCore.Modules/OrchardCore.Media/ViewModels/MediaApplicationViewModel.cs

src/OrchardCore.Modules/OrchardCore.Media/ViewModels/MediaFolderViewModel.cs

src/OrchardCore/OrchardCore.Media.Core/Events/MediaEventHandlerBase.cs

src/docs/reference/modules/Media/README.md

Co-authored-by: Hisham Bin Ateya <[email protected]>

hishamco · 2024-04-21T21:45:28Z

src/OrchardCore.Modules/OrchardCore.Media/SecureMediaPermissions.cs

+        public static readonly Permission ViewOthersMedia = new("ViewOthersMediaContent", "View others media content", new[] { Permissions.ManageMediaFolder });
+        public static readonly Permission ViewOwnMedia = new("ViewOwnMediaContent", "View own media content", new[] { ViewOthersMedia });
+
+        private static readonly Permission _viewMediaTemplate = new("ViewMediaContent_{0}", "View media content in folder '{0}'", new[] { ViewMedia });


I think you need to make this public like the others, that's why the order is matter :)

I don't think so. One can argue that PermissionTemplates can be private though...

Agree if you are not using it as permission, but you declare it as Permission

Really everything should be private except what you need to do authorization calls outside.

I changed the visibility, but note that this may have to be reversed in future. Some other code may want to reuse the dynamic permissions to check a requirement of a folder for a different purpose.

…rBase.cs Co-authored-by: Hisham Bin Ateya <[email protected]>

Piedone · 2024-04-23T23:17:47Z

@coderabbitai review

coderabbitai · 2024-04-23T23:17:57Z

Walkthrough

This update introduces the "Secure Media" feature in OrchardCore, enhancing security and access control over media files. Changes include new configurations for caching secure files, permissions for media access, and event handling for media directory operations. The update also refines UI elements and backend services to support these security enhancements, ensuring that media files are handled securely based on user roles and permissions.

Changes

File Path	Change Summary
`.../appsettings.json`	Added `MaxSecureFilesBrowserCacheDays` configuration.
`.../Media/Assets/js/...`	Updated JS logic for folder creation and deletion permissions.
`.../Media/Controllers/AdminController.cs`	Enhanced folder management and added `AttachedMediaFieldFileService`.
`.../Media/Drivers/MediaFieldDisplayDriver.cs`	Updated temp folder assignment method.
`.../Media/Events/...`, `.../Media/Core/Events/...`	Added event handling for media directory operations.
`.../Media/Manifest.cs`, `.../Media/SecureMediaPermissions.cs`	Introduced "Secure Media" feature and related permissions.
`.../Media/Processing/MediaImageSharpConfiguration.cs`	Added cache control logic for secure files.
`.../Media/Services/...`	Added multiple services and handlers for secure media management.
`.../Media/ViewModels/...`, `.../Media/Views/Admin/...`	Updated view models and views to support new media security settings.
`.../Media/wwwroot/Scripts/media.js`	Modified Vue template for conditional rendering of folder actions.
`.../Media.Abstractions/...`, `.../Media.Core/...`	Extended media options and event handling interfaces.
`src/docs/...`	Documented new features and configurations in module README and release notes.
`test/OrchardCore.Tests/...`	Added tests for new authorization handling and updated test utilities.

This table summarizes the key changes across the OrchardCore project, focusing on enhancing media file security and access control.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

Piedone · 2024-04-24T00:03:58Z

@coderabbitai review

coderabbitai

Actionable comments posted: 1

Out of diff range and nitpick comments (1)

src/docs/reference/modules/Media/README.md (1)

Line range hint 1-1: Add a space after the period in the first sentence for better readability.

...Core.Tests/Modules/OrchardCore.Media/SecureMedia/ViewMediaFolderAuthorizationHandlerTests.cs

Piedone

Final approval and merge up to you, @hishamco.

hishamco

Just a quick comment, then we can merge

src/OrchardCore.Modules/OrchardCore.Media/Controllers/AdminController.cs

hishamco · 2024-04-25T20:39:56Z

Thanks for your contribution @gvkries

gvkries added 3 commits January 26, 2024 15:41

Adds 'view' permissions for media folders.

cf07183

Removes an unnecessary static field.

eb23458

Fix typo.

281fed5

github-actions bot mentioned this pull request Feb 1, 2024

Monthly community metrics report for 2024-01-01..2024-01-31 #15218

Closed

github-actions bot added the merge conflict label Feb 2, 2024

Merge branch 'main' into gvkries/secure-media-9369

526d453

github-actions bot added merge conflict and removed merge conflict labels Feb 3, 2024

Piedone requested changes Mar 22, 2024

View reviewed changes

This was referenced Mar 22, 2024

Forced effective Manage Media permission #12057

Closed

Media permissions #9369

Closed

Require authentication to access media files (secure media) #3590

Closed

Piedone reviewed Mar 23, 2024

View reviewed changes

src/OrchardCore.Modules/OrchardCore.Media/Services/ViewMediaFolderAuthorizationHandler.cs Outdated Show resolved Hide resolved

Added documentation.

9e271dd

gvkries requested a review from agriffard as a code owner April 3, 2024 10:50

gvkries and others added 11 commits April 3, 2024 12:54

Added sample configuration.

9158ba0

Update src/OrchardCore.Modules/OrchardCore.Media/Processing/MediaImag…

223ecd6

…eSharpConfiguration.cs Co-authored-by: Zoltán Lehóczky <[email protected]>

Update src/OrchardCore.Modules/OrchardCore.Media/Services/MediaOption…

b30fc42

…sConfiguration.cs Co-authored-by: Zoltán Lehóczky <[email protected]>

Update src/OrchardCore.Modules/OrchardCore.Media/Startup.cs

8021896

Co-authored-by: Zoltán Lehóczky <[email protected]>

Update src/OrchardCore.Modules/OrchardCore.Media/SecureMediaPermissio…

87f58cb

…ns.cs Co-authored-by: Zoltán Lehóczky <[email protected]>

Update src/OrchardCore.Modules/OrchardCore.Media/SecureMediaPermissio…

4f85d29

…ns.cs Co-authored-by: Zoltán Lehóczky <[email protected]>

Update src/OrchardCore.Modules/OrchardCore.Media/SecureMediaPermissio…

4051858

…ns.cs Co-authored-by: Zoltán Lehóczky <[email protected]>

Update src/OrchardCore.Modules/OrchardCore.Media/Controllers/AdminCon…

c90c09e

…troller.cs Co-authored-by: Zoltán Lehóczky <[email protected]>

Update src/OrchardCore.Modules/OrchardCore.Media/Services/ViewMediaFo…

1ca5a20

…lderAuthorizationHandler.cs Co-authored-by: Zoltán Lehóczky <[email protected]>

Update src/OrchardCore.Modules/OrchardCore.Media/Services/ViewMediaFo…

941c0c7

…lderAuthorizationHandler.cs Co-authored-by: Zoltán Lehóczky <[email protected]>

Update src/OrchardCore.Modules/OrchardCore.Media/Services/ViewMediaFo…

d231e42

…lderAuthorizationHandler.cs Co-authored-by: Zoltán Lehóczky <[email protected]>

gvkries added 3 commits April 17, 2024 18:28

React to code review.

a1ef044

Fix authorization for folders with special characters like spaces.

24db9fa

Added video to the docs.

9a7fe73

Piedone mentioned this pull request Apr 17, 2024

DI dependencies of features in the same assembly are mapped to the wrong feature #15782

Closed

Piedone approved these changes Apr 17, 2024

View reviewed changes

github-actions bot added the merge conflict label Apr 18, 2024

Merge branch 'main' into gvkries/secure-media-9369

e7d4733

github-actions bot removed the merge conflict label Apr 19, 2024

hishamco requested changes Apr 19, 2024

View reviewed changes

Apply suggestions from code review

ee8e77b

Co-authored-by: Hisham Bin Ateya <[email protected]>

hishamco reviewed Apr 21, 2024

View reviewed changes

gvkries and others added 4 commits April 23, 2024 09:30

Update src/OrchardCore/OrchardCore.Media.Core/Events/MediaEventHandle…

e6ef83c

…rBase.cs Co-authored-by: Hisham Bin Ateya <[email protected]>

Renamed SecureMediaMarkerService to SecureMediaMarker.

1f1381c

Corrects the visibility of members in SecureMediaPermissions.

06a89de

Merge branch 'main' into gvkries/secure-media-9369

524e9ac

coderabbitai bot reviewed Apr 24, 2024

View reviewed changes

...Core.Tests/Modules/OrchardCore.Media/SecureMedia/ViewMediaFolderAuthorizationHandlerTests.cs Show resolved Hide resolved

Piedone mentioned this pull request Apr 24, 2024

Set up CodeRabbit for AI code reviews (Lombiq Technologies: OCORE-159) #15813

Merged

Piedone approved these changes Apr 24, 2024

View reviewed changes

Merge branch 'main' into gvkries/secure-media-9369

af69a47

hishamco approved these changes Apr 25, 2024

View reviewed changes

src/OrchardCore.Modules/OrchardCore.Media/Controllers/AdminController.cs Show resolved Hide resolved

Merge branch 'main' into gvkries/secure-media-9369

380ec79

hishamco merged commit 9cf1f78 into OrchardCMS:main Apr 25, 2024
5 checks passed

gvkries deleted the gvkries/secure-media-9369 branch May 24, 2024 11:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added 'view' permissions for media folders. #15173

Added 'view' permissions for media folders. #15173

gvkries commented Jan 26, 2024 •

edited by Piedone

Loading

github-actions bot commented Feb 2, 2024

hishamco commented Feb 2, 2024

github-actions bot commented Feb 9, 2024

hishamco commented Feb 9, 2024

Piedone left a comment

Piedone left a comment

github-actions bot commented Apr 18, 2024

hishamco commented Apr 18, 2024

Piedone commented Apr 19, 2024

hishamco Apr 19, 2024

gvkries Apr 20, 2024

hishamco Apr 20, 2024

hishamco Apr 21, 2024

gvkries Apr 22, 2024

hishamco Apr 22, 2024

Piedone Apr 22, 2024

gvkries Apr 23, 2024

Piedone commented Apr 23, 2024

coderabbitai bot commented Apr 23, 2024 •

edited

Loading

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (`.coderabbit.yaml`)

Documentation and Community

Piedone commented Apr 24, 2024

coderabbitai bot left a comment

Piedone left a comment

hishamco left a comment

hishamco commented Apr 25, 2024

Added 'view' permissions for media folders. #15173

Added 'view' permissions for media folders. #15173

Conversation

gvkries commented Jan 26, 2024 • edited by Piedone Loading

github-actions bot commented Feb 2, 2024

hishamco commented Feb 2, 2024

github-actions bot commented Feb 9, 2024

hishamco commented Feb 9, 2024

Piedone left a comment

Choose a reason for hiding this comment

Piedone left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 18, 2024

hishamco commented Apr 18, 2024

Piedone commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piedone commented Apr 23, 2024

coderabbitai bot commented Apr 23, 2024 • edited Loading

Walkthrough

Changes

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configration File (.coderabbit.yaml)

Documentation and Community

Piedone commented Apr 24, 2024

coderabbitai bot left a comment

Choose a reason for hiding this comment

Piedone left a comment

Choose a reason for hiding this comment

hishamco left a comment

Choose a reason for hiding this comment

hishamco commented Apr 25, 2024

gvkries commented Jan 26, 2024 •

edited by Piedone

Loading

coderabbitai bot commented Apr 23, 2024 •

edited

Loading

CodeRabbit Configration File (`.coderabbit.yaml`)