jkomoros
commented
2 years ago - [x] Disallow commiting a card edit in validateCardDiff or similar that adds or removes a reference to a card that the editor can't currently see
- [x] Security rules for people without edit access to the inbound cards to be able to add or remove inbound references (as long as they have edit access to the card they are adding or removing th einboudn reference to)
- [x] Wait, there's a pre-existing bug in updateInboundLinks, where if you remove a single reference to a card but there is still another reference to that card of another type, it still (erroneously) removes the reference boolean to that card
- [x] Strengthen security rules to actually check userMayEditCard for otherCardID
- [x] Think through: there's a case where, in a batch, one card removes a reference, and one card adds it and they conflict, right?
- [x] Forking a card for someone without blanket edit access fails (and has for awhile)
- [x] Remove selectRawCards use in forking and modifyCard, unexport selectRawCards
- [x] Forking a card should also verify it can edit all tags (and remove the ones it can't with a confirm, similar to how we handle outbound references)
- [x] Forked cards also need to have their inbound links updated on creation
- [x] A hover over disabled tags explaining WHY they're disabled
- [x] Strengthen security rules so we check, using getAfter, that the other card modification matches this one
- [x] Make it so that you also can't remove a reference to a hidden card in the UI (perhaps disable that tag with a tooltip of why it can't be removed)
- [x] Make it so the updateInboundLinks logic is done clientside
- [x] Rip out the updateInboundLink thing (might require a maintenace task or manual step to delete the cloud function from production)
- [x] Rip out cardUpdateOverlay
- [x] Rip out the status cloud function endpoint and clientside use
- [x] Rip out maintenanceMode
- [ ] Delete status, updateInboundLinks in production
firebase functions:delete updateInboundLinks status
We use cloud functions on card writes, mainly to fix up and update inboundLinks.
The reason it's a cloud function is to verify that even badly behaved clients can't mess up the two-way references (outbound and inbound), but also because technically it's possible for an editor of a card to add or remove a reference to a card they aren't able to see (let alone edit). But the machinery is a total pain in the butt--there's a lot of awkward code sharing going on for that function, and the fact that each edited card has its handlers one run at a time, roughly once per second, means that after a multi-edit, we'll get a stream of disruptive card-updates, one at a time, one per second, and each time ruining all of the expensive caches clientside and leading to a whole pipeline recreation.
The last odd bit of #581 is how to do an elaborate dance to work around this behavior. But honestly, it's probably just simpler to rip out the cloud function in the first place and do it all clientside anyway, we already do most of the calculation of inbound links updates in
modifyCardWithBatch. If we did this, then we'd be able to get rid of the whole card overlay system and instead just have the immediate firestore onSnapshot handle everything... and in updateCards, just ignore timestamps. We'd also be able to rip out the odd server-sidemaintenanceModethat effectively just turns off inbound links.The one downside is there's an edge case noted above about adding or removing references to cards the current user can't see... but that's very very rare, and only is affected when one of those is added or removed, not when one simply persists across an edit. For those, we can just disallow removing references to cards we can't see (it's already impossble int the UI to add a reference to a card you can't see.)