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

RESTful API design #582

Open
wvengen opened this issue Oct 19, 2018 · 28 comments
Open

RESTful API design #582

wvengen opened this issue Oct 19, 2018 · 28 comments
Labels

Comments

@wvengen
Copy link
Member

wvengen commented Oct 19, 2018

As talked about in #572, there is a need for a RESTful API which allows access to all relevant models, for both 'member' and 'admin' tasks. Right now the PRs that found their origin in #429 only include access to a member's resources (those relating to its user and ordergroup).

If we ever want to do admin things using the API (the answer would be "yes"), we'll need to be able to work with in an API. This issue is about discussing how this API would look like.

Some important decisions to make are (feel free to add more):

  • Will there be separate endpoints for roles like 'member'- and 'admin'-functionality (the former is what every member in the foodcoop needs to do as themselves, the latter is about managing orders, editing other users, groups, configuration, etc.)?
  • What are the endpoints we need / want to expose?
  • Where do we use nested endpoints, and where not?
  • What authorization scopes do we need?
  • How will authorization and scopes be applied? (i.e. does what resources you see from an endpoint depend on your access level incl. the scopes you requested with OAuth, or does it depend on the parameters with access denied when not allowed)
  • ...

See More or less per resource in #582 (comment) for what we chose for scopes.

@wvengen wvengen added the api label Oct 19, 2018
@wvengen
Copy link
Member Author

wvengen commented Oct 22, 2018

'Member' vs. 'admin' functionality

Almost all users want to order with a foodcoop, and many users perform tasks within the foodcoop, of which some need 'admin'-access (like updating member details, editing orders, creating financial transactions, etc.). We'd like to be able to do all of this using the API.

When a member is ordering for himself, he only needs to see the articles in the currently open orders. He only must update group order articles for his ordergroup. When viewing his ordergroup's account statement, he should only see the ones for his ordergroup (not others). But when doing a workgroup tasks, someone in the orders workgroup needs to see all members' group order articles, and someone doing finances must be able to see all (relevant) member's transactions.

How does this difference between 'member' and 'admin' functionality work in the API?
There may be several approaches to this.

Let the client work it out

If the client knows which access rules apply, it can give the desired query parameters. So only show open orders in the member ordering app. Only show the ordergroup's group order articles. In the boxfill phase, only allow updating group order articles that result in equal or more order result, and check minimum balance in the front-end as well.

Putting these checks in the front-end is highly undesirable. There must be some way for the API to enforce member-related checks, but allow admins to modify things that members can't.

