BC-5424 - persistent storage for tldraw #4685
Conversation
davwas
added
the
WIP
# Conflicts: # apps/server/src/modules/tldraw/config.ts
# Conflicts: # src/services/config/publicAppConfigService.js
davwas
removed
the
WIP
| @@ -72,7 +75,7 @@ export class BoardDoAuthorizableService implements AuthorizationLoaderService { | |||
| userId: user.id, | |||
| firstName: user.firstName, | |||
| lastName: user.lastName, | |||
| roles: [BoardRoles.READER], | |||
| roles: isDrawingElement ? [BoardRoles.EDITOR] : [BoardRoles.READER], | |||
There was a problem hiding this comment.
What is the default value of this boolean?
For me is this change unclear, can we speak about it?
With this line you create a exception for one element that invert the authorisation (logic/role), it look like the first step into the wrong direction.
There was a problem hiding this comment.
The default should be false and only set the student to editors when accessing a drawing element.
Currently all students are readers. In the submission case we actually overrule the permission in the UC layer.
As this is a temporary solution until we have the proper authZ set in place.
I would be better to set exceptions outside this function like how the submission is handling it to keep the core code clean for now.
There was a problem hiding this comment.
The default is false.
We want to store uploaded assets from tldraw in s3 storage - by default every authorized user is supposed to be able to upload to tldraw. There is already a logic in place to upload files to column board nodes. If requested board node is an instance of DrawingElement (which is Tldraw) we want to give student role access to upload.
The alternative I can think of is to create new authorizable service only for DrawingElements which would basically be a copy paste of board-do-authorizable.service with just this one line changed. We are open to any other suggestions.
# Conflicts: # config/test.json
| @@ -54,7 +55,9 @@ export class ElementUc extends BaseUc { | |||
| if (isSubmissionItem(parent)) { | |||
| await this.checkSubmissionItemWritePermission(userId, parent); | |||
| } else { | |||
| await this.checkPermission(userId, element, Action.write); | |||
| // TODO: fix this temporary hack to prevent students from deleting the DrawingElement | |||
There was a problem hiding this comment.
Please add also that the roles are hacked for the drawing element.
| @@ -54,7 +55,9 @@ export class ElementUc extends BaseUc { | |||
| if (isSubmissionItem(parent)) { | |||
| await this.checkSubmissionItemWritePermission(userId, parent); | |||
| } else { | |||
| await this.checkPermission(userId, element, Action.write); | |||
| // TODO: fix this temporary hack to prevent students from deleting the DrawingElement | |||
| const requiredRole = isDrawingElement(element) ? UserRoleEnum.TEACHER : undefined; | |||
There was a problem hiding this comment.
if (isSubmissionItem(parent))
if else (isDrawingElement(element))
const requiredRole
await this.checkPermission
else
await this.checkPermission
|
Please solve the merge conflicts. |
# Conflicts: # apps/server/src/modules/tldraw/config.ts # apps/server/src/modules/tldraw/controller/tldraw.ws.ts # apps/server/src/modules/tldraw/repo/tldraw-board.repo.ts # apps/server/src/modules/tldraw/tldraw.module.ts # config/default.schema.json # config/test.json # package-lock.json # package.json # src/services/config/publicAppConfigService.js
There was a problem hiding this comment.
Expected the open follow ups. it is approved.
Edit: Please add a test for isDrawing https://sonarcloud.io/component_measures?id=hpi-schul-cloud_schulcloud-server&pullRequest=4685&metric=new_uncovered_lines&view=list
CeEv
dismissed
EzzatOmar’s stale review
The open work, should be done later. Let speak @EzzatOmar how we proceed.
|
Description
Links to Tickets or other pull requests
https://ticketsystem.dbildungscloud.de/browse/BC-5424
hpi-schul-cloud/nuxt-client#3002
hpi-schul-cloud/tldraw-client#47
Changes
Datasecurity
Deployment
New Repos, NPM pakages or vendor scripts
Approval for review
generate-client:serverwas executed in vue frontend and changes were tested and put in a PR with the same branch name.