NotificationsYou must be signed in to change notification settings
Fork631
Star14.7k

fix: session creation - checking tenant for user#1063

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Open

tamassoltesz wants to merge4 commits into9.3

base:9.3

Choose a base branch

fromfix/check_tenant_when_creating_session

Open

fix: session creation - checking tenant for user#1063

tamassoltesz wants to merge4 commits into9.3fromfix/check_tenant_when_creating_session

Conversation

Copy link

Contributor

tamassoltesz commentedOct 18, 2024

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 ourdocs 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
- Inbuild.gradle
If added a new paid feature, edit thegetPaidFeatureStats function in FeatureFlag.java file
Had installed and ran the pre-commit hook
If there are new dependencies that have been added inbuild.gradle, please make sure to add them
inimplementationDependencies.json.
Update functiongetValidFields inio/supertokens/config/CoreConfig.java if new aliases were added for any core
config (similar to theaccess_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 formatvX.Y.Z, and then find the
  latest branch (git branch --all) whoseX.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 onapp_id_to_user_id table, make sure to delete from this table when deleting
the user as well ifdeleteUserIdMappingToo is false.

fix: session creation - checking tenant for user

880f22f

tamassoltesz requested review fromrishabhpoddar andsattvikc

October 18, 2024 23:37

tamassoltesz changed the base branch from9.2 to9.3

November 4, 2024 07:58

sattvikc requested changes

Nov 5, 2024

View reviewed changes

build.gradle Outdated

		//}

		version="9.2.3"
		version="9.2.4"

Copy link

Collaborator

sattvikcNov 5, 2024

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

ContributorAuthor

tamassolteszNov 5, 2024

Choose a reason for hiding this comment

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

done

coreDriverInterfaceSupported.json Outdated

		"5.0",
		"5.1"
		"5.1",
		"5.2"

Copy link

Collaborator

sattvikcNov 5, 2024

Choose a reason for hiding this comment

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

Needs updating ?

Copy link

ContributorAuthor

tamassolteszNov 5, 2024

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

sattvikcNov 5, 2024

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

ContributorAuthor

tamassolteszNov 5, 2024

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

		importio.supertokens.storageLayer.StorageLayer;
		importio.supertokens.useridmapping.UserIdMapping;
		importio.supertokens.useridmapping.UserIdType;
		importio.supertokens.utils.SemVer;

Copy link

Collaborator

sattvikcNov 5, 2024

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

ContributorAuthor

tamassolteszNov 5, 2024

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

		@NonnullJsonObjectuserDataInDatabase,
		booleanenableAntiCsrf,AccessToken.VERSIONversion,
		booleanuseStaticKey)
		booleanuseStaticKey,SemVersemVer)

Copy link

Collaborator

sattvikcNov 5, 2024

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

ContributorAuthor

tamassolteszNov 5, 2024

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

		recipeUserId =userIdMappings.get(recipeUserId);
		}

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

Copy link

Collaborator

sattvikcNov 5, 2024

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

ContributorAuthor

tamassolteszNov 5, 2024

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

sattvikcNov 5, 2024

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

ContributorAuthor

tamassolteszNov 5, 2024

Choose a reason for hiding this comment

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

done

chore: merging master to feature branch

6c87f28

sattvikc requested changes

Nov 5, 2024

View reviewed changes

CHANGELOG.md Outdated


		CREATEINDEXoauth_logout_challenges_time_created_indexON oauth_logout_challenges(time_createdASC, app_idASC);
		```
		>>>>>>>origin/master

Copy link

Collaborator

sattvikcNov 5, 2024

Choose a reason for hiding this comment

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

merge error?

Copy link

ContributorAuthor

tamassolteszNov 5, 2024

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

ContributorAuthor

tamassolteszNov 5, 2024

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

		super.sendJsonResponse(200,result,resp);
		}catch (AccessTokenPayloadErrore) {
		thrownewServletException(newBadRequestException(e.getMessage()));
		}catch (UnauthorisedExceptione) {

Copy link

Collaborator

sattvikcNov 5, 2024

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

ContributorAuthor

tamassolteszNov 5, 2024

Choose a reason for hiding this comment

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

sure, okay

Copy link

ContributorAuthor

tamassolteszNov 5, 2024

Choose a reason for hiding this comment

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

done

tamassoltesz added2 commits

November 5, 2024 15:42

fix: review fixes

d2f7e53

chore: changing versions in build file and in changelog

ed671b4

BoomchainLabs approved these changes

Jun 26, 2025

View reviewed changes

Labels

None yet

Movatterモバイル変換

fix: session creation - checking tenant for user#1063

Are you sure you want to change the base?

fix: session creation - checking tenant for user#1063

Uh oh!

Conversation

tamassoltesz commentedOct 18, 2024

Summary of change

Related issues

Test Plan

Documentation changes

Checklist for important updates

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants