crusttech / crust-server

Apache License 2.0
87 stars 21 forks source link

Permission API for RBAC #45

Closed mitjaziv closed 5 years ago

mitjaziv commented 5 years ago

Changes defined today:

1. API Permission definition: API entrypoints should be under system as we are editing permissions for messaging, crm and system.

/rbac/
        POST   -> add role
        GET    -> get all roles
/rbac/{roleID}
        DELETE -> remove role
/rbac/{roleID}
/rbac/vodovodarji15/rules
        POST -> [ APPEND rules
            { object: "messaging:channels:*",  operation: "update", permission: "GRANT" }
            { object: "messaging:channels:1",  operation: "update", permission: "REVOKE" }
            { object: "messaging:channels:2",  operation: "update" } <<<--- removes, this be "inherit"
            { object: "system",                operation: "organisation.create", permission: "GRANT" }
            { object: "system:organisation:*", operation: "update",           permission: "GRANT" }
            { object: "system:organisation:*", operation: "delete",           permission: "GRANT" }
            { object: "messaging:channel:*",   operation: "update.name",      permission: "GRANT" }
            { object: "messaging:channel:*",   operation: "update.topic",     permission: "GRANT" }
            { object: "messaging:channel:*",   operation: "members.manage",   permission: "GRANT" }
        ]
        GET -> returns all defined permissions for specific role 
        DELETE -> remove all define permissions for specific role

2. Current permission hierarchy:

system (ie admin)
    - organisation.create
    - team.create
    - role.create
    organisation:X
        - update
        - delete
    team:X
        - update
        - delete
    role:X
        - update
        - delete

messaging
    - channel.create
    channel:X
        - update
        - delete
        - send messages
        - pin messages

compose:X
compose:crm
    - module.create
    module:X
        - update
        - delete
        - create records

3. Rules entity definition: value = grant|revoke operation = object = [:"*"|]

roleId + object + operation + value

titpetric commented 5 years ago
  1. System isn’t a parent to other microservices. While it does have some overlap (org, team), it most definitely shouldn’t provide messaging or crm specific output, but only output specific to the system (manage orgs and team operations). This is not an area for denormalization, but we can think on reuse further. The main concern currently seems to be ensuring the same API signature over the microservices, and joining them into one creates other concerns (tight coupling of system against everything else - resources need validation from messaging or crm on save).

Other:

  1. Proposal is unclear. The structure LGTM but it obviously has some verbosity omitted.

For example:

Clarify any suggestions here.

  1. The only sense this is making is consitently either using team or role naming. Currently we’re using team for the sql table name, for the relationships, for the fields and the front end. Correct me if I’m wrong (there’s a good chance I might be), but if we’re already using “Team” consistently, this can be just fixed with a glossary doc as already suggested in #34 ;

Additionally: the existing naming we have here is already good. Using “resource” is a good distinction from the more generic object. We can even say not every object (e.g. message) is a resource and get that semantic distinction between Go objects and RBAC resources in future discussions.

darh commented 5 years ago
  1. we will keep the system as a fallback for things that do not belong to messaging/compose
  2. grant/revoke follows RBAC ANSI terminology.
  3. resource => object, follows RBAC ANSI terminology.
  4. this is not a proposal, it is a done thing and mostly implemented. will throw UI over it today/tomorrow.
titpetric commented 5 years ago
  1. What is this referencing?
  2. And 3.: No need to follow RBAC ANSI, not into renaming things just to rename them. ACL rules are understood. We’re not forcing didmos terminology changes on their API either. Consider a larger scope to see that RBAC is just a part of the system and not a whole. Hard no on these.

On Thu, 21 Feb 2019 at 10:47, Denis Arh notifications@github.com wrote:

  1. we will keep the system as a fallback for things that do not belong to messaging/compose
  2. grant/revoke follows RBAC ANSI terminology.
  3. resource => object, follows RBAC ANSI terminology.
  4. this is not a proposal, it is a done thing and mostly implemented. will throw UI over it today/tomorrow.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/crusttech/crust/issues/45#issuecomment-465896182, or mute the thread https://github.com/notifications/unsubscribe-auth/AAOPkAVedY0GQYOc2F-_6GgyrqBRkPSLks5vPk75gaJpZM4bGD2A .

darh commented 5 years ago

Implemented.