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.

Sign up for GitHub

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

Jump to bottom

fix: session creation - checking tenant for user #1063

Open
wants to merge 4 commits into
base: 9.3
Choose a base branch
from
Open

fix: session creation - checking tenant for user #1063

wants to merge 4 commits into from

Conversation

tamassoltesz
Copy link
Contributor

Summary of change

When creating a session for a userId which is known by ST, check if the user is part of that tenant.

Related issues

  • Link to issue1 here
  • Link to issue1 here

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.
  • If added a foreign key constraint on app_id_to_user_id table, make sure to delete from this table when deleting
    the user as well if deleteUserIdMappingToo is false.

@tamassoltesz tamassoltesz changed the base branch from 9.2 to 9.3 November 4, 2024 07:58
sattvikc
sattvikc requested changes Nov 5, 2024
View reviewed changes
build.gradle Outdated
@@ -19,7 +19,7 @@ compileTestJava { options.encoding = "UTF-8" }
// }
//}

version = "9.2.3"
version = "9.2.4"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be 9.3.1 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

coreDriverInterfaceSupported.json Outdated
@@ -20,6 +20,7 @@
"3.1",
"4.0",
"5.0",
"5.1"
"5.1",
"5.2"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs updating ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

CHANGELOG.md Outdated
Comment on lines 10 to 11
- Adds support for CDI 5.2
- In CDI 5.2, when creating a new session for a known user, checks if the user is a member of that tenant.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be 5.3 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

src/main/java/io/supertokens/session/Session.java Outdated
@@ -53,6 +54,7 @@
import io.supertokens.storageLayer.StorageLayer;
import io.supertokens.useridmapping.UserIdMapping;
import io.supertokens.useridmapping.UserIdType;
import io.supertokens.utils.SemVer;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we avoid using SemVer in this layer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

src/main/java/io/supertokens/session/Session.java Outdated
@@ -132,11 +135,11 @@ public static SessionInformationHolder createNewSession(TenantIdentifier tenantI
@Nonnull JsonObject userDataInJWT,
@Nonnull JsonObject userDataInDatabase,
boolean enableAntiCsrf, AccessToken.VERSION version,
boolean useStaticKey)
boolean useStaticKey, SemVer semVer)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of passing semVer here, pass a boolean that indicates whether to check the user tenant or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

src/main/java/io/supertokens/session/Session.java Outdated
@@ -166,6 +170,16 @@ public static SessionInformationHolder createNewSession(TenantIdentifier tenantI
if (userIdMappings.containsKey(recipeUserId)) {
recipeUserId = userIdMappings.get(recipeUserId);
}

if(semVer!= null && semVer.greaterThanOrEqualTo(SemVer.v5_2)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simply use a boolean whether to do this check or not

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

CHANGELOG.md Outdated

- Adds support for CDI 5.2
- In CDI 5.2, when creating a new session for a known user, checks if the user is a member of that tenant.
If not, returns UNAUTHORISED.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think UNAUTHORISED is the right thing to return here. You may want to add a different status like USER_DOES_NOT_BELONG_TO_TENANT_ERROR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

sattvikc
sattvikc requested changes Nov 5, 2024
View reviewed changes
CHANGELOG.md Outdated
@@ -149,6 +156,7 @@ CREATE TABLE IF NOT EXISTS oauth_logout_challenges (

CREATE INDEX oauth_logout_challenges_time_created_index ON oauth_logout_challenges(time_created ASC, app_id ASC);
```
>>>>>>> origin/master
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is. Sorry I missed this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

src/main/java/io/supertokens/webserver/api/session/SessionAPI.java Outdated
@@ -143,6 +144,11 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws I
super.sendJsonResponse(200, result, resp);
} catch (AccessTokenPayloadError e) {
throw new ServletException(new BadRequestException(e.getMessage()));
} catch (UnauthorisedException e) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

catching Unauthorised and returning a different status could get confusing. Create a new exception type for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, okay

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sattvikc sattvikc sattvikc requested changes

@rishabhpoddar rishabhpoddar Awaiting requested review from rishabhpoddar

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tamassoltesz @sattvikc