Introduce ResourceWithVault layer #6621
Conversation
types/src/front/auth.ts
Outdated
| // Authenticator is a concept that lives in front, | ||
| // but it's used when doing api calls to other services. | ||
| // This interface is a cheap way to represent the concept of an authenticator within types. | ||
| export interface BaseAuthenticator { |
There was a problem hiding this comment.
Will be used in the follow up PR.
| // Although we have the capability to support multiple workspaces, | ||
| // currently, we only support one workspace, which is the one the user is in. | ||
| // This allows us to use the current authenticator to fetch resources. | ||
| const allWorkspaceIds = _.uniqBy(dataSourceConfigurations, "workspaceSId"); |
There was a problem hiding this comment.
the property is workspaceId
maybe new Set(dataSourceConfigurations.map(ds => ds.workspaceId) would be more efficient than lodash ?
| }); | ||
|
|
||
| return blobs.map((b) => { | ||
| const vault = new VaultResource(VaultResource.model, b.vault.get()); |
There was a problem hiding this comment.
why not including the vault in other includedResults ? you dont need specific handling for the vault here, and that would simplify the constructors. Actually you could even have a more generic baseFetch outside of this file, which do not add vault, but just takes the list of includes
There was a problem hiding this comment.
The reason vault is not in includedResults is because here vault is a resource whereas includedResults contains a raw model from Sequelize.
| blob: Omit<CreationAttributes<DataSource>, "vaultId">, | ||
| vault: VaultResource |
There was a problem hiding this comment.
I would have added the vault in blob and destructured it inside the body, to have one single "blob" parameter containing all the attributes (so, in the end, with vault instead of vaultId).. ?
| super(model, blob); | ||
| } | ||
|
|
||
| protected static async baseFetch< |
There was a problem hiding this comment.
This function is in charge of ensuring authorization given its role in I injecting the workspace id.
So let's be more explicit about it in its name. Maybe authorizedFetch rather than baseFetch?
Also since the resource that have it are in a vault we can enforce that auth has the group that owns the vault and error otherwise no?
There was a problem hiding this comment.
I initially wanted to include a permission check, but it does not work. Today, everyone needs to be able to fetch data source to list them in various places (assistant builder, Connections, ...). Additionally, system groups are never loaded in auth, so the permissions (read and write) primarily apply to writing to and reading from the data source, but does not prevent listing these items. In the future once we will have refactored everything maybe we will have a way to make this work.
There was a problem hiding this comment.
Yes that might be premature. Let's still rename the method to emphasize the fact that it plays a security role (even now with workspace filtering).
There was a problem hiding this comment.
Yes that might be premature. Let's still rename the method to emphasize the fact that it plays a security role (even now with workspace filtering).
Isn't that similar to most resources, which are always filtered based on the workspace?
There was a problem hiding this comment.
Yes but here it's particularly important :-)
There was a problem hiding this comment.
Renamed to baseFetchWithAuthorization.
flvndvd
added
the
migration-ack
| resourceName: ResourceNameType, | ||
| sId: string | ||
| ): boolean { | ||
| return sId.startsWith(RESOURCES_PREFIX[resourceName]); |
There was a problem hiding this comment.
We should test that it starts with the prefix and the underscore to remove any chance of a collision on the prefix since _ is not a valid legacy sId character 👍
* Implement ACL on data source and data source views. * Introduce ResourceWithVault * ✨ * Add fetchDataSource on DataSourceView * 🙈 * Address comments from review * 🙈 * ✂️ * s/baseFetch/baseFetchWithAuthorization
Description
This PR introduces a new
ResourceWithVaultlayer. This addition brings in abaseFetchmethod, to retrieve a resource along with its associated vault. This is essential for ensuring proper permission checks before any read or write operations. While it won’t be widely used at the moment, having synchronous access to the ACL is required.TypeScript has been pretty challenging with the Sequelize types, so I've left a few TODOs to ensure that all
fetch*andlist*methods accept an auth parameter.This PR also prevent creating assistant configurations using data sources from another workspace than the current one (see thread here).
Risk
Worst case it breaks listing/editing data sources. Safe to rollback.
Deploy Plan
No migration needs to be run.