New users should not be granted access to manage and view all forms

[sussexrick]

First reported as http://issues.umbraco.org/issue/CON-1022 on Umbraco 7.6.12 and Forms 6.0.5.

When a new form is created all other users with access to Forms can see and edit that form. When a new user is created they have access to all forms.

It seems Umbraco Forms has an overall permissions record for each user (Manage Forms, Manage Datasources etc) and per-user permissions records for each form (Access to form). If any permissions record is missing, as they are for new users and new forms, the default is to 'allow'. The default should be to 'deny'.

For anyone looking for a workaround until this is fixed, I have written some code which looks for any missing permissions records and sets them to 'deny'. Existing permissions, either 'allow' or 'deny', are preserved. Ideally the code would be run when a new user or form is created, but there don't seem to be events for either. Instead I plan to run it frequently using a scheduled task to call it as a web API:

https://github.com/east-sussex-county-council/Escc.Umbraco.Forms/
https://www.nuget.org/packages?q=Escc.Umbraco.Forms

The controller code for the entries viewer also has this assumption that data should not be secured:

//By default set to have access (in case we do not find the current user's per indivudal form security item)
    $scope.hasAccessToCurrentForm = true;

[marcond05]

+1 for this request. It renders Umbraco Forms pretty useless in a multi-site situation giving all users access to all forms that are created. We only just discovered this, and it's surprising to see that all users would always get all permissions to all forms.

Rick, thanks for your workaround. I did hunt down and change $scope.hasAccessToCurrentForm = false;

That didn't seem to have an affect on newly created forms. I was walking through your code here, and it looks like it all boils down to line 220 of https://github.com/east-sussex-county-council/Escc.Umbraco.Forms/blob/master/Escc.Umbraco.Forms.Security/UmbracoFormsSecurity.cs

So given this use case. There are 2 existing forms, User A has 1 form, User B has 1 form. User A then creates a new form.

User B will have access to User A's form (they are different clients, yikes). But when exactly will Umbraco Forms create the User Form permission records you mentioned? Or rather, how are you differentiating between "existing permissions" and newly created forms?

[sussexrick]

@marcond05 My code looks at each user and each form, and checks for a permissions record. If it finds anything it leaves it alone - that's how it preserves existing permissions. If it finds nothing then the permissions for that user have never been set, and unfortunately the default is to allow, so it creates a deny record.

To answer your question the first way, User B will have access to User A's form from the moment it's created until the moment my workaround runs (or permissions are set manually). That's why I recommend running it regularly, to reduce that exposure. On a big site it takes a while to run though so you can't run it frequently enough to completely eliminate the chance that User B will see User A's form.

To Umbraco HQ: I think this should be a fairly quick fix. It doesn't require radical changes to the permissions system, just a change to the default behaviour when no permissions are present.

[marcond05]

Thanks @sussexrick ,

I see what you mean now. For anyone else arriving at this issue cold, here is precisely what happens:

1. User ID 6 creates a form
2. If you look at the UFUserFormSecurity table you will see this single entry has been created
   
3. Now log out, and log in as some other user (for me User ID 5)
4. Now visit backoffice /umbraco/#/forms
5. Form table now reports 2 entries
   

The user doesn't even have to visit the form itself, just click on the root /umbraco/#/forms url and that user will have access to the form.

Have you tried addressing this with plane ole' SQL script by chance? I have done Syncing processes and ended up modifying revision nodes directly as the Umbraco services tend to be a little slow on recurring processes like this.

PS - I arrived at your ticket after Umbraco Forms team told me to put a feature request in. But considering you originally raised this issue in 2016 I think we may end up doing something similar to your workaround :)

[sussexrick]

No I haven't tried using SQL directly. I stick to APIs when I can to insure against changes between versions.

If you can fork and improve my workaround rather than starting from scratch then please do. I was made aware that there are FormStorage.Saving / .Saved events that might speed things up, but I haven't had a chance to revisit it. I'm not sure whether it helps because the "new user" case would still require a regular sweep of the site.

[marcond05]

Thanks @sussexrick . That's interesting, support just told me there were no events. But I think he was referencing the documentation, which I think only covers the fluffy front-end stuff for developers. I have a feeling those events are more geared toward actual form submissions, not form management, but I would love to be wrong.

Events would certainly be a cleaner solution. Although hopefully we can get a Default permission behavior on forms some day.

I will let you know what I come up with, but much appreciated for your efforts and putting this post & code together! 👍 I will send this over to the Support fellow, as he told me to put a ticket in.

Cheers,
-Marc

[ronaldbarendse]

Probably related to issue #3, as 'Manage Forms' is required to even view the entries.

[markrenwickshout]

+1 for this request, the default should be that users are not granted access to manage and view all forms

[Chris-N1]

+1 for this (as of Forms 8.4.0) - especially when there's no "check all/uncheck all" box on the Forms permissions page, and we have about 50 - 60 forms.

[marcemarc]

Just adding these snippets here in case it helps workaround this issue, or help consider a fix:

It's possible to tap into the formstorage_created event that fires when a new form is created, and loop through all users to add a deny record to the new form for each user...

    public void Initialize()
        {
            //fires everytime a form is created      
            _formStorage.Created += _formStorage_Created;
        }

private void _formStorage_Created(object sender, global::Umbraco.Forms.Core.FormEventArgs e)
        {
            //here we want to make sure that only the person creating the form can have ownership of the form
            //get reference to the form being created
            Form form = e.Form;
            //we need access to the 'WebSecurity' property of the current UmbracoContext to determine the Current user
            //But we are outside of a WebRequest, eg not a Controller or View
            //So we need to call EnsureUmbracoContext to achieve this
            using (UmbracoContextReference umbracoContextReference = _umbracoContextFactory.EnsureUmbracoContext())
            {
                var umbracoContext = umbracoContextReference.UmbracoContext;

            // get current user
            IUser user = umbracoContext.Security.CurrentUser;
                // now this is the bit that will need to be reviewed in the future
                // we are just getting 'All' the users without any attempt to 'page' the recordset
                // and remember this is happening whilst a form is being created, so this may give a delay to the user in the backoffice, or it may be fine...
                // (the way to improve this in the future is to just all an api endpoint that makes a bulk insert of the form records.
                _logger.Info<SubscribeToFormStorageCreated>("GetAllUsers");
                    IEnumerable<IUser> allusers = _userService.GetAll(0, int.MaxValue, out long count);

                // looping through the users adds a deny permission for all users other than
                // the form creator and admins.
                _logger.Info<SubscribeToFormStorageCreated>("Start - loop through all Users to set default permissions for this form: " + form.Name);
                foreach (IUser formUser in allusers)
                    {
                    // the current user will automatically have access to the form (you could allow admins too: formUser.IsAdmin())
                    if (formUser.Id == user.Id)
                        {
                            continue;
                        }
                        //for everyone else:
                        UserFormSecurity userFormSecurity = new UserFormSecurity();
                        userFormSecurity.Form = form.Id;
                        userFormSecurity.User = formUser.Id.ToString();
                        userFormSecurity.HasAccess = false;
                        userFormSecurity.AllowInEditor = false;
                        userFormSecurity.SecurityType = FormSecurityType.ReadOnlyViewEntries;

                        _userFormSecurityStorage.InsertUserFormSecurity(userFormSecurity);
                    }
                _logger.Info<SubscribeToFormStorageCreated>("Finished - loop through all Users to set default permissions for this form: " + form.Name);
            }
        }

and the other way around you can tap into the User Saved event (there isn't a first time create event for User), and add a deny record for all forms for the new user:

  public class SubscribeToUserSaved : IComponent
    {
        private readonly IFormStorage _formStorage;
        private readonly IUserService _userService;
        private readonly IUserFormSecurityStorage _userFormSecurityStorage;
        private readonly ILogger _logger;
    
        public SubscribeToUserSaved(IFormStorage formStorage, IUserService userService, IUserFormSecurityStorage userFormSecurityStorage, ILogger logger)
        {
            _formStorage = formStorage;
            _userService = userService;
            _userFormSecurityStorage = userFormSecurityStorage;
            _logger = logger;

        }
        public void Initialize()
        {
            //fires every time a User is saved
            //there is no created event, but when a user is saved it has a 'state' so we can determine new users
            UserService.SavedUser += UserService_SavedUser;
        }

        private void UserService_SavedUser(IUserService sender, Events.SaveEventArgs<IUser> e)
        {
            // check if the user being created is new
            foreach (IUser user in e.SavedEntities)
            {
                // Form permissions need to be set when user is first saved
                // eg when they are invited, or when they have been setup and not logged in yet - eg inactive
                if (user.UserState == UserState.Invited || user.UserState == UserState.Inactive)
                {
// this will fire everytime a user is saved before they are logged in so we also compare updateDate/CreateDate to see if they are the same
                    if (user.UpdateDate == null || user.CreateDate == user.UpdateDate)
                    {
                        //, unfortunately, none of the Groups at this point have been populated - which means on 'first save' we can't determine if the user being created is an Admin... (but we can on second save! - so you 'could have logic here based on groups, and remember to always save people twice - but an admin can always add themselves to a form anyway...)
                        // we are getting 'all forms' in this way may be slow if there are a million forms
// future improvement would be to ping an API that sets the deny permissions in one batch.
                        _logger.Info<SubscribeToUserSaved>("Getting All Forms");
                        var allForms = _formStorage.GetAllForms();
                        _logger.Info<SubscribeToUserSaved>("Start - looping through forms and setting form permissions for user: " + user.Name);
                        foreach (var form in allForms)
                        {
                            UserFormSecurity userFormSecurity = new UserFormSecurity();
                            userFormSecurity.Form = form.Id;
                            userFormSecurity.User = user.Id.ToString();
                            userFormSecurity.HasAccess = false;
                            userFormSecurity.AllowInEditor = false;
                            userFormSecurity.SecurityType = FormSecurityType.ReadOnlyViewEntries;

                            _userFormSecurityStorage.InsertUserFormSecurity(userFormSecurity);
                        }
                        _logger.Info<SubscribeToUserSaved>("Finished - looping through forms and setting form permissions for user: " + user.Name);
                    }                    
                }
            }
        }
}}

So you can sort of see 'this approach works' but it's not super scalable if there are a 1000 users or 1000 forms!- what would be great would be to perhaps have bulk insert options for UserFormSecurityStorage that would allow you to insert instruction to multiple users for a form, or multiple forms for a user, with a single SQL command...

The other thing would be handy would be having a Created event for Users which had the Groups populated.

Also, there is a real world scenario where users shouldn't have access to manage forms - but should have access to view and export entries from individual forms, but perhaps that's a separate issue.

[umbrabot]

Hiya @sussexrick,

Just wanted to let you know that we noticed that this issue got a bit stale and might not be relevant any more.

We will close this issue for now but we're happy to open it up again if you think it's still relevant (for example: it's a feature request that's not yet implemented, or it's a bug that's not yet been fixed).

To open it this issue up again, you can write @umbrabot still relevant in a new comment as the first line. It would be super helpful for us if on the next line you could let us know why you think it's still relevant.

For example:

@umbrabot still relevant
This bug can still be reproduced in version x.y.z

This will reopen the issue in the next few hours.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[ronaldbarendse]

Fixing this will be part of overhauling the Forms permissions and making sure managing and viewing forms are separated,, which is already discussed in #3.

[sussexrick]

@umbrabot still relevant

It'll be good to see this fixed as part of the work that fixes #3, but I'm reopening to make sure this remains on the radar because it's not directly discussed there.

[AndyButland]

Due in the next minor release, 8.11.0 and 9.3.0.

[Marco-Rinia]

+1 for this - especially when there's no "check all/uncheck all" box on the Forms permissions page, and we have about 50 - 60 forms.

Currently using:
Umbraco version 10.3.2
Current installed version of Umbraco Forms: 10.1.3