Feat/endpoint authorization #849

luizgribeiro · 2023-04-05T12:00:52Z

Hey @mcollina, @ivan-tymoshenko.

Have a look on the initial draft for endpoint based access control.
Let me know if I'm missing out something.

luizgribeiro · 2023-04-06T00:43:50Z

I think the structure is not quite there.
After having a deeper look on fastify I think a valid approach would be:

Use the "onReady" hook that is alredy creating hooks for entities to create a "onRequest" hook
This onRequest hook would check on the request content and enpoint rules to allow or deny access to route handler
The "onReady" hook function would have to be refactor because if a rule that has no entity on it is processed it throws

mcollina · 2023-04-06T06:00:39Z

I would follow a slightly different structure:

create an req.authorize(permissions) method as described in the issue first. This allows you to develop things in baby-steps.
Use the onRoute hook to add a onRequest hook that calls req.authorize()

Wdyt?

luizgribeiro · 2023-04-06T17:34:15Z

You mean to use a basic structure like this in platformatic.db.json?

{
  "rules": [
    {
      "endpoint": "/someCustomEndpoint",
      "authorizer": "<path to exported authorizer>"
    }
  ]
}

I was thinking about using a structure like the one used for entities for endpoints as well, but it seems reasonable to have different forms for both cases.

I'd like to hear your thoughts about how to be specific for a route (considering that a route is made of both HTTP Method and Path). This way only the Path is being considered.

More general questions are:

I think of authorizers returning either a boolean or throwing. Do you see any case in witch this is a problem?
Authorizer should actually be authorizers. This way an array can be passed and if any of the authorizers provided returns true the request handler is used.

mcollina · 2023-04-07T07:46:21Z

I don't really get your previous comment.

The first step I recommended is to add a single req.authorize(permissions) function that would check a bunch of user-specified permissions before progressing. It should throw if the current user does not have the rights to perform the operation described in permissions. I would represent permission in a similar way we have represented all other rules.

luizgribeiro · 2023-04-10T14:35:32Z

Alright! I've just pushed with what I imagine that is the first step using the approach that you seem to be talking about.
Turns out the issue had a lot of different and incompatible ideas and I focused on what I think that would be easier to the end user/developer.

I followed the approach suggested by @ivan-tymoshenko to provide a method to be called inside the handler, but in terms of developer experience I think it would be better to do it before that and make it invisible for developers. It might be a bit more difficult to implement because of the way that we deal with request hooks and plugins at fastify core (I don't know much, it's my first time using it).

I still have a doubt about using on onRoute hook. In the way that I implemented it it looks like whenever a new route is registered by fastify a new onRequest hook is also created. Wouldn't it be enough just to use one single onRequest hook to check authorization?

mcollina · 2023-04-18T19:03:08Z

packages/db-authorization/index.js

@@ -61,6 +61,37 @@ async function auth (app, opts) {
    }
  }

+  app.addHook('onRoute', function () {
+    app.addHook('onRequest', function (request, reply, done) {
+      request.authorize = async () => {


I think this could be implemented as

app.decorateRequest('authorize', authorizeFn)

Instead of using the hooks.

Signed-off-by: Luiz Ribeiro <ltrindaderibeiro@gmail.com>

mcollina · 2023-04-25T21:39:39Z

wdyt @ivan-tymoshenko ?

packages/db-authorization/fixtures/plugin.js

mcollina · 2023-04-25T21:36:53Z

packages/db-authorization/index.js

+    await request.extractUser()
+
+    const authorizationMatch = endpointRules.find(rule => {
+      if (rule.role === request.user['X-PLATFORMATIC-ROLE'] && rule[restMapper[request.method]]) {


Suggested change

if (rule.role === request.user['X-PLATFORMATIC-ROLE'] && rule[restMapper[request.method]]) {

if (rule.role === request.user[rokeKey] && rule[restMapper[request.method]]) {

mcollina · 2023-04-25T21:39:24Z

packages/db-authorization/index.js

+        return !!request.raw.url.match(endPointRegEx)
+      }
+      return false
+    })


The whole point of adding a decorator is to support not doing this in here. We want to provide a generic utility folks could use manually.

Then we use the data in rule.endpoint to automatically add a preHandler that call this endpoint with the given rule.

I don't get the part about using it manually. Do you mean something like passing the rules to req.authorize() like @ivan-tymoshenko said?

It looks a bit confusing to me to be honest. I like the idea of using preHandler to prevent calling req.authorize() inside the plugin, but I'm not sure if it is possible if it's not using all the authorization rules from a configuration files

I find this idea confusing too... @mcollina ..😉🙂

ivan-tymoshenko

I see the idea of defining all rules in one file, but I don't like the idea of mapping custom routes to the rules/entities manually. It would be impossible to maintain it with a real application. I would prefer an explicit request.authorize(rules) call.

ivan-tymoshenko · 2023-04-25T21:47:32Z

packages/db-authorization/index.js

+    const endpointRules = opts.rules.filter(rule => {
+      if (rule?.endpoint) {
+        const endPointRegEx = new RegExp(rule.endpoint)
+        return !!request.raw.url.match(endPointRegEx)


This would not work for all possible fastify routes. It should work for parametric and regexp routes.

The idea behind this was to use a root/base endpoint to cover the parametric routes. I agree that it is not ideal though. A rule using the endpoint /pages would also cover /pages/pageId.
I did not think about how to cover regexp rules.

ivan-tymoshenko · 2023-04-25T21:48:43Z

packages/db-authorization/index.js

+      return false
+    })
+
+    const restMapper = {


Why only these methods?

Still wip, there is still a lot to be done on this

ivan-tymoshenko · 2023-04-25T22:15:28Z

packages/db-authorization/test/endpoint.test.js

+    roleKey: 'X-PLATFORMATIC-ROLE',
+    anonymousRole: 'anonymous',
+    rules: [{
+      role: 'user',


I don't understand what this rule means. find, save, and delete should be related to some entity.

I am not a big fan of this either. It was implemented like that to look like the rules provided for entities

Co-authored-by: Matteo Collina <matteo.collina@gmail.com>

luizgribeiro · 2023-04-26T01:14:06Z

Hey folks!

I was still working on this but it was nice to have a review from both of you.
At this point I think that there might be a few different ideas about how this should work.
I believe that it can be worked out before advancing on this.

If the spec becomes to big it might be worth to split it in a few features to have it fully covered.

My main concerns at this point are:

Where should authorization rules be kept? If it's passed to the authorize call I'd expect an array with all the roles that are allowed to access that specific endpoint (seems clean to me).
What should be the structure used for securing an endpoint? In the case above an array would be enough, but with rules coming from a config file this wold be important. As @ivan-tymoshenko said, using find, save, delete for a plugin sounds weird

mcollina · 2024-05-22T10:37:53Z

closing for no updates

luizgribeiro added 2 commits April 5, 2023 08:55

initial sketch for endpoint based authorization

b028291

add comment

d21d30d

luizgribeiro added 3 commits April 8, 2023 11:41

initial test for endpoint based authoriztion

419c623

add initial structure for authorize function

117903e

route based auth - find rest baby step

a94860c

mcollina reviewed Apr 18, 2023

View reviewed changes

luizgribeiro added 3 commits April 23, 2023 12:31

add rest endpoint default auth mechanisms

8b7a4f0

add other methods (REST) and tests

9fbbfd6

Signed-off-by: Luiz Ribeiro <ltrindaderibeiro@gmail.com>

lint

d757dda

Signed-off-by: Luiz Ribeiro <ltrindaderibeiro@gmail.com>

mcollina requested a review from ivan-tymoshenko April 25, 2023 21:39

mcollina reviewed Apr 25, 2023

View reviewed changes

ivan-tymoshenko requested changes Apr 25, 2023

View reviewed changes

ivan-tymoshenko reviewed Apr 25, 2023

View reviewed changes

add strict

201672f

Co-authored-by: Matteo Collina <matteo.collina@gmail.com>

ivan-tymoshenko force-pushed the main branch from 6d7cc3d to 764d204 Compare August 21, 2023 16:23

mcollina closed this May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/endpoint authorization #849

Feat/endpoint authorization #849

luizgribeiro commented Apr 5, 2023

luizgribeiro commented Apr 6, 2023

mcollina commented Apr 6, 2023

luizgribeiro commented Apr 6, 2023

mcollina commented Apr 7, 2023

luizgribeiro commented Apr 10, 2023

mcollina Apr 18, 2023

mcollina commented Apr 25, 2023

mcollina Apr 25, 2023

mcollina Apr 25, 2023

luizgribeiro Apr 26, 2023

ghost Apr 30, 2023

ivan-tymoshenko left a comment

ivan-tymoshenko Apr 25, 2023

luizgribeiro Apr 26, 2023

ivan-tymoshenko Apr 25, 2023

luizgribeiro Apr 26, 2023

ivan-tymoshenko Apr 25, 2023

luizgribeiro Apr 26, 2023

luizgribeiro commented Apr 26, 2023

mcollina commented May 22, 2024

	if (rule.role === request.user['X-PLATFORMATIC-ROLE'] && rule[restMapper[request.method]]) {
	if (rule.role === request.user[rokeKey] && rule[restMapper[request.method]]) {

Feat/endpoint authorization #849

Feat/endpoint authorization #849

Conversation

luizgribeiro commented Apr 5, 2023

luizgribeiro commented Apr 6, 2023

mcollina commented Apr 6, 2023

luizgribeiro commented Apr 6, 2023

mcollina commented Apr 7, 2023

luizgribeiro commented Apr 10, 2023

Choose a reason for hiding this comment

mcollina commented Apr 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivan-tymoshenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luizgribeiro commented Apr 26, 2023

mcollina commented May 22, 2024