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

Allow encrypting access tokens in Prisma dbs #819

Closed

Conversation

paulomarg
Copy link
Contributor

WHY are these changes introduced?

With the ability to encrypt access tokens built into the Session class (#818), we should update our storage options to included that for an extra layer of security.

WHAT is this pull request doing?

Adding a new parameter to PrismaSessionStorage to pass in an encryption key to be used to encrypt data. This doesn't require apps to migrate right away since we can update rows as they rotate, but we should still recommend apps load and store every session (or wipe their table).

Type of change

  • Minor: New feature (non-breaking change which adds functionality)

Checklist

  • I have used yarn changeset to create a draft changelog entry (do NOT update the CHANGELOG.md files manually)
  • I have added/updated tests for this change
  • I have documented new APIs/updated the documentation for modified APIs (for public APIs)

@paulomarg paulomarg requested a review from a team as a code owner April 24, 2024 18:48
@paulomarg paulomarg force-pushed the encrypt_access_tokens branch 2 times, most recently from a425cfe to ebf73ed Compare April 25, 2024 19:25
@paulomarg paulomarg force-pushed the encrypt_prisma_session_access_tokens branch from fdcc677 to 1450a59 Compare April 25, 2024 19:39
@paulomarg paulomarg force-pushed the encrypt_access_tokens branch from ebf73ed to 5689d31 Compare April 25, 2024 20:23
@paulomarg paulomarg force-pushed the encrypt_prisma_session_access_tokens branch from 1450a59 to 907f5e9 Compare April 25, 2024 20:24
scope: sessionParams.scope || null,
expires: sessionParams.expires ? new Date(sessionParams.expires) : null,
accessToken: sessionParams.accessToken || '',
userId: (sessionParams.onlineAccessInfo as unknown as bigint) || null,
Copy link
Contributor

Choose a reason for hiding this comment

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

We still want just the ID here?

sessionParams.onlineAccessInfo?.associated_user.id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I need to rebase this onto your PR that returns the user info?

Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't. The current functionality should have the ID in the sessionParams.onlineAccessInfo?.associated_user.id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That changed slightly because we're using the property array here instead of toObject.

Copy link
Contributor

Choose a reason for hiding this comment

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

aaah I missed that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You shouldn't

I think we should still be setting all of the user fields here though, right? That's why I was thinking about rebasing.

@paulomarg
Copy link
Contributor Author

In the end, we decided not to go forward with this as developers are probably better served by using encryption at the infrastructure level for their production databases, rather than making the API more complex at the software level.

@paulomarg paulomarg closed this Apr 29, 2024
@paulomarg paulomarg deleted the encrypt_prisma_session_access_tokens branch April 29, 2024 17:32
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.

2 participants