Skip to content

Consider refactoring how permission data are represented #442

@cmacmackin

Description

@cmacmackin

Suggestions made by @ZedThree while reviewing #421:

I realised why I've been struggling to get my head around some of the implementation details, and that's because the design feels a little backwards. We have an array where each index is a permission level and the element is a region. But if we look at say unix permissions, we have a structure where the position is the user/group and the value is the permission level (and this is a bit-field, being able to set read | write permissions).

What if we had something like this:

enum class AccessRights { none, read_if_set, read, write, final_ };

enum class PermissionRegions { interior, boundaries };

struct AccessPermission {
  AccessRights interior = AccessRights::none;
  AccessRights boundaries = AccessRights::none;

  auto operator[](PermissionRegions region) {
    switch(region) {
    case PermissionRegions::interior:
       return interior;
    case PermissionRegions::boundaries:
       return boundaries;
    }
  }
};

Is this way round easier to reason about? It feels a lot simpler. To take the examples from the AccessRights docstring, we'd now have:

auto only_read_if_set = AccessPermission {
  .interior = AccessRights::read_if_set,
  .boundaries = AccessRights::read_if_set
};

auto read_only = AccessPermission {
  .interior = AccessRights::read,
  .boundaries = AccessRights::read
};

auto write_boundaries = AccessPermission {
  .interior = AccessRights::none,
  .boundaries = AccessRights::write
};

auto read_write_everywhere = AccessPermission {
  .interior = AccessRights::write,
  .boundaries = AccessRights::write
};

auto final_write_boundaries_read_interior = AccessPermission {
  .interior = AccessRights::read,
  .boundaries = AccessRights::write
};

Checking if a AccessPermission is valid for a given region and access level is just:

read_only[region] >= AccessRights::read

This way round would of course also work with an array, but I keep suggesting structs because names are really important, and the lack of them in the current design makes e.g. applyLowerPermissions very difficult to parse. I'm not even sure it's needed this proposed design.

Applying the higher of two permissions is easier:

auto result = AccessPermission {
  .interior = std::max(lhs.interior, rhs.interior),
  .boundaries = std::max(lhs.boundaries, rhs.boundaries),
};

Originally posted by @ZedThree in #421 (review)

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions