<!--IssueSummary start--> <details> <summary> Everyone can contribute. [Help move this issue forward](https://handbook.gitlab.com/handbook/marketing/developer-relations/contributor-success/community-contributors-workflows/#contributor-links) while earning points, leveling up and collecting rewards. </summary> - [Label this issue](https://contributors.gitlab.com/manage-issue?action=label&projectId=278964&issueIid=582452) </details> <!--IssueSummary end--> Making an epic to describe current status of open issues and MRs ### High-level summary In Cells, we cannot have globally UNIQUE indexes in Postgres, since different Cells will each have their own Postgres instances. Globally-unique elements need to be claimed in Topology Service, and this should be as rare as possible. UNIQUE indexes should be scoped to the table's sharding key wherever possible. Toward this end, we identified 4 indexes owned by ~"group::authentication" that need to be ready for Protocells. We need to migrate these indexes to be unique-per-sharding key: `organization_id` for the `users` table and `user_id` for the `emails` table. | Task | Issue | Merge Request(s) | Notes | | ------ | ------ | ------ | ------ | | Update routes sent in password-reset, confirm-email, and unlock-account emails to ensure routing info is present | | https://gitlab.com/gitlab-org/gitlab/-/merge_requests/209346 | Merged | | Revert above | | https://gitlab.com/gitlab-org/gitlab/-/merge_requests/212005 | Merged | | Update above routes, v2 | | https://gitlab.com/gitlab-org/gitlab/-/merge_requests/212116 | Merged | | Clean up FF `devise_email_organization_routes` | https://gitlab.com/gitlab-org/gitlab/-/issues/580586 | https://gitlab.com/gitlab-org/gitlab/-/merge_requests/213743 | Merged | | De-scope `users.password_reset_token` | https://gitlab.com/gitlab-org/gitlab/-/issues/562092 | https://gitlab.com/gitlab-org/gitlab/-/merge_requests/214292 | :raising_hand: Awaiting re-review by `@bdenkovych` over blocking concerns | | De-scope `users.unlock_token` | https://gitlab.com/gitlab-org/gitlab/-/issues/562094 | https://gitlab.com/gitlab-org/gitlab/-/merge_requests/211988 | :raising_hand: Awaiting re-review by `@bdenkovych` over blocking concerns | | De-scope `users.confirmation_token` | https://gitlab.com/gitlab-org/gitlab/-/issues/562090 | https://gitlab.com/gitlab-org/gitlab/-/merge_requests/214348 | :hourglass_flowing_sand: In discussion with `@bdenkovych` re: approach | | De-scope `emails.confirmation_token` | https://gitlab.com/gitlab-org/gitlab/-/issues/562035 | https://gitlab.com/gitlab-org/gitlab/-/merge_requests/214499 ( requires merging https://gitlab.com/gitlab-org/gitlab/-/merge_requests/214348 , https://gitlab.com/gitlab-org/gitlab/-/merge_requests/213743 , and https://gitlab.com/gitlab-org/gitlab/-/merge_requests/214600 first ) | :hourglass_flowing_sand: In discussion with `@bdenkovych` re: approach | | Fix context helpers for `with_current_organization` (no longer blocking other MRs; spec workarounds applied) | N/A | https://gitlab.com/gitlab-org/gitlab/-/merge_requests/214324 | Merged | | Add `user_id=` to secondary email confirmation links | N/A | https://gitlab.com/gitlab-org/gitlab/-/merge_requests/214600 ( requires merging https://gitlab.com/gitlab-org/gitlab/-/merge_requests/213743 first ) | :hourglass_flowing_sand: In discussion with `@bdenkovych` re: approach | ### Sequencing Updating the `emails.confirmation_token` is complex. Unlike `users` , the `emails` table is sharded by `user_id` instead of `organization_id` . This means we can't just use `Current.organization` to look up a secondary email address by its `confirmation_token` . We need to pass a `user_id` parameter to the email confirmation action, then modify the confirmation action to use this parameter. To reduce merge conflicts and avoid sequencing problems, I planned the following order: 1. https://gitlab.com/gitlab-org/gitlab/-/merge_requests/213743 - clean up the FF affecting Devise email action routes 2. https://gitlab.com/gitlab-org/gitlab/-/merge_requests/214600 - add `user_id` parameter to secondary email confirmation templates - it is not used yet 3. https://gitlab.com/gitlab-org/gitlab/-/merge_requests/214348 - de-scope `users.confirmation_token` . This modifies `ConfirmationsController` to use different queries based on the type of `confirmable` resource being actioned 4. https://gitlab.com/gitlab-org/gitlab/-/merge_requests/214499 - de-scope `emails.confirmation_token` - this extends the modifications in the above step to properly handle `emails` using the `user_id` parameter added previously. Deploying this later than the `user_id` changes will ensure the confirmation continues working as expected when deployed, and doesn't require users to re-send the confirmation instructions ### Challenges #### Duplicate Indexes Per discussion with ~database in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/211988#note_2900205309 - the new indexes on `users` create a "duplicate index" for `organization_id` . The new index on `emails` creates a duplicate index for `user_id` . These are performance-critical indexes, and we want to ensure the new indexes are created successfully and the new code is fully functional before cleaning up the old indexes. The de-scoping MRs all create entries in `spec/support/helpers/database/duplicate_indexes.yml` to get the specs passing, which is likely to lead to merge conflicts once they start getting merged. #### Test route helpers Currently, organization-scoped route helpers cannot be used in specs. We can call `user_confirmation_path` , but not `organization_user_confirmation_path` in specs. We can still stub out `Current.organization` , so the path may not actually matter when doing controller, request, and feature specs. But it would be better to use the paths that will be used in production for these requests. I have attempted to fix here: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/214324 . I judged this non-critical to the other MRs, since we can stub out `Current.organization` , and should not be manually finding an Organization from the route parameters. #### Feature Flags The previous indexes are all `UNIQUE` ones, so we won't actually know if the de-scoped indexes are fully functional until those are removed. Because we are dealing with indexes on the `users` table, we want to ensure that the actively-running code is using indexed queries for Devise actions. So if we wanted to disable a feature flag to turn off the new code paths, we would need to ensure that the previous indexes on bare `confirmation_token` and `reset_password_token` (without the sharding keys) are still present. Thus it is hard to implement feature flags, since we would have to not clean up the previous indexes while creating new ones. #### Incident Previously, we attempted to modify all email routes to explicitly use organization-scoped routes: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/209346 This caused a problem with the secondary email confirmation flow due to the route helpers not working as expected, and a test gap in end-to-end coverage of these flows. We fixed and re-deployed in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/212116 with a feature flag: https://gitlab.com/gitlab-org/gitlab/-/work_items/580586 . This fixed the confirmation flow, but caused another unexpected issue. After resetting their passwords, users were redirected to an organization-scoped sign-in page for the `default` organization, due to the changes in global route helpers between the two MRs: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/209311 . The organization-scoped sign-in page was not ready yet and threw an error. Due to this, we disabled the feature flag and decided to remove the feature-flagged code in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/213743 Going forward, we do not need to explicitly use organization-scoped routes. Using normal route helpers will allow them to be scoped to the correct organization when in a non-default org.