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

feat: account linking #742

Merged
merged 149 commits into from
Sep 20, 2023
Merged

feat: account linking #742

merged 149 commits into from
Sep 20, 2023

Conversation

rishabhpoddar
Copy link
Member

@rishabhpoddar rishabhpoddar commented Jul 7, 2023

Summary of change

Base PR for account linking related changes in the core

Related issues

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your
changes work. Bonus points for screenshots and videos!)

Documentation changes

(If relevant, please create a PR in our docs repo, or create a checklist here
highlighting the necessary changes)

Checklist for important updates

  • Changelog has been updated
    • If there are any db schema changes, mention those changes clearly
  • coreDriverInterfaceSupported.json file has been updated (if needed)
  • pluginInterfaceSupported.json file has been updated (if needed)
  • Changes to the version if needed
    • In build.gradle
  • If added a new paid feature, edit the getPaidFeatureStats function in FeatureFlag.java file
  • Had installed and ran the pre-commit hook
  • If there are new dependencies that have been added in build.gradle, please make sure to add them
    in implementationDependencies.json.
  • Update function getValidFields in io/supertokens/config/CoreConfig.java if new aliases were added for any core config (similar to the access_token_signing_key_update_interval config alias).
  • Issue this PR against the latest non released version branch.
    • To know which one it is, run find the latest released tag (git tag) in the format vX.Y.Z, and then find the
      latest branch (git branch --all) whose X.Y is greater than the latest released tag.
    • If no such branch exists, then create one from the latest released branch.

Remaining TODOs for this PR

  • Check query performance for pagination and user count cause we use group by in it now.
  • Check if both indices - all_auth_recipe_users_primary_user_id_index and all_auth_recipe_users_primary_user_id_and_tenant_id_index are needed
  • Tests for user exists where we return true if primary user exists, but it's associated recipe user doesn't and the primary user is linked to other recipe user.
  • User pagination changes.
  • Remove UserInfo object from plugin interface that inherit from AuthRecipeUserInfo
  • Add an equals function in LoginMethods and AuthRecipeUserInfo in plugin interface
  • remove getRecipeId from AuthRecipeUserInfo class
  • Check if separate index is needed for (app_id, user_id) in all_auth_recipe_users given that there already exists a fk on it, and we use this in the getUsers function.
  • Remove input of recipeId from getUserInfoForRecipeIdFromUserIds
  • We should not longer use recipe level functions like getThirdPartyUserInfoUsingId.
  • Implement hashCode function for AuthRecipeUserInfo and LoginMethods
  • When creating user object in queries implementations, we pass false to isPrimary everywhere. Need to actually fetch the value from the db. You can go to UserInfo obj in all the classes and then see where the constructor is being used.
  • Optimise the fetching of userInfo object - instead of doing three queries, to fetch isPrimary, email verification and login info, try doing it in one.
  • Make tenantIds in loginMethods and AuthRecipeUserInfo into a set?
  • Make CDI 4.0 latest in SemVer
  • Implement toJson in AuthRecipeUserInfo
  • Check how can we reduce number of queries done by GeneralQueries.getPrimaryUserInfoForUserId
  • In listPrimaryUsersByEmail, in GeneralQueries.java, we need to implement all auth recipes
  • In listPrimaryUsersByEmail, in GeneralQueries.java, we need a more efficient way to get users based on emails directly - too many queries.
  • check that we no longer need to use fillUserInfoWithIsPrimaryUserBoolean_transaction in the queries files anymore.
  • Optimise query for listing users by email / phone number to fetch primary_or_recipe_id instead of just user_id in the recipe level queries.
  • Active users should take into account linked users and only record the primary user
  • Account linking APIs:
    • getUser -> id -> user object
    • listUsersByAccountInfo
    • deleteUser
    • unlinkAccount
    • linkAccounts
    • canLinkAccounts
    • createPrimaryUser
    • canCreatePrimaryUser
    • getUsers
  • See query plan for queries in getPrimaryUserIdUsingEmail for tp, ep and pless recipe
  • When linking 2 accounts, (A, B), we need to check if they can be linked across ALL tenants in tenantsOf(A) union tenantsOf(B) belong to.
  • When sharing users across tenants, we need to make sure that the state of account linking is not ruined in that tenant.
  • Remove use of getPrimaryUserInfoForUserIds in generalqueries that does not take Connection con as an arg.
  • Test that create new primary user with same email causes deadlock which is eventually auto resolved.
  • Do we need all_auth_recipe_users_pagination_index and all_auth_recipe_users_primary_user_id_and_tenant_id_index (the second one is the same, except that it's missing the time_joined_column)
  • The index column order in all_auth_recipe_users_pagination_index has the app_id and tenant_id at the end as opposed to at the start. Does this matter?
  • For user count function, try out window functions:
    SELECT max(unique_user_rows) from (SELECT ROW_NUMBER() OVER(PARTITION BY app_id, tenant_id, 
    primary_or_recipe_user_id ORDER BY primary_or_recipe_user_id) as unique_user_rows
    FROM all_auth_recipe_users where app_id = '...' AND tenant_id = '...')
    
  • Instead of querying auth recipe tables one by one to fetch info from them (by email, for example), try and use the UNION query across them all to combine the results (in the select part of these, try and use the same number of colums across the tables):
    (select user_id, email from emailpassword_users where email = '...') union (select user_id, email from passwordless_users 
    where email = '...') union (select user_id, email from thirdparty_users where email = '...')
    
    But if there are different number of columns, see this: https://stackoverflow.com/questions/15867152/mysql-union-different-number-of-columns
  • Instead of doing so many FOR UPDATES, maybe checkout advisory lock (https://www.postgresql.org/docs/current/explicit-locking.html#ADVISORY-LOCKS) for psql, and get_lock in mysql
  • Investigate slow down in test when running with psql - it takes 4-5 seconds per test at a minimum.
  • Delete branch in node repo -> account-linking-core-tests-tracking
  • Call to revokeAllSessionsForUser in linkAccounts and unlinkAccounts should pass in false to revoke sessions for only the recipe user id
  • feat: account linking #742 (comment)
  • feat: account linking #742 (comment)
  • Modification to sharing users across tenants to take into account account linking rules
  • Make sure that no production code is calling testOnly code
  • check that older versions of the sign in etc APIs return the right user object
  • Need to add user id mapping for all loginMethods in the user object as well, and not just the primary user.
  • How does user id mapping behave when it's mapped to a primary user was deleted (but still has linked recipe users), and then you try and map another recipe user id to the same primary user ID, or the same external user ID.
  • make db migratino script specific to psql and mysql
  • confirm db migration script is correct for psql and mysql
  • Remove all uses of recipe.getUserId internally, for example, we still use Passwordless.getUserId in updateUser in passwordless recipe.
  • Check for use of deprecated method in non deprecated code, and check for use of testOnly function in production code.
  • Add fkey constrain on primary_or_recipe_user_id to appid to user id table
  • Check that deleting a primary user ID (not unlinking it) while it's linked to other users, should NOT remove it from app id to user id table. Also, this means that in the doesUserIdExist query in generalqueries, we do not need to do a UNION. - UNION is not required as we also have a fkey constraint, appidtouserid table is a single source for truth.
  • Optimise the getPrimaryUserInfoForUserIds function to not query all auth recipes all the time, and instead, do it based on the recipe ids of the fetched user.
  • Test many signIn using 500 threads in parallel and see the time difference between before accountlinking and after.
  • When linking accounts, remove active user for user that was linked and add active user with primaryUserId
  • Test that primary_or_recipe_user_time_joined is updated correctly during account linking and unlinking
  • Change in chagelog about primary_or_recipe_user_time_joined, index related to that and the migration script
  • paid feature stats
  • test user search with account linking
  • Update CDI about returning recipeUserId in sign in/up related APIs and user object in link accounts API
  • uncomment assert on isSetExternalUserIdCalled in LoginMethod class
  • Update thirdparty sign in up API in CDI to accept isVerified
  • see where all listUser**_Transaction are being called and try to minimize the code paths to those functions.
  • paid feature stats -> send boolean true if even one of the users is a primary user + send number of active users that have account linking in them + number of total users that have account linking in them
  • in the test for testNetworkCallIsMadeInCoreInit, does it test with all the features enabled? For examlpe, if multi tenanct was not enabled, then the bug would not have been caught.. but what is stopping this bug to come again with other features like account linking or totp or anything else? you have only added the license key feature for multi tenancy here.. please change the test to account for all EE features and also such that if we add new EE features, they should be accounted for automatically, or the test should fail if the it’s not modified.
  • the get user api for EP returns non EP users as well

sattvikc and others added 20 commits September 11, 2023 18:27
* fix: allow user disassociation from all tenants

* fix: multitenancy

* fix: updated changelog
fix: rename userId to recipeUserId
* fix: account function changes

* fix: function updates
* fix: tests

* fix: tests
* fix: add exp and iat to JWT payloads without scientific notation (#765)

* adding dev-v6.0.9 tag to this commit to ensure building

* fix: fix handling of b64 and b64url encoded access tokens (#767)

* adding dev-v6.0.10 tag to this commit to ensure building

* Update release.md

* Update release.md

* fix: ee featureflag cron job (#778)

* fix: ee featureflag cron job

* fix: test

* fix: tests

* fix: tests

* adding dev-v6.0.11 tag to this commit to ensure building

* fix: test (#779)

* adding dev-v6.0.11 tag to this commit to ensure building

* fix: test (#780)

* fix: test

* fix: test

* adding dev-v6.0.11 tag to this commit to ensure building

* fix: test (#781)

* adding dev-v6.0.11 tag to this commit to ensure building

* Update README.md (#783)

Corrected all the grammatical errors in the README file.

* fix: session concurrency issue (#785)

* adding dev-v6.0.12 tag to this commit to ensure building

* fix: fixing ee folder issue when empty database at startup (#786)

* fix: fixing ee folder issue when empty database at startup

* fix: changelog

* adding dev-v6.0.12 tag to this commit to ensure building

* fix: test (#787)

* fix: fixing ee folder issue when empty database at startup

* fix: changelog

* fix: test

* adding dev-v6.0.12 tag to this commit to ensure building

* bug fixes

* adding dev-v6.0.12 tag to this commit to ensure building

* Update README.md

* Update README.md

* fix: stats fix (#816)

* fix: stats fix

* fix: pr comments

* fix: disable for in mem

* fix: pr comments

* adding dev-v6.0.13 tag to this commit to ensure building

---------

Co-authored-by: Mihály Lengyel <mihaly@lengyel.tech>
Co-authored-by: rishabhpoddar <rishabh.poddar@gmail.com>
Co-authored-by: Abhisar Yadav <112550486+abhisar-yadav@users.noreply.github.com>
@sattvikc sattvikc self-assigned this Sep 19, 2023
@sattvikc sattvikc changed the title account linking feat: account linking Sep 19, 2023
@sattvikc sattvikc marked this pull request as ready for review September 19, 2023 11:06
@rishabhpoddar rishabhpoddar changed the base branch from 6.0 to 7.0 September 20, 2023 06:27
@rishabhpoddar rishabhpoddar merged commit 7d29ee6 into 7.0 Sep 20, 2023
11 of 21 checks passed
@rishabhpoddar rishabhpoddar deleted the feat/account-linking branch September 20, 2023 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants