Security Feature Team (1.5)

Topics: Announcements, Core
Coordinator
Mar 14, 2012 at 10:43 PM

This is the thread for the security feature team, where one can volunteer to do work and discuss design.

Strawman design proposal:

The top work item is item level permissions.
This could be implemented as a new Permission content part.
In this part's editor, the administrator could define what roles can view the item on the front-end.
Other permissions are not managed at this level of granularity for this release.
By default, this part is not added to any type.

Search and filter users by role. The user management screen would have a new drop-down for filtering by role. The existing search box would filter for users or roles containing the search term.

Role and permission export: only users can be imported and exported at the moment. Roles and permission settings should also be imported and exported.

If time permits, User-delegated user management: there would be a new "delegate" permission for each role (one such permission per role). Users that have this permission would be allowed to assign any user into the associated role. The scenario here is for example a moderator on a forum that can give users certain privileges, such as voting posts up or down, posting to the forum, or even moderate themselves. Today, such scenarios would pretty much require full admin rights.

Mar 15, 2012 at 4:21 AM
Edited Mar 15, 2012 at 4:24 AM

Item level permissions:

As one of the most commonly requested features regarding this is the concept of content-type level front-end permissions, I would prefer this part to be implemented in a way that allows for this in the most unintrusive way.

To that end, I would propose two settings for the Content Type-level part configuration:

  1. Default roles - Which roles should be preselected when an item is made.
  2. Show permission editor - If false, these default roles are unchangeable.
Coordinator
Mar 15, 2012 at 4:48 PM

My 2 cents

Create a View permission.

Check for the permission whenever a content item needs to be displayed.

Add an authorization alteration to check the role of the current user.

 

Possible implementation:

  • A part (SecuredContentPart) can be attached to any content type which needs to be validated.
  • The driver will check for the current user, and set a flag in an ActionFilter (like IsAdminFilter)
  • An ActionFilter will redirect to AccessDenied if the flag is set.
  • This assumes only the contents displayed using BuildDisplay are manageable
    • Examples should be found

 

Drawback

Links to content items are still displayed. Having a View Content Item permission which would be handled by permission alterations could (using the same part assignment mecanism) could be a solution. Then it's the module's developer work to decide if the link should be displayed if the content is not accessible. It still requires to load the content item though.

 

  • Optional:
    • In the part, decide what should happen when the user hits a secured content:
      • 404
      • Access denied
    • Optionally let the editor decide what roles can be checked, on define them at the part level
Developer
Mar 15, 2012 at 11:15 PM

I disagree with the use of a 'SecuredContentPart' I think that sounds more like a fudge.

Coordinator
Mar 15, 2012 at 11:34 PM

@Nick, what do you suggest ?

Developer
Mar 16, 2012 at 12:02 AM

I personally think that all content items should be validated at an item level... I dont see why you need a custom part to accomplish this?

Coordinator
Mar 16, 2012 at 12:06 AM

Yes, the part is Displayable/Editable and Actionable at the content item level.

Coordinator
Mar 16, 2012 at 12:07 AM

And the settings, like the ones CPyle is suggesting can be done at the type level. So you don't bother every type in the system. You add the part on the types you want to handle the security for, with or without pre-defined configuration/limits.

Mar 18, 2012 at 7:57 AM
Edited Mar 18, 2012 at 7:58 AM

Some considerations:

  • Assume that you view a page or content item that you (directly) have permission to view, and that at some point a widget or something on the page executes the display of a secured content item that you do not have permission to view. Using the context flag → filter method, the entire page becomes inaccessible. This is a valid design decision, but it may not be intuitive to a user, so its just something to be aware of. We could possibly allow configuration for only certain Display Modes (eg. Detail) to trigger the flag, but not sure if we need this.
  • I assume we will be providing a Projector filter for "User can view item." How much will this feature provide in the way of extensibility? Let's say someone wants to extend this module to add sharing with individual users. It would be easy to just write another PermissionPart Driver that adds its own logic, but can the Projector Filter also somehow be dynamically extended such that any permission-modifying module can add to it? That would be awesome.
  • Is providing a ContentManager query extension that will filter for permissions feasible and/or a good idea? I guess some of the same caveats apply to this as the above item.

 

 

Coordinator
Mar 18, 2012 at 8:48 AM

If an item is not allowed, we should simply not render it. The main content item is generally produced by a controller, and it could be the responsibility of that controller to do anything more. In other words, if the controller does nothing, you get an empty page, and if it does the right thing, it will give a non-authorized http status code.

Mar 30, 2012 at 5:33 PM
Edited Mar 30, 2012 at 5:35 PM

I've made a proof-of-concept implemented the way bertrand described.

It allows you to set role defaults per content type and enable or disable the editor for them.

Viewing the item is disabled via a handler that will load in the allowed roles OnLoading, and check them against the current user's roles OnGetDisplayShape. It sets the placement function (FindPlacement in BuildShapeContext) to return nothing if the user can't view the item.

I'll want to do some refactoring before I put anything up for review, and I'm still not sure on some things.

  1. I named the part to specifically refer to Role-Based view permissions rather than something generic like "ViewPermissionsPart." This would allow developers to create other types of view permissions parts to be added separately and call them something else (UserViewPermissionsPart, GroupViewPermissionsPart, etc.) We could go with the generic part name, and suggest that authors extend the part by providing more drivers for it, but then the different pieces of functionality could not be separately added.
  2. I'm thinking there should be a central service that is queried for content visibility. This service should inject all available IViewPermissionsProvider or something like that. These could be implemented for things like User/Group based permissions modules. Would these other permissions be intersected or unioned?
  3. As for the Content controller correctly returning as Unauthorized result, I think this could still be done via an action filter in the module that specifically targets that controller.
Developer
Mar 31, 2012 at 5:54 PM

Have you uploaded this to a fork?

Mar 31, 2012 at 11:28 PM
Edited Mar 31, 2012 at 11:32 PM

I just pushed a fork to http://orchard.codeplex.com/SourceControl/network/forks/CPyle/ViewPermissions

This is incomplete. A couple things should be done to make this usable:

  1. View permission records are not automatically generated for all content items when you add the part or change part settings on the content type. You have to go to the content and manually republish.
  2. Make a central service to house the IViewPermissionsProviders and make calls to them.
  3. Break out the Role-based view permissions part/provider to its own feature

I looked into making an "Is viewable by current user role" Projections filter and it looks like the HqlQuery interface isn't powerful enough to generate the kind of query we'd need for this. Current Projections layouts will display an item bullet point or other entry even if the contents are empty, so you end up with lists of empty spaces for items you don't have permission to see. You can of course use a custom layout and get around this, which would show the links but then deny you access if you clicked on them.

Coordinator
Apr 1, 2012 at 2:20 AM

Do you want to demo next Wednesday at the committee meeting? We could answer those questions as well.

Apr 2, 2012 at 5:32 PM

Sure. Anything specific I should have prepared?

Apr 7, 2012 at 5:30 AM

From the Wednesday meeting, it came up that framework-level things like the ContentManager and/or HqlQuery may need to be extended to allow for injection of filters based on view permissions, effectively rendering content inaccessible at the lowest level of querying if you do not have access to it. I'm not sure what the best approach is here.

If anyone wants to be added as contributor to the ViewPermissions fork let me know in this thread.

 

Apr 10, 2012 at 2:29 AM
Edited Apr 10, 2012 at 2:30 AM

When I added the ViewPermission part with the UI to a content type everything works as expected so far. But when I add the part with code  

            ContentDefinitionManager.AlterTypeDefinition("PermTest", cfg => cfg
                ......

                .WithPart("RoleViewPermissionsPart")  
                .Creatable()                
            );

