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

Add generic "resource" endpoint #102

Closed
paul121 opened this issue Nov 17, 2020 · 2 comments
Closed

Add generic "resource" endpoint #102

paul121 opened this issue Nov 17, 2020 · 2 comments
Labels
2x farmOS 2x support

Comments

@paul121
Copy link
Member

paul121 commented Nov 17, 2020

Since farmOS 2x will have many more entities exposed via the API as JSONAPI resources, it becomes pretty trivial to support these additional resources. Entity permissions are respected.

farmOS.py will have a client.resource.get() method which can be used to request these other resources (files, data stream, roles, etc.). If no bundle is provided, farmOS.py assumes the resource name is entity--entity eg: file--file.

@paul121 paul121 added the 2x farmOS 2x support label Nov 17, 2020
paul121 added a commit that referenced this issue Mar 4, 2021
@paul121
Copy link
Member Author

paul121 commented Mar 4, 2021

This is largely done... we have the following:
GET, PUT, POST, DELETE: /api/v2/farms/resources/{entity_type}/{bundle}

And a separate endpoint to get a single resource by ID:
GET: /api/v2/farms/resources/{entity_type}/{bundle}/{uuid}

After adding the additional endpoint to get by ID, I'm curious about changing the PUT and DELETE to require a uuid path parameter as well 🤔 At first glance this seems like an improvement, but perhaps not...

Internally, an ID is only a required parameter for the farmOS.py .delete() method. The .send() method is responsible for POST and PUT. Also, the aggregator currently supports delete multiple IDs in a single request. This wouldn't be possible as a path parameter. The separate GET endpoint is needed here for the same reasons described in farmOS/farmOS.py#42


Entity permissions are respected.

This reminds me... permissions are respected on the farmOS server, but in the Aggregator we created separate OAuth scopes for accessing logs, assets, terms and areas:

oauth_scopes = {
    "farm:create": "Create farm profiles",
    "farm:read": "Read farm profiles",
    "farm:update": "Update farm profiles",
    "farm:delete": "Delete farm profiles",
    "farm:authorize": "Complete the OAuth Authorization Code cycle for farm profiles",
    "farm.info": "Read farmOS server info",
    "farm.logs": "Access logs",
    "farm.assets": "Access assets",
    "farm.terms": "Access terms",
    "farm.areas": "Access areas",
}

This doesn't work very well with a single resource endpoint. It would be possible to check the entity_type and/or bundle path parameter to determine access but we are limited to the OAuth scopes we hard code in. For example, 2.x introduces additional resource types such as quantity, data_stream, flag, user, and various others... I don't imagine we want scopes for all of these? It might be possible to allow the Aggregator instance to create OAuth scopes for specific resources on the fly, but that would be quite the enhancement (another part of the Aggregator API/DB model).

Another complication is that it's possible for a request to one resource type to include data from another resource type. The access_token granted by the farmOS server to the Aggregator should prevent include from returning any data the user doesn't have access to. But once the Aggregator is authorized, it becomes much more complicated to implement this level of entity-type access control to those using the Aggregator API.

That said, we can still introduce scopes to limit the CRUD operations an Aggregator user/api-key has (only read resources, vs create and read resources). Anything beyond this and the Aggregator really turns into something "delegated to grant further access" - I think this is outside the scope of OAuth2 & the farmOS -> Aggregator capability :-)

But its worth re-iterating why we added scopes + api-keys... largely as another security measure so that a service using the Aggregator API only has the minimum access needed (likely doesn't need the ability to DELETE farms from the aggregator, perhaps only GET resources from farms).

At the very least we need a farm.resources scope to replicate farm.{entity_type}. Should we add the following CRUD scopes?

resource_scopes = {
    "farm.resources.get": "Read farmOS resources",
    "farm.resources.put": "Update farmOS resources",
    "farm.resources.post": "Create farmOS resources",
    "farm.resources.delete": "Delete farmOS resources",
}

Curious for your thoughts @mstenta

@paul121
Copy link
Member Author

paul121 commented Mar 19, 2021

We decided to remove all of the "resource" related scopes. Removing the old farm.log, etc scopes should not create a problem since they were not being used by our main use case.

@paul121 paul121 closed this as completed Mar 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2x farmOS 2x support
Development

No branches or pull requests

1 participant