Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: add RFC for modular modules RFC #14

Merged
merged 9 commits into from Feb 27, 2024
Merged

docs: add RFC for modular modules RFC #14

merged 9 commits into from Feb 27, 2024

Conversation

aaguiarz
Copy link
Member

No description provided.


Having all teams to maintain a model in a single file has several drawbacks:

- Requires a more comprehensive PR process where all teams need to be involved.
Copy link
Member

@jon-whit jon-whit Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me how this RFC proposes an alternative to this bullet point necessarily, because we don't explore what the intended workflow/experience should be like for developers. Do teams define their own repository with their module in it and then the company defines a single repo with git submodules stitching them all together? Or does an organization define a single repository with different CODEOWNERS assigned to different module files in that repository and any change to the repository rolls out a new FGA model for the organization with the modules stitched together?

If a team updates their module shouldn't that be immediately reflected in the top-level module that imports it? Or does the parent-module pin to a specific release of a team's module? Is there an extension to this RFC that isn't mentioned about module versioning etc..?

With modules as described in this RFC, teams will still need to maintain strongly coupled releases. If a teamA updates their module they will also have to update the organization repository submodule that references it and this will have to be rolled out synchronously relative to each other. If what I have described herein is also the intended experience, then I don't know that we're necessarily reducing the PR process where all teams need not be involved, we'd merely be seperating a model out into modularized components for the sake of cognitive structure, but not organizational process.

Lets make this story more clear in this RFC.

Copy link

@jerclark jerclark Dec 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered this scenario and resolved to the "single-repository with CODEOWNERS" approach. The idea then might be for applications to attach that single repository as a submodule in their app-specific repository.

When considering the notion of freely importable "modules", I began to think about challenges with circular references and conflict resolution. Given the objective of 'partitioning' responsibility amongst developments teams, I didn't think that a full import/extension model was necessary. Instead I came up with the following enhancements to the OpenFGA language:

compositemodel
file: atlassian.composite.fga

compositemodel
  schema 1.1

submodel "submodels/core.sub.fga"
submodel "submodels/confluence.sub.fga"
submodel "submodels/jira.sub.fga"

submodel
file: submodels/core.sub.fga

// "submodels/core.sub.fga"
submodel
  schema 1.1

compositemodel "../atlassian.composite.fga"

type user
type organization
	relations
		define member : [user]
		define admin : [user]
type group
	relations
		define member : [user]

file: submodels/confluence.sub.fga

// "submodels/core.sub.fga"
submodel
  schema 1.1

compositemodel "../atlassian.composite.fga"

type space
  relations
    define organization : [organization] 
type page
  relations
    define space : [space]
    define owner : [user]

file: submodels/jira.sub.fga

// "submodels/core.sub.fga"
submodel
  schema 1.1

compositemodel "../atlassian.composite.fga"

type project
  relations
    define organization : [organization] 
type ticket
  relations
    define project : [project]
    define owner : [user]

Each submodel is associated with exactly one composite model, and the submodel will be assigned in the CODEOWNERS file.

The idea is then to "compose" the compsite model into a standard model file by stitching together everything below the "compositemodel" directive in the submodels (types, conditions). No need to update the openfga server to support any new model syntax.

I extended openfga/language and openfga/vscode-ext to get a working POC.

  • Updated openfga/lanugage to support the syntax extensions noted above
  • Updated openfga/vscode-ext to use the updated language package
  • Updated openfga/vscode-ext with the following functional ehnacements:
    • When a submodel is modified, the vscode language-server constantly 'recomposes' the composite model and uses the fully composed compsite model to validate the submodel.
      • So, for example, if I add a type is a submodel that exists in a sibling submodel, the validator will catch that and display it in the current submodel's editor.
      • The trick here is for the language server to compose the model with the submodel-under-edit FIRST, so the validations map to the appropriate character ranges in the editor. This is a bit hacky and could be improved.
    • Snippets added for new syntax. Type snippets also work 'across' submodules - so typeahead works for types defined in sibling submodules.
    • Added a "compile" model action to the vscode extension which will output a composed model file from the composite model (This needs to be added to cli for dev-ops usability)

I've attached an (unnarrated) video of how the POC operates in vscode right now.

And while the code changes I made are definitely not production ready, I'm happy to open PRs on the language and vscode-ext repos with the changes I made - let me know if you'd like that.

composite-model-poc.mp4

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jerclark Thanks for this! ❤️

I think it would solve well the problem we want to solve initially, which is having multiple teams owning their own authorization model.

We want to build a related feature (no RFC yet) to give permissions to specific applications perform specific actions in each 'submodel', e.g. the client_id for Confluence can't write a tuple for Jira's 'ticket.owner'.

We'd need a way to name each submodel in order to do that, or instead of managing permissions per 'submodel', manage them per type, which will imply managing more permissions.

Copy link

@jerclark jerclark Dec 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback @aaguiarz...I have followed this issue openfga/roadmap#30 closely, as I think "AuthZ for AuthZ" is a universal problem. To deal with this for now, my plan is to use an envoy proxy in front of OpenFGA which will filter mutating API calls through an 'proxy-authorizer' service. The proxy-authorizer service is a client of the same openfga stores being accessed via the original API calls, and will operate on many of the same principles put forward by @le-yams here. The model must include well-known "control relations" (I really like that term :) and the proxy-authz service will check whether the requesting app and/or user has the control relation which allows adding the tuple. As mentioned by you and @le-yams - this could complexify the models/permissions...but I think the rule could be stated simply as "Any directly assignable relation must have a control relation"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jerclark yes, that what's customers have been doing up to now, but we'd love to find a way that does not require you to solve it yourself :)

BTW, I'd love to learn more about how you are using OpenFGA, please reach out to andres.aguiar at openfga.dev if you are up to it :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I'd love to learn more about how you are using OpenFGA, please reach out to andres.aguiar at openfga.dev if you are up to it :)

Sent you an email!


- Define a modules with a set of types that reflect the resources/permissions for an app/service.
- Be able to reference types in other modules.
- Be able to add relationships to types in other modules. This is something that might not be needed and will be discussed later.
Copy link
Member

@jon-whit jon-whit Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me that is a bit out of scope of this work. This feature eludes to a templating/extension mechanism that aims to improve the modular modeling experience, but it seems like it could be potentially counter productive. We want to avoid type definition duplication and encourage ownership by creating clear ownership boundaries across modules. If a relation needs to be added to a type definition, then that should be an organizational conversation that takes place between the teams collaborating across two different modules owned by two different domains. I actually view this as a positive thing and one of the powers behind Zanzibar inspired systems in the first place. You create cross-domain authorization (e.g. "global platform authorization") by encouraging these ownership boundaries and collaborating across them. Giving teams the ability to override/extend another teams module breaks down module boundaries and the clear ownership boundaries between them, and this sort of undermines the objective in the first place.

I think we need to discover this a bit more before embarking on this feature, and I think we can get started with modular models without it initially.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with the above. If a relationship needs to be established across separate modules, it should be a litmus test that the modules are not truly modular and should be defined in "core".

Adding this level of flexibility improves the expressiveness but I'd argue that it runs counter to the DX goals of this RFC. Not to mention that by removing this point, we'd drastically simplify the problem.

@aaguiarz aaguiarz changed the title First draft of the modular modules RFC Modular modules RFC Dec 14, 2023
@aaguiarz aaguiarz marked this pull request as draft December 14, 2023 15:01
Copy link

@le-yams le-yams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to not prefix the types with the module names? (the prefixes make the model reading a lot heavier)


## Open Questions

- Should we allow adding relations to types from other modules?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we allow adding relations to types from other modules?

That's something we would definitively be interested in. We have differents teams working on (and owning) different domains of our app. So we would like to separate the model definition according to these domains. Even for a given type, relations are domain related so it would make sense for us to put them in their dedicated module

the following module is maintained by the core team:

module core

type user

type organization
  define member : [user]
  define admin : [user]

the following module is maintained by the payments team:

module payments
import "core.fga"

extends type core.organization
  define can_create_payment : [core.user] or admin

the following module is maintained by the invoices team:

module invoices
import "core.fga"

extends type core.organization
  define can_mark_invoice_as_verified : [core.user] or admin

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@le-yams Thanks!

Two questions:

  • In that specific use case, would you allow a direct assignment, e.g.?
extends type core.organization
  define can_mark_invoice_as_verified : [core.user] or admin

vs

extends type core.organization
  define can_mark_invoice_as_verified : admin
  • Assuming you could have a different application IDs for the 'core' team and the 'payments' team based on that application ID, you define who can write tuples in each module, in your specific use case, which application would write tuples for the can_create_payment relation? The core team's application? The `payments' team application?

Copy link

@le-yams le-yams Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaguiarz

First question:
Absolutely yes. Some users (not admin) can be given that permission by a direct assignment.

Second question:
That is a very good question and indeed I would say that, as the can_create_payment relation belong to the payments domain, the domain team is the owner of that information and so it should be the only one allowed to "manage" (write/delete) related tuples.

Who should write/delete which tuple/relation is a subject we currently are studying before implementing more use cases. We already draft some ideas, this will probably go further the scope of this rfc but perhaps it will give you ideas so i'll share it here.
In our case we are taking advantage that we implemented a facade service which is the only one to "speak" with openfga.
All other business services speak to the facade using openid authentication.

For now (our model is still pretty small) only a single service manages the tuples (other services only perform checks). But we have a lot of pending use cases and we'd like other services to be able to perform tuples write/delete providing enough isolation and reduce security hazard (kind of like what you mentioned in your question).

So here are the conditions we are thinking about to implement in our facade.

First, only one service can call the facade with an access token coming from a client_credentials authentication flow. It is the one to setup the initial administrator relation on an organization (that's basically what we already have).

To all other cases, we believe that no application is allowed to give permissions. Only end users are (or not) according to their own permission(s).
To do so, all other service must provide 2 access tokens:

  • one comming from a client_credentials authentication flow (this is to ensure that the caller is an authorized service)
  • forward the user access token (coming from an authorization_code flow performed through our frontend) so we have the actual user id

When such a call is made to the facade, it will lookup for "control relations" based on the query information and using the user access token sub (end user id).
Those control relations would follow some convention like:

  • can_<create|delete>_<relation> if the relations are defined on the same type of the query relation
  • can_<create|delete>_<relation>_<objectType> if the relations are defined on the type of the object of the query

Here is an authorization model that should illustrate it:

type user

type organization
  relations
    // managed by the core service
    define administrator: [user]

    // here control relations are defined on the same type
    define entities_administrator: [user]
    define can_create_entities_administrator: administrator  // checked by the facade when a end user tries to write an 'entities_administrator' relation on this organization
    define can_delete_entities_administrator: administrator  // checked by the facade when a end user tries to delete an 'entities_administrator' relation on this organization

    // here are control relations for the relation 'organization' defined in the type 'entity'
    define can_create_organization_entity: entities_administrator  // checked by the facade when a end user tries to write an 'organization' relation on an entity having this organization as object
    define can_delete_organization_entity: entities_administrator  // checked by the facade when a end user tries to delete an 'organization' relation on an entity having this organization as object


type entity
  relations
    // control relations are defined in the object type organization
    define organization: [organization]

    define administrator: entities_administrator from organization

    // here control relations are defined on the same type
    define manager: [user]
    define can_create_manager: administrator  // checked by the facade when a end user tries to write a 'manager' relation on this entity
    define can_delete_manager: administrator  // checked by the facade when a end user tries to delete a 'manager' relation on this entity

    define can_do_stuff: manager

In this example:

  • only the core service can define an organization administrator (basically when a user creates a new organization)
  • only an organization administrator can appoint a user as entities administrator
  • only an entities administrator can create entities within the organization
  • an entities administrator of an organization is the administrator of the organization entities
  • an entity administrator can appoint a user as entity manager
  • an entity manager can do stuff on the entity

I don't known if all that makes sense to you (it might not be all interesting for the current discussion ^^). And as I said this is a draft (we don't know at which point this is scalable with a more complex authorization model).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the context @le-yams!

My understanding is that you are using the "control relations" approach for user identities. E.g. "you need to be an administrator to create a a manager", and that makes sense. I see that as a relatively common pattern when implementing FGA. To create specific instances of resources in your app, and the corresponding tuples, you need to have permission to do it.

What we are trying to figure out is how to manage that for application identities too. In the access token you usually have a claim for the end user (sub) and another for the authorized party (azp) which would usually be a client_id.

In the Atlassian example, a user would want to delete a space and be authorized to do it, but the Jira applications shouldn't delete the tuple, only the Confluence one. In that case, even if the user is authorized, deleting the tuple would fail.

Using your approach, it could be something like:

define can_delete_manager: [application] or administrator

The Facade can now check if `application:' can_delete_manager.

In the case of the end user, it can be more tricky to do it fully from the facade, because at that point you are already writing the tuple, and it's likely that you want to stop the user earlier (e.g. when the DELETE manager/ API call is executed, you'll check if the user can delete a manager).

Makes sense?

@aaguiarz aaguiarz changed the title Modular modules RFC docs: Modular modules RFC Dec 20, 2023
@aaguiarz aaguiarz changed the title docs: Modular modules RFC docs: add RFC for modular modules RFC Dec 20, 2023
Copy link
Contributor

@willvedd willvedd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, solid RFC. This problem has a lot of surface area but its clear that a lot of thoughtful consideration has been poured into it.

My initial thought is that this modular system has the potential to be highly complex. Perhaps we should consider trading some flexibility for a simpler system.

Proposal:

  • the import keyword injecting the contents of the module w/ a very thin processing layer
  • global namespace for all modules and the consuming "core" model
  • implicit extensions of "core" types when defined by modules

Example:

// model.fga
model
  schema 1.1

import confluence.fga
import jira.fga

type user
type organization
  relations
   define member : [user]
   define admin : [user]
type group
  relations
    define member : [user]
// confluence.fga
model
 schema 1.1

type organization
  relations
    define can_create_space : [user] or admin

type space
  relations
    define organization : [organization] 
type page
  relations
    define space : [space]
    define owner : [user]
// jira.fga
model
  schema 1.1

type project
  relations
    define organization : [organization] 
type ticket
  relations
    define project : [project]
    define owner : [user]

By eliminating scoped namespaces, we can freely inherit types from the parent and across modules. It also has the added benefit of forcing uniqueness across the organization, preventing teams from creating multiple user types, for instance.

Having all teams to maintain a model in a single file has several drawbacks:

- Requires a more comprehensive PR process where all teams need to be involved.
- The model can become large and difficult to understand.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This is subjective. There is a possibility that distributing the model across several modules that themselves import sub-modules would be more difficult to comprehend than a single file.

Perhaps this point can be rewritten to focus on the end-developer on a functional team.
Ex: "The model can become noisy and mostly irrelevant to end-developers on specific product teams"


In addition to enable different developers maintain their own modules, it should be possible to enable different applications to perform different actions in different modules.

For example, applications belonging to the domain which owns a module should be able to write/delete tuples of the object types the module defines, but other applications outside of that domain should only be able to query relationships of those object types.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought: ostensibly a good idea but I question wether this is something we'd want to include in this RFC. It doesn't seem like a global requirement and can think of some counterexamples. Ex: creating a Confluence document directly from a Jira epic.

Nor do I think that "actions" should be governed on the module-level anyway. The objective of this RFC is to improve DX of product teams, introducing a permission layer atop it seems to add to the cognitive load.


- Define a modules with a set of types that reflect the resources/permissions for an app/service.
- Be able to reference types in other modules.
- Be able to add relationships to types in other modules. This is something that might not be needed and will be discussed later.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with the above. If a relationship needs to be established across separate modules, it should be a litmus test that the modules are not truly modular and should be defined in "core".

Adding this level of flexibility improves the expressiveness but I'd argue that it runs counter to the DX goals of this RFC. Not to mention that by removing this point, we'd drastically simplify the problem.

Comment on lines 90 to 93
- A way to name a module
- A way to import a module
- A way to reference a type/relation from another model
- A way to add relations to types from another model
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if we wanted to enable those use cases, we may not need these. These points describe an implementation that introduce scoped namespacing. If instead we solely relied on a global namespace, we could drastically reduce the complexity here.

Not saying that relying on a global namespace is an ideal solution though, will comment more on that below.

define owner : [core.user]
```

## Proposed Implementation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: How would we express versioning of submodules? If we're assuming that the modules are distributed across separate repos or otherwise can't be solved by a CODEOWNERS. Or are we assuming that the modules always exist within a single repo?

Perhaps first we need to define recommended workflow practices for product teams.

20231212-modular-models.md Outdated Show resolved Hide resolved
Co-authored-by: Will Vedder <willvedd@gmail.com>
@rhamzeh
Copy link
Member

rhamzeh commented Jan 8, 2024

What if we separate the issues into three:

  • splitting into files
    We can go with something like Go, where the files need to be in the same package

    So we can have a folder:

    folderA/
     - file1.fga
     - file2.fga
     - file3.fga
     - ...
    

    And so long as the files are all part of the same package (we allow adding package <name> in each file), our CLI and VS Code extensions will just work

    importing from the same package does not have a lot of utility. Everything in the same package should be able to use anything else.

    E.g. jira.fga does not need to import base.fga if both of them are in the atlassian package

  • namespacing
    Maybe we can introduce a character to use as a divider

    module1 and module2 are basically just prefixes for types.

    So all a user needs to do is write

    type module1.user
      relations
        ...
    type module2.user
      relations
        ...
    type module3.submodule1.user
      relations
        ...
    

    if they declare the module at the top of the file, the DSL will automatically prefix it to all types in the file

    When permissions come in to play, users would be able to grant permission based on module or submodule or even type

  • importing
    at some point we can allow importing other packages/modules from other folders and even urls

@le-yams
Copy link

le-yams commented Feb 26, 2024

When permissions come in to play, users would be able to grant permission based on module or submodule or even type

We started to add permissions for tuples writing in our system. Those permissions currently are enforced by our facade (none of our services can directly call OpenFga) and are mainly based on the relations.

Our teams and services are very domain related so for example, on a given type company we have several relations bounded to very differents domains (can_invite_user, can_do_bank_stuff, can_do_invoices_stuff, ...).
We (especially our security team ^^) don't want the service A to be able to write relations related to service B domain and vice versa (even if they belong to the same type).

Basically we associated each type:relation to a list of OAuth2 scopes. When the write endpoint is called, the facade checks that the calling service has (in its access token) all the scopes required for the different type:relation present in the query.

20231212-modular-models.md Outdated Show resolved Hide resolved
Co-authored-by: Raghd Hamzeh <raghd.hamzeh@auth0.com>
@aaguiarz aaguiarz marked this pull request as ready for review February 27, 2024 15:58
@aaguiarz aaguiarz merged commit b427511 into main Feb 27, 2024
1 check passed
@aaguiarz aaguiarz deleted the modular-models branch February 27, 2024 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants