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

Add foreign key constraint handler flow in app #9507

Open
3 tasks done
Toilal opened this issue Nov 5, 2021 · 22 comments
Open
3 tasks done

Add foreign key constraint handler flow in app #9507

Toilal opened this issue Nov 5, 2021 · 22 comments

Comments

@Toilal
Copy link
Contributor

Toilal commented Nov 5, 2021

Preflight Checklist

Describe the Bug

An error occurs when trying to delete user that have uploaded files to media library.

To Reproduce

  • Create a new user
  • Login with this user
  • Add a file in the media library
  • Login with admin
  • Try to delete the user

Errors Shown

{
  "errors": [
    {
      "message": "delete from \"directus_users\" where \"id\" in ($1) - update or delete on table \"directus_users\" violates foreign key constraint \"directus_files_uploaded_by_foreign\" on table \"directus_files\"",
      "extensions": {
        "code": "INTERNAL_SERVER_ERROR"
      }
    }
  ]
}

What version of Directus are you using?

v9.0.0

What version of Node.js are you using?

v16.13.0

What database are you using?

Postgres 13

What browser are you using?

Chrome

What operating system are you using?

Linux

How are you deploying Directus?

locally (dev)

@azrikahar
Copy link
Contributor

This should be a duplicate of #9365, and was fixed in #9305, however it was then removed in #9425 due to issue with MSSQL as seen in the discussion under that PR.

This should be the current remaining effects of that PR: #9365 (comment), which still doesn't address your issue here.

Since it wasn't technically solved, I'll keep this open for now. @rijkvanzanten thoughts on this particular scenario? 🤔

@rijkvanzanten
Copy link
Member

Good question. This is technically working as expected. There's a foreign key constraint from files to users (in uploaded_by and modified_by) that makes sure you can't have invalid users stored in those columns. We tried setting those to SET NULL on deletion of the user (which would prevent the OP from showing up), however you're not allowed to have > 1 foreign key constraint triggers to the same related collection in certain databases 🤔

The ideal solution here would be to:

  • Abstract that error properly so it doesn't throw a 500
  • Then in the app, show a dialog when the foreign-key-constraint error throws asking what you want to do with the related items, and handle the changes on the app side before deleting the item

cc @jaycammarano

@rijkvanzanten rijkvanzanten changed the title Can't delete user after uploading a file in media library Add foreign key constraint handler flow in app Nov 5, 2021
@andriusign
Copy link
Contributor

andriusign commented Nov 25, 2021

Does not make any sense at all. You should be able to delete anything you want and leave anything you like, even if it is user's files. This is a valid business scenario to delete user and leave his files.

@benhaynes
Copy link
Sponsor Member

Then in the app, show a dialog when the foreign-key-constraint error throws asking what you want to do with the related items, and handle the changes on the app side before deleting the item

@andriusign — Did you read the response by Rijk? That is exactly what he is recommending by saying:

Then in the app, show a dialog when the foreign-key-constraint error throws asking what you want to do with the related items, and handle the changes on the app side before deleting the item

@andriusign
Copy link
Contributor

andriusign commented Nov 25, 2021

Well that only applies if you use the app, but if you use API, then there is no dialog, just a call to back-end. How would this then work?

@rijkvanzanten
Copy link
Member

@andriusign At that point, it's up to whatever uses the API to resolve the constraint before continuing the original operation, which is exactly what the app would do as well

@shankiflang

This comment was marked as resolved.

@rijkvanzanten

This comment was marked as resolved.

@rijkvanzanten
Copy link
Member

Linear: ENG-288

@JonathanDoelan
Copy link

Is there a workaround to prevent relating users to files? In terms of GDPR we are not allowed to store user data longer than actually needed. What is a good solution for now?

@paescuj
Copy link
Member

paescuj commented Feb 16, 2023

Is there a workaround to prevent relating users to files? In terms of GDPR we are not allowed to store user data longer than actually needed. What is a good solution for now?

Hmm, you might be able to clear out the uploaded_by field with a Flow or Custom Hook. The field is probably required though, so one possibility might be to assign all uploaded files to a generic user.

@JonathanDoelan
Copy link

Is there a workaround to prevent relating users to files? In terms of GDPR we are not allowed to store user data longer than actually needed. What is a good solution for now?

Hmm, you might be able to clear out the uploaded_by field with a Flow or Custom Hook. The field is probably required though, so one possibility might be to assign all uploaded files to a generic user.

Thanks!
Would it also be possible to write a hook extension? Is there an event like user:delete we can bind this behaviour`

@paescuj
Copy link
Member

paescuj commented Feb 16, 2023

Is there a workaround to prevent relating users to files? In terms of GDPR we are not allowed to store user data longer than actually needed. What is a good solution for now?

Hmm, you might be able to clear out the uploaded_by field with a Flow or Custom Hook. The field is probably required though, so one possibility might be to assign all uploaded files to a generic user.

Thanks! Would it also be possible to write a hook extension? Is there an event like user:delete we can bind this behaviour`

Oh yeah, that might work as well. There's a <system-collection>.delete event, see https://docs.directus.io/extensions/hooks.html#filter-events.

@JonathanDoelan
Copy link

JonathanDoelan commented Feb 16, 2023

I have written this extension:

// index.js
export default ({ filter }, { services, getSchema }) => {
    const { FilesService } = services;

    filter("users.delete", async (input, meta, context) => {
        const usersToDelete = input;
        const {
            accountability: { user },
        } = context;
        const filesService = new FilesService({
            ...context,
            schema: await getSchema(),
        });

        await filesService.updateByQuery(
            { filter: { uploaded_by: { _in: usersToDelete } } },
            { uploaded_by: user }
        );
        await filesService.updateByQuery(
            { filter: { modified_by: { _in: usersToDelete } } },
            { modified_by: user }
        );

        return input;
    });
};

Feel free to use it as a workaround, as long as there is no better solution for this ;)

What it does, is to replace all user ids in uploaded_by and modified_by with the id of the user who triggers the deletion.

What do you think?

@paescuj
Copy link
Member

paescuj commented Feb 17, 2023

I have written this extension:

// index.js
export default ({ filter }, { services, getSchema }) => {
    const { FilesService } = services;

    filter("users.delete", async (input, meta, context) => {
        const usersToDelete = input;
        const {
            accountability: { user },
        } = context;
        const filesService = new FilesService({
            ...context,
            schema: await getSchema(),
        });

        await filesService.updateByQuery(
            { filter: { uploaded_by: usersToDelete } },
            { uploaded_by: user }
        );
        await filesService.updateByQuery(
            { filter: { modified_by: usersToDelete } },
            { modified_by: user }
        );

        return input;
    });
};

Feel free to use it as a workaround, as long as there is no better solution for this ;)

What it does, is to replace all user ids in uploaded_by and modified_by with the id of the user who triggers the deletion.

What do you think?

Looks good! Thanks for reporting back 👍

@anthlasserre

This comment was marked as off-topic.

@rijkvanzanten

This comment was marked as resolved.

@anthlasserre
Copy link

@anthlasserre Looks unrelated to the message in the original post. Mind opening a new issue? Ty!

Hey @rijkvanzanten 👋🏼 Thanks for the quick answer and sorry about some details missing in my message above. I think it is related actually by checking Directus logs.

ERROR (43 on 9fdd1f4394ee): delete from "directus_users" where "id" in ($1) - update or delete on table "directus_users" violates foreign key constraint "directus_files_uploaded_by_foreign" on table "directus_files"
    err: {
      "type": "DatabaseError",
      "message": "delete from \"directus_users\" where \"id\" in ($1) - update or delete on table \"directus_users\" violates foreign key constraint \"directus_files_uploaded_by_foreign\" on table \"directus_files\"",
      "stack":
          error: delete from "directus_users" where "id" in ($1) - update or delete on table "directus_users" violates foreign key constraint "directus_files_uploaded_by_foreign" on table "directus_files"
              at Parser.parseErrorMessage (/directus/node_modules/pg-protocol/dist/parser.js:287:98)
              at Parser.handlePacket (/directus/node_modules/pg-protocol/dist/parser.js:126:29)
              at Parser.parse (/directus/node_modules/pg-protocol/dist/parser.js:39:38)
              at Socket.<anonymous> (/directus/node_modules/pg-protocol/dist/index.js:11:42)
              at Socket.emit (node:events:513:28)
              at addChunk (node:internal/streams/readable:315:12)
              at readableAddChunk (node:internal/streams/readable:289:9)
              at Socket.Readable.push (node:internal/streams/readable:228:10)
              at TCP.onStreamRead (node:internal/stream_base_commons:190:23)
      "length": 364,
      "name": "error",
      "severity": "ERROR",
      "code": "23503",
      "detail": "Key (id)=(**********************) is still referenced from table \"directus_files\".",
      "schema": "public",
      "table": "directus_files",
      "constraint": "directus_files_uploaded_by_foreign",
      "file": "ri_triggers.c",
      "line": "2478",
      "routine": "ri_ReportViolation"
    }

@ericlemans
Copy link

Hi all! Is this still being looked into? I'm having the same issue... I have to manually delete the uploaded files from that particular user before I can delete the user itself...

@azrikahar azrikahar mentioned this issue Mar 9, 2023
3 tasks
@rijkvanzanten
Copy link
Member

Hi all! Is this still being looked into? I'm having the same issue... I have to manually delete the uploaded files from that particular user before I can delete the user itself...

Yup! This is an Open Issue, so something we aim to resolve at some point 👍🏻

@Matthewk01
Copy link

Matthewk01 commented Mar 28, 2023

Hello! For anyone wondering how to solve this issue using Flows without needing to do any changes in source code of Directus, here you go:

Basically, you create a flow which hooks into the delete user event and handles modified_by and uploaded_by file attributes before deleting the user.

  1. In settings, create a new flow with a following trigger:
  • Trigger: Event hook
  • Type: Filter (Blocking)
  • Scope: items.delete
  • Collections: Directus Users
  • Response: All data
  1. Create a new action:
  • Type: Update data
  • Collection: directus_files
  • Permissions: From Trigger
  • Payload:
{
    "modified_by": "{{$accountability.user}}"
}
  • Query:
{
    "filter": {
        "modified_by": {
            "id": {
                "_eq": "{{$trigger.payload[0]}}"
            }
        }
    }
}
  • This results in changing modified_by attribute of files modified by user we are deleting to the user triggering the flow.
  1. Create another action with same preset but:
  • Payload:
{
    "uploaded_by": "{{$accountability.user}}"
}
  • Query:
{
    "filter": {
        "uploaded_by": {
            "id": {
                "_eq": "{{$trigger.payload[0]}}"
            }
        }
    }
}
  • This results in changing uploaded_by attribute of files uploaded by user we are deleting to the user triggering the flow.

@dzulic
Copy link

dzulic commented Jun 7, 2023

Hello, I have an issue when trying to remove user that is no longer part of a company, but can't as it has created some resources.

It is a security issue, so was wondering if this is going to be picked up anytime soon?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 📋 Backlog
Development

No branches or pull requests