Basic PR for #154

[jamiehowarth0]

Basic node filtering applied for visible domains and redirects according to "Content start node" property on user and their respective groups.

[abjerner]

Hi @jamiehowarth0 and thanks for the PR 👍

Unfortunately there are a number of issues with your PR, which was one of the reasons I suggested sharing your thoughts before creating a PR. I'll try to find some time to do a detailed reply 😉

But as mentioned in the issue, we should try to avoid breaking changes. Any changes to the method signatures of IRedirectService are breaking changes (same with new methods). Your PR also enables the new start node checks by default. Since the package has never had this before, this is also a breaking change that could potentially cause problems for people upgrading the package. So the permissions should be opt-in instead of being enabled by default without any opt-out options.

[abjerner]

Hi @jamiehowarth0

As mentioned in my previous comment, your PR introduces a number of breaking changes. As much as possible, we should avoid making those.

Permissions

The controller properly needs to deal with permissions. Since the package has never had permissions, checking against the user's start nodes should not be enabled by default, but be op-in.

We can control this by adding a new property in appsettings.json like this:

{
  "Skybrud": {
    "Redirects": {
      "Permissions": {
        "UserStartNodes": true
      }
    }
  }
}

I've just added some logic that allows disabling the content app via appsettings.json, so if you sync these changes back to your fork, you should be able to build on top of this (eg. by creating a new RedirectsPermissionsSettings class similar to how I created the new RedirectsContentAppSettings class): ab3127f

RedirectsService

• GetUserAccessibleNodes
  This method feels a little off to me. For one, adding a new method to RedirectsService and IRedirectsService is a breaking change. I suggest dropping this method and moving it's logic inside the GetRootNodes method instead.

• GetRootNodes
  Adding a new parameter to this method is also a breaking change. Please reverse your changes to this method, and consider creating an overload directly in the IRedirectsService interface instead. With newer versions of C# we can introduce new methods in an interface (including implementation) without creating a breaking change. Eg. such as:

  /// <summary>
  /// Returns an array of all rode nodes that the specified <paramref name="user"/> has access to.
  /// </summary>
  /// <returns>An array of <see cref="RedirectRootNode"/> representing the root nodes.</returns>
  public RedirectRootNode[] GetRootNodes(IUser user) {
  
      RedirectRootNode[] rootNodes = GetRootNodes();
  
      HashSet<int> rootNodeIds = new();
  
      if (user.StartContentIds != null) {
          foreach (var rootNodeId in user.StartContentIds) {
              rootNodeIds.Add(rootNodeId);
          }
      }
  
      foreach (IReadOnlyUserGroup group in user.Groups) {
          if (group.StartContentId != null) {
              rootNodeIds.Add(group.StartContentId.Value);
          }
      }
  
      return rootNodes.Where(rootNode => rootNodeIds.Any(x => rootNode.Path.Contains(x))).ToArray();
  
  }

• GetRedirects
  Again adding a new parameter is a breaking change. The method already takes a parameter of type RedirectsSearchOptions, which then has various options for the search.

  The RedirectsSearchOptionsclass already has a RootNodeKey property of type Guid?. We can't remove this either as it would be another breaking change, but we can mark it as obsolete, and then add another property called RootNodeKeys or something like that:

  /// <summary>
  /// Gets or sets the key the returned redirects should match. <see cref="Guid.Empty"/> indicates all global
  /// redirects. Default is <c>null</c>, in which case this filter is disabled.
  /// </summary>
  [Obsolete($"Use '{nameof(RootNodeKeys)}' property instead.")]
  public Guid? RootNodeKey { get; set; }
  
  /// <summary>
  /// Gets or sets a list of keys for the root nodes the returned redirects should match.
  /// </summary>
  public List<Guid> RootNodeKeys { get; set; }

RedirectsController

• IUserService + GetUser
  Your PR injects an IUserService interface in the constructor. Generally speaking, Umbraco has an IBackOfficeSecurityAccessor interface you can inject instead. You can then access the current user via IUser user = backOfficeSecurityAccessor.BackOfficeSecurity?.CurrentUser;. This then avoids an unnecessary lookup via the IUserService.

  I also just pushed some code that now exposes CurrentUser property on RedirectsBackOfficeHelper, which is already injected in the controller. Using this should help keeping the code clean and avoid injecting other services into the controller. if you check d4a53c8, you can see that I've also used the IBackOfficeSecurityAccessor interface.

• GetRootNodes
  Assuming you added the UserStartNodes flag, this method should check whether the flag is enabled, and if so, make a call to _redirects.GetRootNodes(_backOffice.CurrentUser). If not enabled, the method should call _redirects.GetRootNodes() like it does today.

• GetRedirects
  This method should be updated to no longer use the options.RootNodeKey property, but use the new options.RootNodeKeys property instead.

  If UserStartNodes is enabled, the method should check whether a specific root node has been selected in the UI, if so, set the options.RootNodeKeys to only this root node. If no root node has been selected, the options.RootNodeKeys property should instead be set to the root nodes that the user has access to.

  If UserStartNodes is not enabled, the options.RootNodeKeys property should only be set if the user has selected a root node in the UI.

There are likely other considerations to do as well, but if you can make the changes that I've described above, I can try testing it some more, and then hopefully merge it into the main branch. I hope this makes sense 😉

[jamiehowarth0]

@abjerner this should do the trick, I hope?

[abjerner]

Awesome @jamiehowarth0 👍

I just had a quick look through your changes, and made a few comments. Could you look into this? My comment about the interface part is the most important. The rest is minor stuff.

I haven't tested your changes yet, but I'll try finding some time to look into that as well.

[abjerner]

Sorry - I made a mistake in my description about this property. My intention was to indicate whether to use starts nodes as the basis for permissions.

Could you rename this to UseStartsNodes?

The XML summary is also slightly wrong, as the default is false and not true. And lanword should be langword.

[abjerner]

Isn't the .ToArray() call redundant here?

[jamiehowarth0]

My bad, caught it.

[abjerner]

This is still a breaking change as you're introducing a new method. It's probably not very likely, but developers may choose to write their own implementation of the IRedirectsService interface, and if they do, they'll now be missing the GetRootNodes method.

C# 8 introduces default interface methods. So as the method doesn't use any dependencies, you should be able to move the method body to the interface instead. This ensures that we can add a new method without it being a breaking change.

Hope that makes sense 😉

[jamiehowarth0]

Just pushed latest. That should hopefully do the trick.

[jamiehowarth0]

@abjerner Can you take a minute to review the last set of changes I pushed and see if they're good to merge in?

[abjerner]

Hi @jamiehowarth0

It takes a bit more than a minute to review and test, which is why I haven't had the time to do this yet. I hope to get around to this soon though 😉

[abjerner]

Hi @jamiehowarth0

It's been some time since I last replied in this thread, so I thought I would give a quick update.

I've been able to find some time here and there to look into your PR, but unfortunately I don't think the PR is in a state that I would consider complete, so it needs some more work before it can be merged and released.

For now, you're the only one who was requested this feature, so I need to see this in the big picture, so it fits as many users as possible, and not just your specific use case. I'm not entirely sure yet what the end result should be like, so I'll need some more time looking into this. As such, it's also hard for me to give you suggestions on how to proceed. I hope that makes sense 😉

[jamiehowarth0]

Hi @abjerner:

Your refusal to accept this PR - or criticism of the security model at large - hinges on one fundamental issue: that everyone who accesses the Umbraco backoffice can be trusted.

Umbraco has supported multi-tenancy for websites AFAIK since it's inception, with the ability to restrict access to content nodes on a per-user or per-group basis.

The fact that the dashboard does not filter data based on Umbraco security rules, is very much a Skybrud problem for not forseeing this issue.
My client & I currently are having to run a fork to enable security to work and ensure that users in groups A & B, controlling nodes P & Q, cannot see redirects created & managed by users in groups C & D, who control nodes X & Y.
Users A & B should never see, or be able to control, redirects for nodes X & Y. Vice versa, users in groups C & D should never be able to manage redirects for nodes P & Q.
Disabling the dashboard with a UserComponent isn't an option - users in both groups need to be able to manage redirects for their respective sites.
Leakage of data therefore needs to be managed at the plugin level and work with existing Umbraco security rules.

If you don't want to fix it, that's fine, but we'll then be forced to release a Nuget package and fork your repository, explicitly highlighting the fact we've fixed a data leakage issue that you're not willing to address.

Umbraco's always had multi-tenancy support (or at least, AFAIK, and I've been working with Umbraco since December 2009). Your failure to factor this into a security model, and assuming that every content editor belongs to one trusted authority, is a security design problem with you, not with us for highlighting the underlying issue at large.

TL;DR: You have a fundamental security design flaw, by assuming that everyone who can access the Umbraco content area, can be trusted to manage global redirects across multiple nodes. This is patently false and ignores Umbraco's own user security checks.

[abjerner]

Hi @jamiehowarth0

I've never wrote that I'm going to refuse this PR. In fact I wrote that I agree that this is something that should be added to the package. What holds back this PR is primarily a question about having the time to work on this. I'm currently not in a position where I can work on reviewing and merging this PR during business hours, which then means I have to work on it my spare time instead.

I'm the only developer and maintainer of pretty much all our packages, so I need to prioritize what I'm working on. I'm okay with spending a some of my spare time working on and supporting these packages, but I'm typically prioritizing a mix of stuff that I need my self, and what I see that benefits the Umbraco community the most.

In your case, I need to spent an unknown amount of hours finishing your code before I'm able to merge it, and so far I haven't been able to find that time. I dont really like seeing PRs like yours, that stay idle for long periods of time - but that's generally the way it has to be - otherwise I end up burning out if I jump on it all at once.

I hope that makes sense 😉

[abjerner]

Hi @jamiehowarth0

As I wrote earlier, I don't think this PR is fully complete. It touches some aspects of permissions, but primarily does so for the global dashboard, but not so much other parts of the package.

If this PR should be merged, it should also touch some of these other parts of the package as well. So far, I've uncoved these parts that should be addressed:

• Global redirects
  The redirects package allows creating site specific redirects as well as global redirects. Currently even with your permissions changes enabled, editors with limited access can still create and edit global redirects, although they probably shouldn't be able to. Or it should be configurable.

  I guess it would probably be best to have a setting for indicating whether editors with limited access should have access to global redirects or not. This setting could probably be called Skybrud:Redircts:Permissions:AllowGlobal or something (feel free to suggest a better name).

• Content app + property editor
  So far your changes are mostly around the global dashboard, but the package also has a content app and a property editor for viewing redirects specific to a given content or media item. Currently if a redirect is created to page the user has access to, but from a domain they don't have access to, they can still edit or delete the redirect. This shouldn't be possible.

  I'd say these redirects should still be shown, but the user shouldn't be able to edit or delete them. This probably requires exposing in the underlying API whether the user has access to edit/delete a specific redirect.

  Or maybe this is also something that should be configurable.

• UseStartNodes
  UseStartNodes shouldn't be part of the content app settings (Skybrud:Redircts:ContentApp:UseStartNodes), as it's something that affects the package globally. In my comment the 5th of November, I suggested to be located under Skybrud:Redircts:Permissions:UseStartNodes.

Can you look into these parts as well?

Feel free to use this thread for discussing the progress. I'll try to be better responding 😉