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

Protect runtime RPC with authorization #3034

Closed
wants to merge 28 commits into from

Conversation

apedroferreira
Copy link
Member

@apedroferreira apedroferreira commented Dec 27, 2023

On query requests to the runtime RPC, require that a user has the required roles according to the page the query is called from.
Includes a few refactors too.

@apedroferreira apedroferreira added the new feature New feature or request label Dec 27, 2023
@apedroferreira apedroferreira self-assigned this Dec 27, 2023
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jan 2, 2024
@apedroferreira apedroferreira changed the title [WIP] Protect runtime RPC with authentication/authorization Protect runtime RPC with authentication/authorization Jan 2, 2024
@apedroferreira apedroferreira marked this pull request as ready for review January 2, 2024 16:59
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jan 7, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 12, 2024
@apedroferreira apedroferreira changed the title Protect runtime RPC with authentication/authorization Protect runtime RPC with authorization Jan 18, 2024
@apedroferreira apedroferreira marked this pull request as draft January 18, 2024 23:50
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 29, 2024
@apedroferreira
Copy link
Member Author

@Janpot I'm getting an error here due to the types I created and I'm not even sure if this is a good way to protect queries from users without the correct roles, or if there is a better way we should do it... Let me know what you think, there's a lot of things in these parts of the code that you probably understand better than me.

@Janpot
Copy link
Member

Janpot commented Jan 30, 2024

  • I don't think we need to prioritize this until we've taken the time to change the dataprovider to not use "resource-file+function" to call its method, but rather "page-file+datagrid". Because with the current method it's possible for every user to call arbitrary functions anyway, even if you change execQuery.
  • I'm not sure we should change the rpc implementation for this. I think we could put authentication information in the server context so that users can get this info with getContext(). We should be able to piggy-back on this system to implement strict access enforcement on the server functions. But again, as long as datasources provide an interface that allows calling arbitrary functions, this last thing is pretty much wasted effort.

So I'd say:

  • Forget about RBAC in the rpc layer for now, or refactor the dataprovider first instead.
  • Expand getContext with auth information. this could be a nice user facing feature.

@apedroferreira
Copy link
Member Author

apedroferreira commented Jan 31, 2024

  • I don't think we need to prioritize this until we've taken the time to change the dataprovider to not use "resource-file+function" to call its method, but rather "page-file+datagrid". Because with the current method it's possible for every user to call arbitrary functions anyway, even if you change execQuery.
  • I'm not sure we should change the rpc implementation for this. I think we could put authentication information in the server context so that users can get this info with getContext(). We should be able to piggy-back on this system to implement strict access enforcement on the server functions. But again, as long as datasources provide an interface that allows calling arbitrary functions, this last thing is pretty much wasted effort.

So I'd say:

  • Forget about RBAC in the rpc layer for now, or refactor the dataprovider first instead.
  • Expand getContext with auth information. this could be a nice user facing feature.

Sounds good, thanks! I'll probably close this PR for now then, and can add user to the context in a further PR after the others are merged and authentication is released.

@apedroferreira
Copy link
Member Author

Closing, I'll create issues later for the features that were suggested instead of this implementation.

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

Successfully merging this pull request may close these issues.

2 participants