the role permission fields are not showing. It seems that Orchard has behaves differently when adding a part in code compared to the UI - I think that's a bug. As a work around I've added the green code in RolesPermissionsPartDriver.cs

        protected override DriverResult Editor(RoleViewPermissionsPart part, dynamic shapeHelper) {
            var typeSettings = part.TypePartDefinition.Settings.GetModel<RoleViewPermissionsSettings>();

            if (!typeSettings.ShowRolesEditor && typeSettings.Roles == null)
            {
                var allRoles = _roleService.GetRoles();
                typeSettings.ShowRolesEditor = true;
                typeSettings.Roles = _roleService.GetRoles().ToDictionary(key => key.Name, value => true);
            }

            if (typeSettings.ShowRolesEditor) {

 For me the modified code now works as expected - but I'm not sure if a) this is the right solution and b) if Orchard shouldn't handle adding parts in code .WithPart and in the UI in the same way or c) if I missed something essential?

 

Apr 10, 2012 at 3:06 AM

The fields probably don't show up when added from code because they aren't being added with any default settings, so the part defaults to "No Editor" and also "No roles selected" making everything inaccessible. None of the settings get applied until you load up that content type and save it again. You're right that this is a problem.

It would be possible to add a WithSetting() call in your part alteration to enable the roles editor by default. The possible role selections are dynamic though, so there's no simple way to enable all roles through migrations. Your code covers the case of having the part function as expected without the content type settings needing to be manually saved.

There is still the issue of creating permission records for all existing content when the part is added to a content type with the "Show Editor" option enabled, something I haven't gotten to yet.

I've added you as a collaborator on the fork, feel free to push your changes.

 

 

Apr 10, 2012 at 12:27 PM

So do you think that it is a bug that adding a Part to Content Type with the UI does more than doing the same with code ContentDefinitionManager.AlterTypeDefinition .WithPart is a bug/issue in Orchard?

Or is there a better or official way to initialize the Settings field in Settings_ContentTypePartDefinitionRecord with the same values both in code and the UI?

Apr 10, 2012 at 5:53 PM
Edited Apr 10, 2012 at 5:54 PM
IntranetFactory wrote:

So do you think that it is a bug that adding a Part to Content Type with the UI does more than doing the same with code ContentDefinitionManager.AlterTypeDefinition .WithPart is a bug/issue in Orchard?

Or is there a better or official way to initialize the Settings field in Settings_ContentTypePartDefinitionRecord with the same values both in code and the UI?

It's not really a bug, just more of a limitation in the part system that usually doesn't matter. It matters to us because the logic used to find the possible role settings doesn't come into play until the settings form is generated. Most parts don't have this problem because their settings are static and can be supplied via migrations.

You could also just supply the WithSetting() calls in the migration needed to enable Anonymous and Authenticated viewing and that would cover everyone by default. Those roles are always present.

Apr 12, 2012 at 9:13 PM

Currently the names (and not ids) are stored in the settings:

<settings RoleViewPermissionsSettings.ShowRolesEditor="True" 
	RoleViewPermissionsSettings.Roles_x005B_0_x005D_.Key="Administrator" 
         RoleViewPermissionsSettings.Roles_x005B_0_x005D_.Value="False" RoleViewPermissionsSettings.Roles_x005B_1_x005D_.Key="Editor" RoleViewPermissionsSettings.Roles_x005B_1_x005D_.Value="False" RoleViewPermissionsSettings.Roles_x005B_2_x005D_.Key="Moderator" RoleViewPermissionsSettings.Roles_x005B_2_x005D_.Value="False" RoleViewPermissionsSettings.Roles_x005B_3_x005D_.Key="Author" RoleViewPermissionsSettings.Roles_x005B_3_x005D_.Value="False" RoleViewPermissionsSettings.Roles_x005B_4_x005D_.Key="Contributor" RoleViewPermissionsSettings.Roles_x005B_4_x005D_.Value="False" RoleViewPermissionsSettings.Roles_x005B_5_x005D_.Key="Anonymous" RoleViewPermissionsSettings.Roles_x005B_5_x005D_.Value="False" RoleViewPermissionsSettings.Roles_x005B_6_x005D_.Key="Authenticated" RoleViewPermissionsSettings.Roles_x005B_6_x005D_.Value="True" />

Wouldn't it be better to store just the Ids of the selected roles like:

RoleViewPermissionSettings.Roles="6,7" ?

Apr 12, 2012 at 10:36 PM
Edited Apr 12, 2012 at 10:36 PM

The enabled roles are intersected with the UserRolesPart's Roles property on the current user.  UserRoles.Roles is attached as a list of role names, so storing the settings as IDs would require an additional lookup to get the names anyway.

Originally I was storing the settings as a list of strings too, but found that I needed all available roles and which ones were enabled to display the settings form anyway and just went with a dictionary to do both at once.

Permission records also have a boolean for visibility being enabled or not. I ended up going this way rather than simply defining the presence of a record as providing visibility so that there would be no ambiguity when new content items or roles were added. If you look up a content item's role permission records (during OnLoad) and find nothing, it just means that you should load the default setting for that role rather than assume there should be no permissions. This is a major consideration when it comes to adding the part to content items that didn't have it before, or adding new role settings.

The other way to address this that I am still debating about, is where you go through and add records for every content item that is missing records for a particular role when you save the settings form. I don't know if the OnLoad method impacts performance enough to do it one way or the other.

Apr 13, 2012 at 12:11 AM

How should role permissions be assigned in code e.g. I create new content and want to grant a role a permission:

var newApp = _contentManager.Create("App", VersionOptions.Published);

newApp.As<RoleViewPermissionsPart>().AllowedRoles.Add("Authenticated"); <-- fails, AllowedRoles == null

I'd expected that AllowedRoles equals the selected roles from the part defaults - but it seems that I'm wrong? So I've found no way to assign roles to a content item created by code :-(

Is it really not possible? Did I miss something?

 

Apr 13, 2012 at 2:09 AM

AllowedRoles is attached / generated during the content's OnLoad event. A service will probably have to be created to manipulate permissions similar to how Taxonomies and Tags work.

Apr 17, 2012 at 1:05 AM

Newest revision to the fork uses a service to get and set permissions on a content item. It also gracefully degrades to pulling role-view permissions from the settings if Show Editor is not enabled or records are missing. Lots of code cleaned up in the driver / handler.

Coordinator
Apr 27, 2012 at 12:25 AM

Hi,

In order to take this feature to the next steps, I need one of you to step up and represent the rest of the team. Who would be willing to take that role? Your responsibility as team leader will be to report progress at the weekly steering committee meetings, either in written form or by showing up at the meeting.

Please start using https://trello.com/board/feature-security/4f989a71881ed5e05a47e271 for task management on this feature team.

Thanks,
Bertrand

Coordinator
Apr 27, 2012 at 5:18 PM

I stand as a candidate

Coordinator
Apr 27, 2012 at 5:27 PM

You're it then. Thanks.

Coordinator
May 9, 2012 at 2:03 AM

Feature Team Leaders, please join us tomorrow at 1PM Pacific Time for a status report at https://join.microsoft.com/meet/sebros/TWFQ3LYH (Lync client required). If you can't join us, please leave us a note here giving status before the meeting.

Thanks,

Bertrand

May 13, 2012 at 5:57 AM
Edited May 13, 2012 at 5:59 AM

Hi all. There's been some progress on the permission system and module.  Some updates:

Content-type level view permissions are now integrated into the current role system. You can uncheck the generic "View All Content" permission and then enable "View ___ Content" for individual content types per role.

The item-level role-based permissions are still in the form of a part that you attach to content types. Me and Sebastien have gone over a couple possible implementations of the module, but I'd like to get some input from anyone who is interested as to how the final version will work.

Right now, though the editor is not designed yet, you would get a list of roles with check boxes, and an option to indicate whether you are granting or denying access to the selected roles. Another idea I had for the implementation would be similar, though instead of selecting whether you grant or deny access, there would be a checkbox that would allow you to optionally restrict access to only the specified roles. This would cause the system to actively deny access to roles that aren't checked. This could also be a third option in the current implementation.

Any thoughts?

May 20, 2012 at 1:50 PM

Thank you for the update. For our requirements having a list of roles with checkboxes is not usable - in our use cases (all Intranet related) we have at least several dozens of roles and frequently even several hunderts. So we need a way to search for and select multiple roles similar to e.g. the selection of groups when permissions are added in windows.

So I think actively denying permissions is therefore also no option - so in our use cases access will be denied if not explicitly granted.

 

Coordinator
May 21, 2012 at 3:45 AM

Several hundred roles? May I suggest that you may have factored this wrong? That hardly seems like a typical scenario.

May 21, 2012 at 7:59 AM
We/our customers use ActiveDirectory/Ldap for user Management and the membership to specific user groups decides if specific content should be visible or changeable. Having 100+ groups in AD is - I think - not uncommon.


Sent from my iPad

On May 21, 2012, at 5:45 AM, "bertrandleroy"<notifications@codeplex.com> wrote:

From: bertrandleroy

Several hundred roles? May I suggest that you may have factored this wrong? That hardly seems like a typical scenario.

Coordinator
May 26, 2012 at 5:53 AM

Using AD in Orchard is even less typical. I think that is a problem that you should solve in a custom module, but certainly not something that should be handled in core.

May 26, 2012 at 11:19 AM

I think it will be no real problem to add a simple search option for roles instead of a long list showing all roles. So does this mean core Orchard is limiting itself to a low number of manageable roles? Then we’ll probably need to fork/extend the module when it’s finished.

From: bertrandleroy [email removed]
Sent: Saturday, May 26, 2012 1:53 AM
To: ma@intranetfactory.com
Subject: Re: Security Feature Team (1.5) [orchard:348642]

From: bertrandleroy

Using AD in Orchard is even less typical. I think that is a problem that you should solve in a custom module, but certainly not something that should be handled in core.

Coordinator
May 27, 2012 at 5:47 AM

No, what it means is that we need to prioritize work and that would rank pretty low. For example, we don't even have search on the content screen yet, and that would be much higher priority. But of course, if you want to contribute it, you are more than welcome to do so.

Jul 21, 2012 at 11:48 PM

Did Orchard.ContentPermissions replace Orchard.ViewPermissions in v1.5 ?

Jul 22, 2012 at 12:01 AM

Yes. View permissions were added as a core feature. The part became capable of dealing with more than just view permissions, hence the name change.

 

Jul 22, 2012 at 9:22 AM

Call me naive - but I'm surprised. In a "community driven" project I'd somehow the expectation to read something about the ongoing development either in this thread or at least in the Announcements section. But I didn't see any hint about the ongoing development of Orchard.ContentPermissions.

If I'd seen the developement I'd have questioned if storeing the permissions as a kind of blob in the database is really efficient and not severely limiting possible performance tunings.

Did I look in the wrong places?

Or is "community" input not really expected?

 

Coordinator
Jul 23, 2012 at 5:43 PM

"Or is "community" input not really expected?" Fun you ask it, because this module was started by Chris (CPyle here). 

There is a Trello board for the current development iteration where we kept all tasks related to every feature for 1.5, at the macro level ( https://trello.com/board/current-iteration/4e701edeab430ba869017d2a) and also for every specific features. We also have online meetings every week where we talk about the evolution of every feature, backed up by the Trello board. The meetings have been recorded and published on YouTube for the past few weeks, by another community member. The current and past development priorities are written here: http://docs.orchardproject.net/Documentation/Feature-roadmap

We continued the discussions by email and skype as it was easier for everyone at that point. Maybe we lost you in the loop, sorry for that.

Orchard.ContentPermission is the same code-base as ViewPermissions, it's just the name has changed because it was not reflecting what the purpose was.

Aug 26, 2012 at 9:24 AM
Edited Aug 26, 2012 at 10:46 AM

So guys, any news about the import/export of roles + permissions? permission delegation? These two features are very important for me now, so may I have some instructions on starting to implement + contrib them?

Aug 26, 2012 at 2:30 PM
Edited Aug 26, 2012 at 2:31 PM

I managed to achieve the import/export of roles + permissions feature by building a module with three c# files -I used the Rules/Actions module as a reference-:

PermissionsCustomExportStep.cs

using System.Collections.Generic;
using Orchard.Events;

namespace Ikd.PermissionsImportExport.ImportExport {
    public interface ICustomExportStep : IEventHandler {
        void Register(IList<string> steps);
    }

    public class PermissionsCustomExportStep : ICustomExportStep {
        public void Register(IList<string> steps) {
            steps.Add("Permissions");
        }
    }
}

 

PermissionsExportEventHandler.cs

using System.Collections.Generic;
using System.Linq;
using System.Xml.Linq;
using Orchard.Events;
using Orchard.Roles.Services;

namespace Ikd.PermissionsImportExport.ImportExport {
    public interface IExportEventHandler : IEventHandler {
        void Exporting(dynamic context);
        void Exported(dynamic context);
    }

    public class PermissionsExportHandler : IExportEventHandler {
        private readonly IRoleService _roleService;

        public PermissionsExportHandler(IRoleService roleService) {
            _roleService = roleService;
        }

        public void Exporting(dynamic context) {
        }

        public void Exported(dynamic context) {
            if (!((IEnumerable<string>)context.ExportOptions.CustomSteps).Contains("Permissions")) {
                return;
            }

            var allRoles = _roleService.GetRoles();

            var root = new XElement("Permissions");
            context.Document.Element("Orchard").Add(root);

            foreach (var role in allRoles) {
                root.Add(new XElement("Role",
                    new XAttribute("Name", role.Name),
                    new XElement("Permissions", role.RolesPermissions.Select(permission =>
                        new XElement("Permission",
                            new XAttribute("Name", permission.Permission.Name ?? string.Empty),
                            new XAttribute("Feature", permission.Permission.FeatureName ?? string.Empty),
                            new XAttribute("Description", permission.Permission.Description ?? string.Empty),
                            new XAttribute("Id", permission.Permission.Id)
                        )
                    ))
                ));
            }
        }
    }
}

 

PermissionsRecipeHandler.cs

using System;
using System.Linq;
using Orchard.Recipes.Models;
using Orchard.Recipes.Services;
using Orchard.Roles.Services;

namespace Ikd.PermissionsImportExport.ImportExport {
    public class PermissionsRecipeHandler : IRecipeHandler {
        private readonly IRoleService _roleService;

        public PermissionsRecipeHandler(IRoleService roleService) {
            _roleService = roleService;
        }

        // <Data />
        // Import Data
        public void ExecuteRecipeStep(RecipeContext recipeContext) {
            if (!String.Equals(recipeContext.RecipeStep.Name, "Permissions", StringComparison.OrdinalIgnoreCase)) {
                return;
            }

            foreach (var role in recipeContext.RecipeStep.Step.Elements()) {

                // get role from db
                var roleName = role.Attribute("Name").Value;
                var roleRecord = _roleService.GetRoleByName(roleName);

                // create the role if it doesn't already exist
                if (roleRecord == null) {
                    _roleService.CreateRole(roleName);
                    roleRecord = _roleService.GetRoleByName(roleName);
                }

                // set the role permissions
                var rolePermissions = role.Element("Permissions").Elements().Select(
                    permission => permission.Attribute("Name").Value);
                _roleService.UpdateRole(roleRecord.Id, roleRecord.Name, rolePermissions);
            }

            recipeContext.Executed = true;
        }
    }
}

Coordinator
Aug 27, 2012 at 5:33 AM

Looks nice. Did you consider contributing that?

Aug 27, 2012 at 9:37 AM
Edited Aug 27, 2012 at 1:36 PM
bertrandleroy wrote:

Looks nice. Did you consider contributing that?

Yes, I'm thinking to, but was wondering what you have done with the same issue as I know you were working on!

Also what about permission delegation? I will work on this soon, so need any info/notes/refs or ref persons to talk/discuss.

Coordinator
Aug 28, 2012 at 3:52 AM

For every release, we make proposals of features that we think would be nice to build. Whether they get built or not depends on whether a developer actually makes it happen. This particular set of features didn't, but we sure would take the contribution if it matches our quality criteria.

All notes are public, so what you find on the forums is what we have.

Aug 28, 2012 at 10:47 AM
Edited Aug 28, 2012 at 2:13 PM

Great, so can you tell me more about your quality criteria so I can follow it?

And about 'delegation' permission, isn't building such feature will need changes in the core, not just a new contrib module?

Coordinator
Aug 29, 2012 at 5:17 AM

They are our totally arbitrary appreciation ;) Try to be consistent with the coding style of existing core code.

About delegation, I don't know, it would require some inquiry.

Aug 29, 2012 at 10:42 AM

I will do my best and try to contrib both features soon.