Refactor rules to accept interface instead of string for resource

[darh]

Simplify rules.Resource struct and refactor all fucs that accept it

[titpetric]

I'm gonna look at the API's (internal checkAccess) if we can drop the .String() and just use resource.

I have to git blame the resource struct, I remember only needing three things there - ID of the record (uint64), Name (channel, module,...), and Scope (crm, messaging,...). This gives us a globally unique resource ID (the fmt.Stringer interface provides it), which is %s:%s:%d, so there's definitely one field off here...

[titpetric]

Ok, researched a bit:

return fmt.Sprintf("%s:%s:%d", r.Service, r.Scope, r.ID)

// Resource returns a system resource ID for this type
func (r *Organisation) Resource() rules.Resource {
    resource := rules.Resource{
        Service: "system",
        Scope:   "organisation",
    }
    if r != nil {
        resource.ID = r.ID
        resource.Name = r.Name
    }
    return resource
}

Name was added to hold a readable name of the resource that's being managed. That way the user would know, that they are managing something like Facebook and not system:organisation:%1. I don't see that we can drop any fields from here. If we don't need that, then I suggest:

1. Drop Name,
2. Rename Scope into Kind

@darh thoughts and additions re: simplify rules.Resource?

[darh]

Our initial assumptions regarding permission changed in the last couple of weeks and with that, a lot of code is now redundant (see #96)

I would go as far as removing rules.Resource and replacing it with a string type. Rationale behind this is:

• we're doing string manipulation (through split/join) anyway (parts := strings.Split(resource, delimiter))
• there is no need for doing any kind of differentiation & filtering resources, all we're doing is a bit of manipulation to replace resource ID with an asterix inside resources.Check

const organisationResource = "system:organisation:"

func (r Organisation) Resource() string {
  return organisationResource + r.ID
}

Why not ptr? Because if we're always talking about a specific resource here and as such, it is always an instance with a valid ID/identifier.

For 1st level we can go with this:

type System struct {}
const systemResource = "system"
func (System) Resource() string {
  return systemResource
} 

and so on...

---

Re: renaming: I would ditch the Scope term because we do not use it anywhere. Even the proposed Kind is not an appropirate choice -- we use it for something else. It is simply a resource.

---

[darh]

let me extend that one a bit...

// pseudo, untested code

type (
    Resource string
)

const delimiter = ':'

func (r *Resource) AppendID(ID uint64) {
    if !r.Final() {
        // I'm ok with that as I'm ok with "index out of range" panics
        panic("can not append ID to non-final resource " + r.String())
    }

    *r = Resource(r.String() + strconv.FormatUint(ID, 10))
}

func (r Resource) String() string {
    return string(r)
}

// + func that replaces ID with asterisk for the needs of resource.Check 

func (r Resource) Final() bool {
    return len(r) > 0 && r[len(r)-1] != resDelimiter
}

Then, constants under different services:

const SystemResource = Resource("system")
const MessagingResource = Resource("messaging")

// under channel
const ChannelResource = Resource("messaging:channel:")

// under compose
const ModuleResource = Resource("compose:module:")

And finally, how channels use these resources

func (c *Channel) Resource () Resource {
    return ChannelResource.AppendID(c.ID)
}

[titpetric]

I think we have to go a few steps backwards:

• The intent of a Resource{} type is to designate a database record. This means that having a "SystemResource" is already a violation of the intent here.
• Resource has .All() to produce messaging:channel:* for a given channel resource (with or without a provided ID field). This should have preference over string.Split + "*" example mentioned above.
• Resource has a readable Name for the record, which is a front-end helper field, so the user would know they are editing channel Name #general instead of messaging:channel:1 specifically. Are we solving this differently today?
• Internally we are not forced to use string types for passing resources, we could just use the Resource{} all the way until the responsible package where we actually need the ResourceID (currently a fmt.Stringer). We just convert this too soon, instead of passing it along.
• non-record resources ("system", "messaging" and "crm"): we can assert a logic change that a resource may be either [Service], [Service, Kind], or [Service, Kind, ID] (partially implemented already), based on their nil/empty values. SystemResource today is already constructed as Resource{Service: "system"}. If anything, we can be more strict, and enumerate the values for Service into a ResourceService type to not allow "anything goes".
• Encoding logic for the current Resource{} ID must be contained in the implementation for the type. Moving to string and basically using CSV by convention puts the responsibility from the code to the developer, and we know that enforcing things by convention is bad.

I think we need to do:

1) pass Resource instead of String higher up in app-specific check functions to the internal apis, 2) rely on Resource methods to provide the Resource ID or wildcards (drop strings pkg and delimiter implementations outside of Resource) 3) consider dropping Name if we don't need it for the front-end anymore. If we only need the ResourceID for the front-end, update the marshalling function to only produce the ID output as string

[darh]

Agreed on Resource string and typed resources (const SystemResource = Resource("system") ).

[titpetric]

Implemented