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

don't use session.user object in _getAuthenticatorAssuranceLevel() #909

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kizivat
Copy link

@kizivat kizivat commented May 14, 2024

One avoidable source of the getUser() warnings

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

Can't get rid of getUser() warning

What is the new behavior?

The warning is no longer logged on the line defining verifiedFactors

Additional context

N/A

@kizivat
Copy link
Author

kizivat commented May 14, 2024

The force push was to adhere to conventional commits.

@sleepdotexe
Copy link

If the solution for this issue is making an additional getUser check inside the method – which I'm assuming should make the method trustable on the server(?) – should this call be made at the beginning of the getAuthenticatorAssuranceLevel() method (ie. before we do checks on the session)?

Otherwise, we might end up using less secure information from _useSession since this has the possibility to return early before it reaches the _getUser call.

Also to consider: what is the added performance overhead for this function by making an additional user call? Do we need to provide an option for a jwt to be passed as an argument to getAuthenticatorAssuranceLevel() since getUser can optionally take a jwt?

@j4w8n
Copy link
Contributor

j4w8n commented May 15, 2024

You may know this, but making sure: keep in mind this change adds a network call to Supabase.

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.

None yet

3 participants