Implement authorization data recovery for deleted users #9463
Conversation
|
@andreslucena I will still work on the feature which would show the different records that were recovered from the old account but I opened the PR already so that you can take a look and check whether the general approach seems fine for you. Could you please check this initially whether the approach seems fine to you? |
Yes, I like the current approach. I had some doubts, but I think that it's because of the current lack of user story definition. I'd need to give a spin with @carolromero, about what's the user expectation - or at least the behavior that brings less surprise on this. With the current approach, a user after reauthorizing herself will have these old contents:
What I had in mind is to recover only the Votes/Orders, as they're the ones that make sense, at least for the use case of "respecting the one person one vote rule" - so that'd be:
But as I mentioned, I'd need to discuss this first with @carolromero (and probably with @decidim/product in our weekly meeting) as there are for sure some use cases that I'm not seeing |
|
Thanks for the feedback @andreslucena !
I think you already know this but also to state it publicly here that the initial idea with this PR was to bring back all data that MIGHT require authorization. Of course, this will most likely also include some data that does not require authorization because we don't know if the authorization configuration has changed at some point which is why the current implementation brings back everything. So this was the initial thinking behind this that if we are bringing back some data that might require authorization, we would need to bring back all data that require authorization. The alternative approach (bring back votes only) might open other "holes" (sort of speak) in the authorization functionality, such as being able to submit as many proposals as the user wants in proposal components where the maximum amount per participant is configured e.g. at 5 proposals or being able to answer to a survey unlimited amount of times when the survey component requires authorization. Similar "holes" might also appear in the future in case similar options are added, e.g. how many debates the user can start, how many meetings the user can submit, etc. Just to note here that we are all on the same page. |
This is what I meant when I said "there are for sure some use cases that I'm not seeing" 😄 I agree with the "holes" explanation. Let me give it a second thought with the rest of @decidim/product. I understand the rationale, but it could be really weird as a participant deleting my account and recovering all this data. It'd weird just to recover the votes, but having all these other resources is on another league. Probably by tuning the deletion/recovery process is enough though. |
We've just talked today about this and everyone agreed that this approach is OK. Let me know if you need anything else from our side. |
In order to store information about the authorization transfers, create active record objects to store this information.
|
@ahukkanen can you solve the merge conflicts 🙏🏽? |
|
Done @andreslucena |
|
@ahukkanen I've reviewed the general feature and I say that's really well done, congratulations 👏🏽 👏🏽 Regarding my main concerns on UX while thinking about the initial feature, they're both really covered with the before message and the after message with the HTML flash. This will be even better with the redesign of the flash. I think I found a bug outside of the happy path, regarding the "Authorizations revocation" admin feature (/admin/authorization_workflows).
Can you check it out please? |
|
|
||
| initializer "decidim_meetings.authorization_transfer" do | ||
| Decidim::AuthorizationTransfer.register(:meetings) do |transfer| | ||
| transfer.move_records(Decidim::Meetings::Meeting, :decidim_author_id) |
There was a problem hiding this comment.
For what I've seen we don't have a permission for Meeting creation, although for consistency I can expect that some time in the future we want to have it. Does that make sense @carolromero?
There was a problem hiding this comment.
I also think for consistency it would be good to keep here, if we are anyways transferring data, so it may feel weird that some data was not actually transferred.
In my opinion this feature is more about data recovery in general, as we know that data was originally created by the same person based on the authorization.
@andreslucena This issue is now fixed. I am deleting the authorization transfers along with the authorization. I don't think it makes sense to preserve them if we are already deleting the parent record (i.e. the authorization itself). |
* feature/redesign-turbo: Install turbo-rails Fix incorrect expecations after change of allowed file extensions (#9877) Update `@joeattardi/emoji-button` to `@picmo/popup-picker` (#9667) Address Crowdin feedback (#9678) Fix cryptic file validation errors (#9663) Fix disappearing sub-lists in rich text editors (#9867) Implement authorization data recovery for deleted users (#9463) Redesigned: admin bar (#9874)
* Implement data recovery when a deleted user has authorization conflict * Add specs to test the authorization transfer in different modules * Add a spec for the authorizations message during account deletion * Create authorization transfers and associated records In order to store information about the authorization transfers, create active record objects to store this information. * Update the authorization transfer to the updated API * Add test factories for the authorization transfers * Show information about the authorization transfer to the user * Update the engine specific authorization transfers to the new API * Report the coauthorable resource types correctly * Lint * Check that the transferred event has the transfer parameter * Improve the block registry's unregister method * Add specs for the unregister method at the block registry * Add YARD docs for the BlockRegistry * Add method to disable the authorization transfer functionality * Implement a better way of disabling the functionality * Handle the disabled case properly at the authorize user command * Register conflict with duplicates if authorization transfers are disabled * Add CHANGELOG notes * Add comment votes to the authorization transfers * Fix string sorting for the authorization record transfer engine specs String sorting causes e.g. record with ID 10 to be sorted before record with ID 9 because 1 < 9. Fix this issue by padding the IDs with leading zeros for the string sorting for them to be always in correct order. * Add endorsements and amendments to the authorization transfers * Add the new transfer resources to CHANGELOG and improve the docs - Add new transfer resources to the CHANGELOG, i.e. amendments, endorsements and comment votes - Add also module-specific descriptions of the transferred resources if the actial transfer is handled by another module - Improve the transfer resources list documentation through nested lists for each module * Add extra note for module developers at CHANGELOG * Add CHANGELOG example of unregistering multiple modules at once * Rename the method to fetch the resource type to avoid naming conflict The endorsement records already defined a method named `resource_type` so rename the method to `mapped_resource_type` to fetch the mapped resource for the authorization transfers. * Add mapped_resource_type method to the amendment records * Test amendments and endorsements in authorization transfer presenter * Test information method with amendments and endorsements * Change the way the authorization handler is provided to the blocks * Document the delegated methods * Improve documentation * Clarify that automated transfers only happen for deleted users * Change the added methods in AuthorizationHandler to use YARD in docs * Improve the HtmlSafeFlash example documentation * Add the param documentation for Authorization#transfer! * Fix the hash type documentation according to YARD types * Add a line break to the CHANGELOG section * Rubocop auto-correct * Fix the section order in CHANGELOG * Fix revoking authorizations with transfers * Fix revoking authorizations with transfers that have transfer records * Rephrase the code comment as more understandable * Add a system spec for testing an authorization conflict situation * Fix typo Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com> Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
🎩 What? Why?
Implements a recovery procedure for data mapped to a conflicting authorization record for a deleted user as described in #9179.
This procedure will recover all data that can have authorization permission requirements on them but it won't check whether each component/record has been configured to require authorization for the authorizations to be recovered because of few reasons:
So for example, the budgeting component could allow the user casting a vote without an authorization. Later, the admin could notice that they forgot to set the authorization rules to the component and they fix this situation for the second day of voting. Now the user already cast a vote which did not require any authorization but any further votes will require authorization. We do not store any information that would allow the budgeting vote/order itself to tell whether it required authorization or not when it was cast.
Therefore, to avoid unnecessary complexity with the transfer procedures, it is assumed that all records that may or may not require an authorization needs to be transferred over to the new account.
This PR also introduces the
AuthorizationTransferandAuthorizationTransferRecordmodels into the core in order to trace back the transfers and store information about the transferred records. The amount of each transferred record with the record type is showed to the user when they authorize their account and data was transferred (see the screenshots).It is also possible to disable the transfer functionality completely or disable the record transfers for a specific module. See the added CHANGELOG entries for explanation of the feature and further information about the changes.
📌 Related Issues
Testing
user@example.orgunique_id(i.e. the "document number") using the example authorization📋 Checklist
docs/.📷 Screenshots
Before deleting the account that has an authorization, the following extra message will be displayed at the delete account page (visible only when the user has authorizations):
After a successful authorization which was transferred from a deleted user, the user will see the amount of each transferred record type in the flash message: