One more general comment: we should find an easy way to document what are the protected resources and actions to be permitted on each endpoint of a secured server.
@tokoko do you have any suggestions to apply this in a reliable and effective way? (e.g. are docstrings enough to draw the user's attention?)

@dmartinol No, I think we will need a separate security page in the docs, explaining both permission model and mapping of secure endpoints to necessary permissions. I guess you were looking for something more reliable 😄 I doubt it will be too dynamic, just manual updates should suffice.

@dmartinol No, I think we will need a separate security page in the docs, explaining both permission model and mapping of secure endpoints to necessary permissions. I guess you were looking for something more reliable 😄 I doubt it will be too dynamic, just manual updates should suffice.

@redhatHameed you can add a section in the user guide to expose the permission requirements for every endpoint, but I'm not sure in which section: I see some servers are not properly documented (offline store and registry, IIUC), so I can't suggest the best place for that. Anyway we can add a section somewhere instead of increase the undocumented features.

@dmartinol Is the action list in assert_permissions applied with an and or a or? If it's an and, I don't get the point of having a separate QUERY action.

@dmartinol Is the action list in assert_permissions applied with an and or a or? If it's an and, I don't get the point of having a separate QUERY action.

🤔 Trying to recap aloud:

• Each Permission has a list of actions specifying which operations it applies to (any)
• QUERY and WRITE have been designed as shortcuts for[QUERY_ONLINE, QUERY_OFFLINE]and [WRITE_ONLINE, WRITE_OFFLINE] but are however in the enum
• When we validate a request, we should verify that the user is permitted all the requested actions that we assume will be performed on the resources, so it's and IMO

Anyway, I think the model is misleading and we should move away QUERY and WRITE from the enum and instead add aliases for them:

QUERY=[QUERY_ONLINE, QUERY_OFFLINE] 
WRITE=[WRITE_ONLINE, WRITE_OFFLINE]

This avoids any misunderstanding and at validation time we must specify either the actual store type(s) or use the alias.
The same is for the ALL entry that can be replaces by another alias with all the values.
Note: Maybe it was already clear, but aliases can simplify the admin's life at configuration time.

yeah, I think alias makes more sense. I forgot about the permissions part, using QUERY and WRITE there seems valuable. But I think they should never be used on validation side, right? (Other than maybe push endpoint which can write to both stores) If we are validating a method in feature server, we should use *_ONLINE actions, in case of a flight server, we use *_OFFLINE.

I think they should never be used on validation side, right?

Agree, validation should list all the executed actions, aliases are unlikely to be used (I see there's a CRUD in the PR which of course is the exception to the rule for apply methods)

lgtm but IMO we shoud try to minimize the impact of validating code by preparing fuctions leading to more concise code (e.g. accepting lists and returning the same type as the input object)
We can probably merge now

lgtm but IMO we shoud try to minimize the impact of validating code by preparing fuctions leading to more concise code (e.g. accepting lists and returning the same type as the input object) We can probably merge now

@dmartinol @tokoko if you guys are fine we can merge this. There are remaining Task which we need to address in next PRs

1- Handle The permission on These three functions

2- Add a section in the user guide to expose the permission requirements for every endpoint

3- Refactor the permission validation code accepting lists and returning the same type as the input object.