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

UUIDs should be unique, and must not be reused, even after deletion #1741

Closed
mapkyca opened this issue May 9, 2017 · 8 comments · Fixed by #1745
Closed

UUIDs should be unique, and must not be reused, even after deletion #1741

mapkyca opened this issue May 9, 2017 · 8 comments · Fixed by #1745

Comments

@mapkyca
Copy link
Member

mapkyca commented May 9, 2017

While trying to do this:

  • Create an object with a generated uuid e.g. example.com/foo+bar
  • Reference it from another object
  • Delete example.com/foo+bar
  • Create a new object, with new associated data, but with a generated uuid example.com/foo+bar
  • Follow reference from your object... bingo, your old object is referencing the newly created foo+bar, with associated different data and metadata.

I encountered this error:

UUIDs should be Universally Unique, meaning that it should not be possible to create, delete, and recreate an entirely new object with a UUID of a previously deleted object.

This causes problems for membership lists for things like subscription groups, and potentially could be a security problem if a new ACL is created with the same UUID as an old one, since ACL membership is referenced by UUID.

@mapkyca mapkyca changed the title Tomb-stone old UUIDs so the can't be reused. Tombstone old UUIDs so the can't be reused. May 9, 2017
@mapkyca mapkyca changed the title Tombstone old UUIDs so the can't be reused. UUIDs should be unique, and must not be reused, even after deletion May 9, 2017
@mapkyca
Copy link
Member Author

mapkyca commented May 9, 2017

Two possible solutions to this:

  1. We introduce some randomness into UUID generation so that they always are generated with a unique ID - this happens to a certain extent, unless you allow for slug generation, UUIDs are generated with the object's ID... so for these UUIDs uniqueness after deletion is preserved. However, this solution will make for some ugly urls.
  2. Tombstone deleted objects, either by maintaining a list of deleted UUIDs to check against, or by flagging deleted objects as deleted (and modifying all database retrieval functions to ignore such objects by default - except, obviously, when checking for existing objects for UUID creation) perhaps using the publish_status field. Modify delete to do a soft delete by default (with the option to really remove as well. This latter method would allow for undelete, which could also be useful.

@mapkyca
Copy link
Member Author

mapkyca commented May 9, 2017

Ahh... maybe we do need a delete flag on the schema... that way we seamlessly handle folk who've used !='draft' in db listings

@benwerd
Copy link
Member

benwerd commented May 10, 2017

Using URIs as UUIDs was an idiot mistake and I wish I hadn't done it.

At this point I don't care if UUIDs are ugly at all (although URLs should be readable), and I think ripping off the bandaid for a more robust internal system would be good.

My position is that all the early stuff around federation should be fully ripped out.

@mapkyca
Copy link
Member Author

mapkyca commented May 10, 2017

Thinking that 1) can't be done, since UUIDs can change after save or if a url format is provided (separate problem).

So, my feeling is tombstoning... a lookup of UUIDs past is probably the quickest, but the delete flag may be the most useful... hmm...

@mapkyca
Copy link
Member Author

mapkyca commented May 10, 2017

Another issue is that generate resilient slug has no concept of UUIDs (although it is used to generate them)... this means that it's going to be very hard to have the lookup (which almost certainly has to be done in save) to result in a transparent recovery... basically we're just going to have to throw an exception, which is far from ideal... but at least means we don't get data corruption.

So, maybe, since it's just slug generation here that's the problem, make that check against tombstoned objects (soft deleted or otherwise)... getURL will still be a problem, but at least if they're doing lookup on slug, then that will still result in a canonical value.

mapkyca added a commit to mapkyca/idno that referenced this issue May 11, 2017
mapkyca added a commit to mapkyca/idno that referenced this issue May 11, 2017
mapkyca added a commit to mapkyca/idno that referenced this issue May 11, 2017
mapkyca added a commit to mapkyca/idno that referenced this issue May 11, 2017
mapkyca added a commit to mapkyca/idno that referenced this issue May 11, 2017
mapkyca added a commit to mapkyca/idno that referenced this issue May 11, 2017
mapkyca added a commit to mapkyca/idno that referenced this issue May 11, 2017
mapkyca added a commit to mapkyca/idno that referenced this issue May 11, 2017
mapkyca added a commit to mapkyca/idno that referenced this issue May 11, 2017
mapkyca added a commit to mapkyca/idno that referenced this issue May 11, 2017
mapkyca added a commit to mapkyca/idno that referenced this issue May 11, 2017
mapkyca added a commit to mapkyca/idno that referenced this issue May 11, 2017
mapkyca added a commit to mapkyca/idno that referenced this issue May 11, 2017
mapkyca added a commit to mapkyca/idno that referenced this issue May 11, 2017
@mapkyca
Copy link
Member Author

mapkyca commented May 11, 2017

Hmmm.. of course, doing this check globally causes problems with users, who's handle forms part of their UUID - i.e, if I create user "marcus", delete him, it would not be possible to reuse "marcus"... which I guess might be desirable...

Thoughts @benwerd?

@mapkyca
Copy link
Member Author

mapkyca commented May 11, 2017

A possibility/fudge might be that users deletes also remove the tombstone... but that seems a little... wrong... not sure.

Thoughts?

@mapkyca
Copy link
Member Author

mapkyca commented Aug 24, 2017

Related to #1864, as that could result in duplicate UUIDs when #1864 comes in to play

mapkyca added a commit to mapkyca/FixSlugBug that referenced this issue Aug 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants