-
Notifications
You must be signed in to change notification settings - Fork 537
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
fix: tests #776
fix: tests #776
Changes from 10 commits
5a01102
d62c2fc
c11e096
09c2247
98fa30c
49c0360
db0679f
4371515
a51130c
c340a77
d9410ec
5134184
ec286b2
74a9ab1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -396,7 +396,7 @@ public static boolean addUserIdToTenant(Main main, TenantIdentifierWithStorage t | |
String tenantId = tenantIdentifierWithStorage.getTenantId(); | ||
AuthRecipeUserInfo userToAssociate = storage.getPrimaryUserById_Transaction(tenantIdentifierWithStorage.toAppIdentifier(), con, userId); | ||
|
||
if (userToAssociate.isPrimaryUser) { | ||
if (userToAssociate != null && userToAssociate.isPrimaryUser) { | ||
Set<String> emails = new HashSet<>(); | ||
Set<String> phoneNumbers = new HashSet<>(); | ||
Set<LoginMethod.ThirdParty> thirdParties = new HashSet<>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the checks below for same email etc for account linking should check:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the other places where this type of check also happens consider the login method being different? |
||
|
@@ -442,6 +442,9 @@ public static boolean addUserIdToTenant(Main main, TenantIdentifierWithStorage t | |
} | ||
} | ||
|
||
// userToAssociate may be null if the user is not associated to any tenants, we can still try and | ||
// associate it. This happens only in CDI 3.0 where we allow disassociation from all tenants | ||
// This will not happen in CDI >= 4.0 because we will not allow disassociation from all tenants | ||
try { | ||
boolean result = ((MultitenancySQLStorage) storage).addUserIdToTenant_Transaction(tenantIdentifierWithStorage, con, userId); | ||
storage.commitTransaction(con); | ||
|
@@ -467,15 +470,40 @@ public static boolean addUserIdToTenant(Main main, TenantIdentifierWithStorage t | |
} | ||
} | ||
|
||
@TestOnly | ||
public static boolean removeUserIdFromTenant(Main main, TenantIdentifierWithStorage tenantIdentifierWithStorage, | ||
String userId, String externalUserId) | ||
throws FeatureNotEnabledException, TenantOrAppNotFoundException, StorageQueryException, | ||
UnknownUserIdException { | ||
try { | ||
return removeUserIdFromTenant(main, tenantIdentifierWithStorage, userId, externalUserId, false); | ||
} catch (DisassociationNotAllowedException e) { | ||
throw new IllegalStateException("should never happen"); | ||
} | ||
} | ||
|
||
public static boolean removeUserIdFromTenant(Main main, TenantIdentifierWithStorage tenantIdentifierWithStorage, | ||
String userId, String externalUserId, boolean disallowLastTenantDisassociation) | ||
throws FeatureNotEnabledException, TenantOrAppNotFoundException, StorageQueryException, | ||
UnknownUserIdException, DisassociationNotAllowedException { | ||
if (Arrays.stream(FeatureFlag.getInstance(main, new AppIdentifier(null, null)).getEnabledFeatures()) | ||
.noneMatch(ee_features -> ee_features == EE_FEATURES.MULTI_TENANCY)) { | ||
throw new FeatureNotEnabledException(EE_FEATURES.MULTI_TENANCY); | ||
} | ||
|
||
if (disallowLastTenantDisassociation) { | ||
AuthRecipeUserInfo userInfo = AuthRecipe.getUserById(tenantIdentifierWithStorage.toAppIdentifierWithStorage(), userId); | ||
if (userInfo != null) { | ||
for (LoginMethod lM : userInfo.loginMethods) { | ||
if (lM.getSupertokensUserId().equals(userId)) { | ||
if (lM.tenantIds.size() == 1 && lM.tenantIds.contains(tenantIdentifierWithStorage.getTenantId())) { | ||
throw new DisassociationNotAllowedException(); | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
boolean finalDidExist = false; | ||
boolean didExist = AuthRecipe.deleteNonAuthRecipeUser(tenantIdentifierWithStorage, | ||
externalUserId == null ? userId : externalUserId); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
/* | ||
* Copyright (c) 2023, VRAI Labs and/or its affiliates. All rights reserved. | ||
* | ||
* This software is licensed under the Apache License, Version 2.0 (the | ||
* "License") as published by the Apache Software Foundation. | ||
* | ||
* You may not use this file except in compliance with the License. You may | ||
* obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT | ||
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the | ||
* License for the specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
package io.supertokens.multitenancy.exception; | ||
|
||
public class DisassociationNotAllowedException extends Exception { | ||
public DisassociationNotAllowedException() { | ||
super("Disassociation not allowed"); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -216,12 +216,11 @@ private static SignInUpResponse signInUpHelper(TenantIdentifierWithStorage tenan | |
AuthRecipeSQLStorage authRecipeStorage = | ||
(AuthRecipeSQLStorage) tenantIdentifierWithStorage.getAuthRecipeStorage(); | ||
|
||
storage.startTransaction(con -> { | ||
{ // Try without transaction | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add more comment - why are we doing this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
AuthRecipeUserInfo userFromDb = null; | ||
|
||
AuthRecipeUserInfo[] usersFromDb = authRecipeStorage.listPrimaryUsersByThirdPartyInfo_Transaction( | ||
AuthRecipeUserInfo[] usersFromDb = authRecipeStorage.listPrimaryUsersByThirdPartyInfo( | ||
appIdentifier, | ||
con, | ||
thirdPartyId, thirdPartyUserId); | ||
for (AuthRecipeUserInfo user : usersFromDb) { | ||
if (user.tenantIds.contains(tenantIdentifierWithStorage.getTenantId())) { | ||
|
@@ -231,10 +230,8 @@ private static SignInUpResponse signInUpHelper(TenantIdentifierWithStorage tenan | |
userFromDb = user; | ||
} | ||
} | ||
|
||
if (userFromDb == null) { | ||
storage.commitTransaction(con); | ||
return null; | ||
continue; // try to create the user again | ||
} | ||
|
||
LoginMethod lM = null; | ||
|
@@ -250,35 +247,75 @@ private static SignInUpResponse signInUpHelper(TenantIdentifierWithStorage tenan | |
throw new IllegalStateException("Should never come here"); | ||
} | ||
|
||
|
||
if (!email.equals(lM.email)) { | ||
// before updating the email, we must check for if another primary user has the same | ||
// email, and if they do, then we do not allow the update. | ||
if (userFromDb.isPrimaryUser) { | ||
for (String tenantId : userFromDb.tenantIds) { | ||
AuthRecipeUserInfo[] userBasedOnEmail = | ||
authRecipeStorage.listPrimaryUsersByEmail_Transaction( | ||
appIdentifier, con, email | ||
); | ||
for (AuthRecipeUserInfo userWithSameEmail : userBasedOnEmail) { | ||
if (!userWithSameEmail.tenantIds.contains(tenantId)) { | ||
continue; | ||
if (email.equals(lM.email)) { | ||
return new SignInUpResponse(false, userFromDb); | ||
} else { | ||
// Email needs updating, so repeat everything in a transaction | ||
|
||
storage.startTransaction(con -> { | ||
AuthRecipeUserInfo userFromDb1 = null; | ||
|
||
AuthRecipeUserInfo[] usersFromDb1 = authRecipeStorage.listPrimaryUsersByThirdPartyInfo_Transaction( | ||
appIdentifier, | ||
con, | ||
thirdPartyId, thirdPartyUserId); | ||
for (AuthRecipeUserInfo user : usersFromDb1) { | ||
if (user.tenantIds.contains(tenantIdentifierWithStorage.getTenantId())) { | ||
if (userFromDb1 != null) { | ||
throw new IllegalStateException("Should never happen"); | ||
} | ||
if (userWithSameEmail.isPrimaryUser && | ||
!userWithSameEmail.getSupertokensUserId().equals(userFromDb.getSupertokensUserId())) { | ||
throw new StorageTransactionLogicException( | ||
new EmailChangeNotAllowedException()); | ||
userFromDb1 = user; | ||
} | ||
} | ||
|
||
if (userFromDb1 == null) { | ||
storage.commitTransaction(con); | ||
return null; | ||
} | ||
|
||
LoginMethod lM1 = null; | ||
for (LoginMethod loginMethod : userFromDb1.loginMethods) { | ||
if (loginMethod.thirdParty != null && loginMethod.thirdParty.id.equals(thirdPartyId) && | ||
loginMethod.thirdParty.userId.equals(thirdPartyUserId)) { | ||
lM1 = loginMethod; | ||
break; | ||
} | ||
} | ||
|
||
if (lM1 == null) { | ||
throw new IllegalStateException("Should never come here"); | ||
} | ||
|
||
if (!email.equals(lM1.email)) { | ||
// before updating the email, we must check for if another primary user has the same | ||
// email, and if they do, then we do not allow the update. | ||
if (userFromDb1.isPrimaryUser) { | ||
for (String tenantId : userFromDb1.tenantIds) { | ||
AuthRecipeUserInfo[] userBasedOnEmail = | ||
authRecipeStorage.listPrimaryUsersByEmail_Transaction( | ||
appIdentifier, con, email | ||
); | ||
for (AuthRecipeUserInfo userWithSameEmail : userBasedOnEmail) { | ||
if (!userWithSameEmail.tenantIds.contains(tenantId)) { | ||
continue; | ||
} | ||
if (userWithSameEmail.isPrimaryUser && | ||
!userWithSameEmail.getSupertokensUserId().equals(userFromDb1.getSupertokensUserId())) { | ||
throw new StorageTransactionLogicException( | ||
new EmailChangeNotAllowedException()); | ||
} | ||
} | ||
} | ||
} | ||
storage.updateUserEmail_Transaction(appIdentifier, con, | ||
thirdPartyId, thirdPartyUserId, email); | ||
} | ||
} | ||
storage.updateUserEmail_Transaction(appIdentifier, con, | ||
thirdPartyId, thirdPartyUserId, email); | ||
} | ||
|
||
storage.commitTransaction(con); | ||
return null; | ||
}); | ||
storage.commitTransaction(con); | ||
return null; | ||
}); | ||
} | ||
} | ||
|
||
AuthRecipeUserInfo user = getUser(tenantIdentifierWithStorage, thirdPartyId, thirdPartyUserId); | ||
return new SignInUpResponse(false, user); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the try catch below is incorrectly scoped There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,11 +21,13 @@ | |
import io.supertokens.Main; | ||
import io.supertokens.featureflag.exceptions.FeatureNotEnabledException; | ||
import io.supertokens.multitenancy.Multitenancy; | ||
import io.supertokens.multitenancy.exception.DisassociationNotAllowedException; | ||
import io.supertokens.pluginInterface.RECIPE_ID; | ||
import io.supertokens.pluginInterface.emailpassword.exceptions.UnknownUserIdException; | ||
import io.supertokens.pluginInterface.exceptions.StorageQueryException; | ||
import io.supertokens.pluginInterface.multitenancy.exceptions.TenantOrAppNotFoundException; | ||
import io.supertokens.useridmapping.UserIdType; | ||
import io.supertokens.utils.SemVer; | ||
import io.supertokens.webserver.InputParser; | ||
import io.supertokens.webserver.WebserverAPI; | ||
import jakarta.servlet.ServletException; | ||
|
@@ -68,7 +70,8 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I | |
} | ||
|
||
boolean wasAssociated = Multitenancy.removeUserIdFromTenant(main, | ||
getTenantIdentifierWithStorageFromRequest(req), userId, externalUserId); | ||
getTenantIdentifierWithStorageFromRequest(req), userId, externalUserId, getVersionFromRequest(req).greaterThanOrEqualTo( | ||
SemVer.v4_0)); | ||
|
||
JsonObject result = new JsonObject(); | ||
result.addProperty("status", "OK"); | ||
|
@@ -79,7 +82,10 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I | |
JsonObject result = new JsonObject(); | ||
result.addProperty("status", "UNKNOWN_USER_ID_ERROR"); | ||
super.sendJsonResponse(200, result, resp); | ||
|
||
} catch (DisassociationNotAllowedException e) { | ||
JsonObject result = new JsonObject(); | ||
result.addProperty("status", "DISASSOCIATION_NOT_ALLOWED_ERROR"); | ||
super.sendJsonResponse(200, result, resp); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add a reason string, and also have this showed on the backend sdk There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
} catch (StorageQueryException | TenantOrAppNotFoundException | FeatureNotEnabledException e) { | ||
throw new ServletException(e); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead, on line 819, check if primaryUserIdToDeleteNonAuthRecipe == null, and then if it is, call whats on line 854
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done