Skip to content

Deleting a note should really delete the note in database #766

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

Open
dekonnection opened this issue Mar 15, 2018 · 12 comments
Open

Deleting a note should really delete the note in database #766

dekonnection opened this issue Mar 15, 2018 · 12 comments
Labels
database/sequelize Somehow this is related to the database or ORM discussion Exchange of some opinions needed

Comments

@dekonnection
Copy link

dekonnection commented Mar 15, 2018

Hi,

Currently, it seems like deleting a note just marks the note as deleted (by adding the deletion date in the deletedAt field), but keep the note in the database.

Since all notes are stored in plaintext, I know that the database administrator is able to see all the active notes, so user privacy is limited, but I think that a deleted note shouldn't be available, like, ever.

Demonstration :

mysql> select * from Notes where content like '%mysecretnote%';
+--------------------------------------+-----------+-------+------------+-----------+----------+----------------------------------------+-----------------------------------------------------------------------------+---------------------+---------------------+---------------------+---------------------+-----------+--------------------------------------+--------------------------------------+
| id                                   | shortid   | alias | permission | viewcount | title    | content                                | authorship                                                                  | lastchangeAt        | savedAt             | createdAt           | updatedAt           | deletedAt | ownerId                              | lastchangeuserId                     |
+--------------------------------------+-----------+-------+------------+-----------+----------+----------------------------------------+-----------------------------------------------------------------------------+---------------------+---------------------+---------------------+---------------------+-----------+--------------------------------------+--------------------------------------+
| 8b530b18-0796-4833-a213-81189cadef20 | SJR4bZuKz | NULL  | private    |         0 | Untitled | mysecretnote that I don't want anymore | [["8c158b3e-ce0b-4550-8347-4f533f50aaba",0,38,1521123644145,1521123662902]] | 2018-03-15 14:21:03 | 2018-03-15 14:20:38 | 2018-03-15 14:20:38 | 2018-03-15 14:21:03 | NULL      | 8c158b3e-ce0b-4550-8347-4f533f50aaba | 8c158b3e-ce0b-4550-8347-4f533f50aaba |
+--------------------------------------+-----------+-------+------------+-----------+----------+----------------------------------------+-----------------------------------------------------------------------------+---------------------+---------------------+---------------------+---------------------+-----------+--------------------------------------+--------------------------------------+
1 row in set (0.00 sec)

After deletion :

mysql> select * from Notes where content like '%mysecretnote%';
+--------------------------------------+-----------+-------+------------+-----------+----------+----------------------------------------+-----------------------------------------------------------------------------+---------------------+---------------------+---------------------+---------------------+---------------------+--------------------------------------+--------------------------------------+
| id                                   | shortid   | alias | permission | viewcount | title    | content                                | authorship                                                                  | lastchangeAt        | savedAt             | createdAt           | updatedAt           | deletedAt           | ownerId                              | lastchangeuserId                     |
+--------------------------------------+-----------+-------+------------+-----------+----------+----------------------------------------+-----------------------------------------------------------------------------+---------------------+---------------------+---------------------+---------------------+---------------------+--------------------------------------+--------------------------------------+
| 8b530b18-0796-4833-a213-81189cadef20 | SJR4bZuKz | NULL  | private    |         0 | Untitled | mysecretnote that I don't want anymore | [["8c158b3e-ce0b-4550-8347-4f533f50aaba",0,38,1521123644145,1521123662902]] | 2018-03-15 14:21:03 | 2018-03-15 14:20:38 | 2018-03-15 14:20:38 | 2018-03-15 14:21:03 | 2018-03-15 14:21:18 | 8c158b3e-ce0b-4550-8347-4f533f50aaba | 8c158b3e-ce0b-4550-8347-4f533f50aaba |
+--------------------------------------+-----------+-------+------------+-----------+----------+----------------------------------------+-----------------------------------------------------------------------------+---------------------+---------------------+---------------------+---------------------+---------------------+--------------------------------------+--------------------------------------+
1 row in set (0.01 sec)

Thanks for your work :)

@jackycute
Copy link
Member

Hi @dekonnection
That's part of current design, it's called soft delete.
In normal ORM query, these notes won't show up unless you specified paranoid: false.

@SISheogorath SISheogorath added the database/sequelize Somehow this is related to the database or ORM label Mar 15, 2018
@dekonnection
Copy link
Author

While understanding the design, I'm still a little bit concerned, since after deleting a note, the content is secretly kept and the user don't have control on it anymore.

I think that providing a "secure delete" setting (when activated, deletion really drops the note) could be a good thing.
Or maybe automatically erasing the content field before marking the note as deleted ?

@ccoenen
Copy link
Contributor

ccoenen commented Mar 15, 2018

Yes, we should have some kind of process that will delete notes after some kind of grace period. We should also scrub related tables like revisons.

@ccoenen
Copy link
Contributor

ccoenen commented Mar 15, 2018

That said: if you're not trusting the server administrator, you should keep in mind: they could have backups (or other ways of reconstructing the note) even if it was deleted.

From a user's perspective i would not distinguish between different types of deletions, this will just confuse people. I would keep soft-delete as default and offer some means of "undo" for some period of time. Right now i'm thinking 1 hour, but this might not be a perfect fit for everyone, either.

@dekonnection
Copy link
Author

dekonnection commented Mar 15, 2018

@ccoenen I agree with everything you said 👍

The idea of a "recycle bin" is great, along with a configurable grace period.

Concerning your (valid) point about past backups or reconstruction means, I also agree, but I think it's still not a justification to voluntarily keep the full note in database, subject to future backups :)

@SISheogorath SISheogorath added the discussion Exchange of some opinions needed label Mar 15, 2018
@SISheogorath
Copy link
Contributor

Well, from a technical point of view, I can say the following:
https://sequelize.readthedocs.io/en/v3/docs/models-definition/#configuration

It is possible to delete notes. The question is can we really make it configurable without a migration?

And if we do, will it delete all previously deleted notes? Someone want to write a PoC and see how it works?

@ccoenen
Copy link
Contributor

ccoenen commented Mar 19, 2018

I don't see why we would need a migration at all? The way I see it, we need two pieces of code:

  • Garbage-Collector (i.e. select everything with deletedAt < now() - 1h -> hard-delete those)
  • Restore-Interface (i.e. some way of saying "OH SH*T I WASN'T SUPPOSED TO DELETE THAT JUST YET" which would unset the deletedAt field, i.e. restoring it to a regular note.

It's debatable how the garbage-collector should be implemented, personally, I could totally live with a separate script (like the user management script) that one has to run via cron. But this also adds another moving part.

@SISheogorath
Copy link
Contributor

This was added with #830 we only need to add some later clean up.

@lephix
Copy link

lephix commented May 22, 2019

Hi @dekonnection
That's part of current design, it's called soft delete.
In normal ORM query, these notes won't show up unless you specified paranoid: false.

@jackycute

I am using latest image hackmdio/hackmd in Docker. When the note is delete by owner, the note is physically delete in table notes, not a soft delete.

@jackycute
Copy link
Member

Hi @lephix
Soft deletion was changed in PR #830 which was for GDPR compliant.

@lephix
Copy link

lephix commented May 23, 2019

@jackycute
I am not familiar with GDPR. But for enterprise, I think data need be tracked. Maybe CodiMD could provide a configuration for the "soft deletion". By default it could be disabled.
I don't know it would violate GDPR or not?

@brlin-tw
Copy link
Contributor

@jackycute

Hi @lephix
Soft deletion was changed in PR #830 which was for GDPR compliant.

Does that imply this issue is resolved? Or some UI changes is still necessary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database/sequelize Somehow this is related to the database or ORM discussion Exchange of some opinions needed
Projects
None yet
Development

No branches or pull requests

6 participants