(Note: It may be possible to avoid we the issues mentioned here -- by not allowing admins to update a group order article's quantity and tolerance, only the result -- but the general idea that an admin-as-member may have different behaviour than admin still holds, even if we may happen to be able to avoid it right now.)

Access token scopes

Authorization can (and must) happen by access token scope. The default scope would be members-only, which is what front-end apps use when doing things 'as a member'. Admin applications would request the scope applicable for what it needs to do.

If there would be a single app with both member and admin functionality, it would need to handle different access tokens and make sure to use the correct one. I haven't seen this pattern elsewhere, and I can see client developers accidentaly choosing the wrong access token.

Scope as query parameter

Use a query parameter to indicate if one would want to do something as 'member' or as 'admin' (SO idea). Default would be 'member', I guess.
Another option may be a custom header, but that seems a bit of a hidden way to do so, I'd favour a query parameter.

Different endpoints

Member-related endpoints only return the member's and ordergroup's records, admin-related endpoints work on all records. Authorization is different for both, but they return the same objects (except for fields that may be visible for admins only). This would result in some code duplication, because there are basically two APIs (controllers, docs, specs - models and serializers can be shared).

@wvengen
Copy link
Member Author

wvengen commented Oct 27, 2018

Regarding 'member' vs. 'admin', I propose to use access token scopes.

  • Use the same endpoints.
  • An access token needs either the member or the admin scope (exclusive). If no or both scopes are requested, all access is denied (or optionally the token wouldn't be returned at all).

This allows us to have clear API endpoints, and start implementing the member-part without being in the way for admin-parts.

@wvengen
Copy link
Member Author

wvengen commented Oct 27, 2018

Scopes

This is a list to get started. As the API develops, we may want to add or refine some scopes (e.g. currently the finance permission allows financial transactions, bank accounts and balancing, but since balancing works on orders and group orders, perhaps it's better to split that, not sure - let's not worry too much until we start implementing that, if ever).

  • member:all - all member scopes (later we can introduce more fine-grained scopes if we want)
  • admin:all:read - access to all endpoints, but don't allow changing things
  • admin:all:write - everything
  • admin:config:read - read all config variables (incl. those hidden to members)
  • admin:config:write - read + write all config config variables
  • admin:finance:read - read finance tasks
  • admin:finance:write - read + write finance tasks
  • admin:orders:read - read orders and order_articles
  • admin:orders:write - read + write orders and order_articles
  • admin:users:read - read all users and groups
  • admin:users:write - read + write all users and groups

Note that any member:* conflicts with any admin:*. When they are requested in a single access token request, either no access token is granted, or all access is denied when it is used.

The admin:all:* scopes are mostly for development convenience, so that one can get started easily without having to think too much about token scopes; I would discourage their use in production apps.

Endpoint URLs

Lists endpoints with required scopes. (#) = while accessible with multiple scopes, the admin scope would typically return more information than the member scope.

  • /api/v1/navigation
    • GET /api/v1/navigation - (any scope) - return navigation menu(s)
  • /api/v1/config
    • GET /api/v1/config - (any scope) - return configuration variables (#)
    • GET /api/v1/config/:key - (any scope) - return configuration variable (#)
    • PUT /api/v1/config/:key - (admin:config:write) - update configuration variable
  • /api/v1/users (here :id can be self to reference the current user)
    • GET /api/v1/users - (member:all, admin:users:*) - return user list (#)
    • GET /api/v1/users/:id - (member:all, admin:users:*) - return specific user (#)
    • POST /api/v1/users - (admin:users:write) - create new user
    • PUT /api/v1/users/:id - (member:all, admin:users:write) - update existing user (member only self)
    • DELETE /api/v1/users/:id - (admin:users:write) - delete existing user
  • /api/v1/orders
    • GET /api/v1/orders - (member:all, admin:orders:*) - list of orders (# admin gets comments)
    • GET /api/v1/orders/:id - (member:all, admin:orders:*) - get order (# admin gets comments)
    • ...
  • ...

Allright, I guess the rest would speak for itself / will become clear during development.

@paroga
Copy link
Member

paroga commented Oct 27, 2018

I personally like the "Let the client work it out" approach combined with "Access token scopes" the most. IMHO all endpoints must return the same structure of data: users with less privileges only get a subset of the data.

I don't like the difference between admin and member. Is the management of orders an admin task? IMHO it's not. In my understanding admin should be similar to the current admin role, which can change configuration and manage users/ordergroups.
I'd like to see scopes something like orders:order and orders:manage (similar to what we have now, maybe just a little bit more fine granted)

PS: I also don't like the idea of 'admin' and 'member' apps. I personally like to work on one uniform Foodsoft application, which implements everything in the (far) future.

@wvengen
Copy link
Member Author

wvengen commented Oct 31, 2018

IMHO all endpoints must return the same structure of data: users with less privileges only get a subset of the data.

Yes.

I don't like the difference between admin and member.

My terminology is perhaps confusing. I mean that there are different roles. All members want to order, look at the list of users, send messages, etc. Then most people will also be in a workgroup, and when performing a task as a member of that workgroup (like creating an order, or entering financial transactions, or dividing extra articles over members), what the API allows is different. So one accesses the API with a certain role, and depending on the API, you are or are not allowed to do certain things. So even when you are capable of having the managing-orders role, when you order articles like all members, this happens as role 'regular member'.

The access token scopes can function as selecting roles for a task.

I'd like to see scopes something like orders:order and orders:manage

That could be a useful angle. With clear descriptions of what they mean (like orders:order could mean ordering as a member at the foodcoop, but it might also mean ordering as a foodcoop at a supplier).
For ordering there are roles like viewing existing orders, creating and updating them, closing them, maybe more. Updating group order articles could be part of that or could be a separate scope. For group order articles, there is viewing your own, updating/creating/deleting your own (when allowed by your balance, apple points, etc.), reading anyone's, updating/creating/deleting anyone's (without restrictions). Would that be something like orders:(read|write):(self|all) with the implicit assumption that orders:write:self includes the restrictions and orders:read:all doesn't?

I personally like to work on one uniform Foodsoft application, which implements everything in the (far) future.

From an end-user perspective, yes. Internally it could be different apps sharing the same layout. But that's an implementation detail, I think.

@carchrae
Copy link
Contributor

i would be careful about simplicity in your rest api design that sacrifices simplicity in implementation. aside from a few geeks like ourselves, nobody is looking at the rest api routes, so even if the resource mapping is so elegant that there are the smallest number of routes (and much more logic in the controller and things that consume this rest data), it won't be appreciated by many, and i think even the developer will hate the bugs it can create. (i speak from my experience working with spree/solidus and the mess that they made in this manner)

so, my suggestion in terms of api design is for more routes so you can keep each controller a very simple 'crud' with no logic or checking.

for an access policy, specify which roles can access which routes (and what methods on that route) rather than changing the content returned depending on the role. (for code simplicity and client/consumer simplicity). yes, you will have more routes, but you can make more assumptions about the response from those routes, which keeps client code simpler, and less checking inside the controller (you are allowed or not).

@carchrae
Copy link
Contributor

also, i like the scoped access levels and i agree the current (old) rules aren't fine grained enough. for example, having 'finance' access lets you not only settle an order, but also record that someone paid some money to the coop. but maybe in some coop that is fine and we all know that few coops work in the same way. in practice it hasn't been a problem because every finance change has someones name on it (which is great).

@wvengen
Copy link
Member Author

wvengen commented Nov 22, 2018

What about using /api/v1/user/ordergroup, /api/v1/user/group_orders, etc. for relations scoped by the user, and /api/v1/config, /api/v1/navigation, /api/v1/orders, etc. for relations that are accessible to all roles? Modifying resources would require the correct roles/scopes, naturally.

There will probably be a small number of places left where the 'general' endpoints (not scoped by /user/) need to show/hide certain attributes based on access level/scope, but the most important distinctions are then separated (see #582 (comment)).

[This was inspired by GraphQL, where often viewer is used as the root query for the currently logged-in user, and other user-scoped objects are accessible from there. Though the same issues arise there as well.]

@wvengen
Copy link
Member Author

wvengen commented Nov 28, 2018

Another scopes proposal

I'm trying to integrate my and @paroga's ideas in how we'd use scopes. It's probably not perfect yet.
What level of granularity do we need? One one hand it's nice to allow apps to specify what access they
need, on the other hand simple is easier for everyone.

Two options follow.

Existing roles

Use the existing roles:

  • suppliers
  • article_meta
  • orders
  • finance
  • invoices
  • admin
  • and finally user - this is what everyone gets by default

More or less per resource

If we want more granularity, something like this could could make sense.

  • configuration
    • config:user - reading all configuration variables relevant to the user (default)
    • config:read - reading all configuration variables (except passwords and keys)
    • config:write - the above, plus writing all configuration variables (incl. writing passwords and keys)
  • users
    • user:read - reading your own user, ordergroup and membership data (default)
    • user:write - reading and writing your own user, ordergroup and membership data
    • users:user - reading foodcoop-public member, ordergroup data
    • users:read - reading all data of all users and ordergroups
    • users:write - writing all data of all users and ordergroups
    • workgroups:user - reading foodcoop-public workgroup data
    • workgroups:read - reading all workgroup data
    • workgroups:write - reading and writing all workgroup data (&)
  • suppliers
    • suppliers:read - reading all suppliers (#)
    • suppliers:write - reading and writing all suppliers
  • articles
    • articles:read - reading all articles
    • articles:write - reading and writing all articles
  • orders
    • orders:read - reading all orders and order articles (default)
    • orders:write - reading and writing all orders and order articles (*)
  • group_orders
    • group_orders:user - reading and writing the user's ordergroup's group orders and group order articles (default)
    • group_orders:read - reading any ordergroup's group orders and group order articles
    • group_orders:write - the above, plus writing them
  • finance
    • finance:user - reading the user's financial transactions (perhaps also account balance) (default)
    • finance:read - reading everyone's financial transactions
    • finance:write - reading and writing all financial transactions
  • invoices
  • tasks
  • documents
  • messages
  • ...

Scopes marked with (default) are included in the default scopes and require no extra permissions (through workgroups). Perhaps it is also useful to include a 'default' scope that can be added to include the default scopes when adding another one.

Note that some data where scopes are needed, may still be accessible to the user (but then only a portion of it). For example, it makes sense to include article data in the order_articles endpoint (because you'd usually want to show the article name, for example).

(&) Do we need some restriction of modifying workgroups with more permissions than the logged-in user?
(#) Other scopes one could think of are reading suppliers used in orders in the past half year. Not for now.
(*) Later perhaps, we may want to introduce more fine-grained scopes (e.g. just closing orders).

@carchrae
Copy link
Contributor

sorry for the delay replying. other work got busy, but i want my co-op up to the latest version so i can help more effectively on the api.

ACL is a very hard to get right. it'll often end up nearly right, and then people start to hack around it to get reasonable use cases working. i saw this in spree - where they started using cancan but then quickly the controllers became littered with logic that was checking whether something was allowed or not. the net result was a real mess.

i think your scopes above seem reasonable. i presume the intention is to take these scopes and then tell doorkeeper to enforce them via https:/doorkeeper-gem/doorkeeper/wiki/Using-Scopes#using-in-your-api

i would guess that you won't be finished working on the scopes until you have finished working on the api. so, the best thing you can get perfect now is trying to define a consistent/extensible approach.

i also think you are right that simplicity to understand it is really one of the main goals. and having an agreed way to customise security per co-op would be useful (ie, extensibility/customisation in a way that doesn't prevent applying upgrades)

@wvengen
Copy link
Member Author

wvengen commented Nov 29, 2018

Thanks for your input, @carchrae. I hope we can integrate your contributions into mainline Foodsoft sometime.

What would you choose: role-based vs. ~resource-based, or something more in-between?
And yes, the intention is to use doorkeeper_authorize! in API controllers (plus probably a ransack auth_object to allow/deny searching on certain attributes/relations - or are strong parameters better here?

having an agreed way to customise security per co-op would be useful

Do you have a concrete example, perhaps, to explain what is meant here?
Currently foodcoops need to register all apps, and would also need to specify allowed scopes (only apps provided as a plugin would auto-create these Doorkeeper app credentials, but they are already trusted by either the Foodsoft developers including them in the default setup, or by the foodcoop having added them).

wvengen added a commit to wvengen/foodsoft that referenced this issue Feb 7, 2019
wvengen added a commit to wvengen/foodsoft that referenced this issue Feb 20, 2019
wvengen added a commit that referenced this issue Apr 3, 2019
wvengen added a commit that referenced this issue Apr 3, 2019
@orangeman
Copy link
Contributor

would it make sense to specify some since=last_modified parameter?
just as an optimization to make periodic polling cheaper..?

Are financial_transactions ordered by (autoincrement) id? and never edited?

@carchrae
Copy link
Contributor

@wvengen - i am curious, do you know if anyone is currently using the api for a vue/react app? i have heavily customised the ordering page and balancing page, but using rails templates and scraps of javascript. i would much rather do that with front-end js code. i am reasonably proficient with vue.

i think it probably takes building a full feature ui to really tease out all the details needed from the api.

@wvengen
Copy link
Member Author

wvengen commented Jan 28, 2021

@carchrae I don't really have an overview, but know that the financial transactions are being used. The rest hasn't really been merged yet (but I plan to do so in the next weeks), so I guess that is hasn't been used very widely yet. I vaguely remember hearing from one or two people that they were using it. But the (to be merged) API should be ready to do member ordering - foodsoft-shop uses it.

@wvengen
Copy link
Member Author

wvengen commented Jan 28, 2021

@orangeman what is your use-case?

@carchrae
Copy link
Contributor

thanks @wvengen. i will try out the api when i try to get our coop's site back up to date (and will submit some other PRs). not clear on when i will find time to do that, but i'd like to improve a few pages

@orangeman
Copy link
Contributor

orangeman commented Jan 28, 2021 via email

@wvengen
Copy link
Member Author

wvengen commented Jan 29, 2021

@orangeman that makes sense; for real-time updates I hope someday something with websockets could be used (so that e.g. when the order is closing soon, members can see real-time updates when ordering), for offline syncing some standard parameter (like since) could be added to endpoints to only return records that were modified since that timestamp. I'd welcome PRs for that.

@orangeman
Copy link
Contributor

orangeman commented Jan 30, 2021 via email

@SomeMichael
Copy link

Hi all!

I would like to give the REST API a try. Is it ready to be used? I could not find a documentation site despite of this thread...

I just tried /api/v1/navigation on our foodsoft account and it returned an "invalid_token" error, which looked promising ;-)

Thanks, Michael

PS: I have to mention, so far, I have no experience in REST API usage. I understand how that it works on regular HTTP requests. But, e.g,, from the content in this thread I'm not sure how authentication works.
PPS: So far, I use perl scripting and WWW::Mechanize to sync data between foodcoops.net and our shop managing system (SQL Server). That's not a very convenient way of doing so...

@wvengen
Copy link
Member Author

wvengen commented Apr 2, 2023

Yes, it is ready to be used. Mostly the user-facing part has been implemented, which is used by foodsoft-shop. Yes, this is usable by a recent version of Foodsoft. You have to setup a token from within Foodsoft. This is document in docs/API.md, which also links to the API documentation.
I know that financial transactions are also used by an application using the REST API, I think that is the only part that has more 'admin' related things (e.g. not ordering and such as a user, but working with financial transactions from all users).

@SomeMichael
Copy link

SomeMichael commented Apr 2, 2023 via email

@SomeMichael
Copy link

Ok, I got access via Foodsoft API set up from within my Perl script. Now, checking the docs, I see I am missing "some" admin tasks.

As I already mentioned above I am currently running a perl script interacting with a foodsoft instance on app.foodcoops.net to sync data with our shop management DB. This is currently done by loading, analyzing and form filling the foodssoft web pages themselves. Tasks are

  • Getting user data of all users
  • Updating, creating and deleting users where necessary
  • Getting all order groups
  • Updating, creating and deleting order groups where necessary
  • Filling up accounts of order groups

I would like to switch to an approach, where my script manipulates the data via Foodsofts API. Currently, the API doesn't seam to support any kind of the transactions I would need. That would be, I assume, something like:

  • GET users
  • POST user/{id}
  • DELETE user/{id}
  • GET ordergroups
  • POST ordergroup/{id}
  • DELETE ordergroup/{id}
  • POST ordergroup/{id}/financial_transaction

Are there any plans to extend the API to support such admin tasks?

@wvengen
Copy link
Member Author

wvengen commented Apr 3, 2023

Thank you for listing your findings and needs clearly.
At this moment, none of the people involved have a need to expand the API (or the needs are not pressing enough to start implementing them), hence this is not on the roadmap. Contributions would be welcome in this area.

@SomeMichael
Copy link

Ok, I will take a look at the code and put my limited programming knowledge to the test and see what I can achieve.

@wvengen
Copy link
Member Author

wvengen commented Apr 3, 2023

:) 👍
You may want to start from #969.

@SomeMichael
Copy link

SomeMichael commented May 10, 2023

I added admin/users as a first step in fork branch. This is based upon #969 as suggested ;-). Commit b4348ff

Anyone feel free to have a look at it if you find the time. I'm looking forward to any comments/suggestions/whatever since I'm all new to that ruby/rails/rspec/git stuff. Thanks!

I will start with order groups implementation meanwhile.

@yksflip
Copy link
Member

yksflip commented May 12, 2023

thats great @SomeMichael ! If you like to we could make a pair session, I could show you some rswag insights and we could talk about your api changes :)
feel free to contact me via matrix @yksflip:matrix.kaputt.cloud or mail [email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants