Conversation
|
|
||
| let permissions: ListPermissionsResponse | undefined = undefined; | ||
| try { | ||
| permissions = await this.client.getPermissions(projectId); |
There was a problem hiding this comment.
I don't think you want this. Querying the edges directly means re-implementing the logic in spicedb, since edges != permissions. Instead, I think you want to use permissions/my to ask "What permissions do I have": https://github.com/anaconda/bigbend-platform/blob/main/apps/projects/projects/api/endpoints.py#L442
There was a problem hiding this comment.
It appears that the only usages of listPermissions are for getting a list of collaborators, so querying permissions/my wouldn't apply here. Is there a better way of determining that?
There was a problem hiding this comment.
I think this is a case where semantics need to be pretty clear. Looking at how checkPermissions is used in the codebase, you have code like this:
const permissionResult = await permissionsProvider.checkPermission(
passport.user.id,
notebookId
);
if (!permissionResult.hasAccess) {
return c.json(
{
error: "Not Found",
message: "Notebook not found or access denied",
},
404
);
}
This is querying if the user has access to a notebook (or possibly which level of access read, write, find etc).
For all of these "is access granted" reading the edges directly is an anti-pattern. The presence of edges does not guarantee access. Only the graph and schema can tell you that. Spicedb is the only source of truth for this type of determination. So, for this usage of checkPermission, this should be permissions/my. The other thing is that querying edges like this is slow and spicedb has challenges scaling this out. Spicedb is built around checking if permission is granted quickly and at scale, not determining which edges are in the graph.
Now, let's say you were trying to build a share dialog, similar to google docs when you want to share a document with someone else. Then, at that point you are trying to see who you gave access to the document, and that is the point where you want to start reading edges
There was a problem hiding this comment.
I see, I was looking at uses for listPermissions and your comment was regarding checkPermissions. I verified that we can use permissions/my for the checkPermissions flow and made that change.
It does still look like we are stuck reading the edges for the listPermissions flow, because the projects service doesn't expose an API for "what permissions does this user have on this project", so we must read the edges to determine this.
There was a problem hiding this comment.
This might be a case for a quick huddle? From https://github.com/anaconda/bigbend-platform/blob/main/apps/projects/projects/api/endpoints.py#L442 permissions/my has this code
response = await get_user_permission_for_project(
project_id=project_id,
user_id=UUID(passport.user_id),
passport=passport,
consistency_token=consistency_token,
)
return response
so it's going to return one of these:
class MyPermissionResponseItem(BaseModel):
own: bool
modify: bool
read: bool
share: bool
find: bool
delete: bool
Is that not what you want?
|
Do we need to make changes to https://github.com/runtimed/intheloop/blob/main/.github/workflows/deploy-production.yml as well? Maybe just changing |
markmiro
left a comment
There was a problem hiding this comment.
I ran the code locally and it works great!
The env vars are getting complex now. It would be good to simplify that in a future PR.
I love all the comments around edge cases considered and caveats.
|
|
||
| let permissions: ListPermissionsResponse | undefined = undefined; | ||
| try { | ||
| permissions = await this.client.getPermissions(projectId); |
There was a problem hiding this comment.
I think this is a case where semantics need to be pretty clear. Looking at how checkPermissions is used in the codebase, you have code like this:
const permissionResult = await permissionsProvider.checkPermission(
passport.user.id,
notebookId
);
if (!permissionResult.hasAccess) {
return c.json(
{
error: "Not Found",
message: "Notebook not found or access denied",
},
404
);
}
This is querying if the user has access to a notebook (or possibly which level of access read, write, find etc).
For all of these "is access granted" reading the edges directly is an anti-pattern. The presence of edges does not guarantee access. Only the graph and schema can tell you that. Spicedb is the only source of truth for this type of determination. So, for this usage of checkPermission, this should be permissions/my. The other thing is that querying edges like this is slow and spicedb has challenges scaling this out. Spicedb is built around checking if permission is granted quickly and at scale, not determining which edges are in the graph.
Now, let's say you were trying to build a share dialog, similar to google docs when you want to share a document with someone else. Then, at that point you are trying to see who you gave access to the document, and that is the point where you want to start reading edges
@rgbkrk This should set projects_artifacts to true by default, but I need to create environment vars on the repo for them to be picked up by the frontend. I don't have permissions to create those yet. |
Use anaconda projects service for permissions and file management
TODOs:
After pulling these changes ensure you re-create your .dev.vars file
Design doc: https://docs.google.com/document/d/1ST6WcUyKYV7E2I4zIyyk9asHlv-vBNr3zSGh1neyuNw/edit?tab=t.0#heading=h.4eukoeeacbj2
Some quick screenshots:
File upload:

Project permissions list after adding a collaborator:
