U4-9573 - PublicAccess not working - SQL LEFT JOIN unordered

Created by Simone Coletta 28 Feb 2017, 11:28:46 Updated by Shannon Deminick 02 Mar 2017, 17:46:07

I used public access to manage the access to various node of my site for the members. Yesterday the mechanism broke, some resources that previously were visible to some member groups was still not visible, so I checked the Public Access of the node and saw that there was only one or two groups, instead of all I set. I tried to add all the previous groups but I got an error from Umbraco, told the indexes were duplicated.

So I checked the table [umbracoAccessRule] on DB, and saw that ALL the groups were correctly listed there, but the Public Access dialog didn't show them, and also the method "MemberHasAccess" didn't work correctly (returned node only for one or two groups of members).

I went deeper with a debug on Umbraco source, and found that the issue was on the database fetch and map on the tables [umbracoAccessRule] and [umbracoAccess], made in the method below:

protected override IEnumerable PerformGetAll(params Guid[] ids) { var sql = GetBaseQuery(false);

        if (ids.Any())
            sql.Where("umbracoAccess.id IN (@ids)", new { ids = ids });

        var factory = new PublicAccessEntryFactory();
        var dtos = Database.Fetch<AccessDto, AccessRuleDto, AccessDto>(new AccessRulesRelator().Map, sql);
        return dtos.Select(factory.BuildEntity);

The issue was that AccessRulesRelator needed the records of the query to be ordered, but that wasn't the case. I tried to solve this issue adding an OrderBy on the query, like this:

protected override Sql GetBaseQuery(bool isCount) { var sql = new Sql(); sql.Select("*") .From(SqlSyntax) .LeftJoin(SqlSyntax) .On<AccessDto, AccessRuleDto>(SqlSyntax, left => left.Id, right => right.AccessId)

            .OrderBy(new string[] { "umbracoAccess.id" }); 
        return sql;

and everything worked. So SQL joining the two table without an explicit OrderBy could return an unordered list of record but mapping code assumed that the list was ordered.


Sebastiaan Janssen 02 Mar 2017, 10:40:55

PR: https://github.com/umbraco/Umbraco-CMS/pull/1769

Shannon Deminick 02 Mar 2017, 16:51:40

Only problem with this PR is that the PerformGetAll already has an order by so there will now be 2x order by, this was originally 'fixed' in U4-6247 but that doesn't seem right either because it's sorting on the wrong id. I will have a look at what needs to be changed.

Alessandro Calzavara 02 Mar 2017, 17:00:15

Maybe the change could be implemented on the mapper (so that it works also with non-ordered data) and not on the dataset itself.

Shannon Deminick 02 Mar 2017, 17:02:29

It's all a bit messy, we'll fix this one up manually for now, in v8 these Relators don't exist since NPoco deals with this more elegantly. I'll fix it up, all good!

Priority: Show-stopper

Type: Bug

State: Fixed


Difficulty: Normal


Backwards Compatible: True

Fix Submitted:

Affected versions:

Due in version: 7.5.11

Sprint: Sprint 53

Story Points: