feat(sql): expose user identity as Snowflake session variable for row-level security#108
Closed
robertlacok wants to merge 1 commit into
Closed
feat(sql): expose user identity as Snowflake session variable for row-level security#108robertlacok wants to merge 1 commit into
robertlacok wants to merge 1 commit into
Conversation
…-level security
Parse the user email already carried in the audit comment appended to each SQL
query and set it as a Snowflake session variable (DEEPNOTE_USER_EMAIL) on the
same connection before the query runs. Row access policies or secure views can
then filter rows via GETVARIABLE('DEEPNOTE_USER_EMAIL').
The variable is set on every Snowflake query (session variables persist on
pooled connections, so a stale value could otherwise leak another user's rows);
an empty value denies all rows by default for anonymous viewers. The value is
restricted to a safe character set before being embedded in the SET statement.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughSnowflake row-level security support is added to Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
📦 Python package built successfully!
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deepnote_toolkit/sql/sql_execution.py`:
- Around line 629-643: The function _extract_rls_user_email lacks explicit type
annotations for its parameter and return value. Add a type hint for the query
parameter to indicate it accepts an optional string (using Optional[str] or str
| None based on the `query or ""` usage pattern), and add a return type hint of
str since the function always returns a string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59e36f91-a04e-4a2d-9ad9-b8e854c7431e
📒 Files selected for processing (1)
deepnote_toolkit/sql/sql_execution.py
Comment on lines
+629
to
+643
| def _extract_rls_user_email(query): | ||
| """Extract the Deepnote user email from the audit comment embedded in *query*. | ||
|
|
||
| Deepnote appends an audit comment such as ``/* {"user_email":"a@b.com",...} */`` to | ||
| every SQL block. We reuse that identity for row-level security. Returns "" when no | ||
| email is present (e.g. anonymous app viewers) so that the policy denies all rows. | ||
|
|
||
| The value is restricted to a safe character set so it can be embedded directly into a | ||
| ``SET`` statement without risk of SQL injection, regardless of the caller. | ||
| """ | ||
| match = re.search(r'"user_email"\s*:\s*"([^"]+)"', query or "") | ||
| if not match: | ||
| return "" | ||
| return re.sub(r"[^A-Za-z0-9.+@_-]", "", match.group(1)) | ||
|
|
Contributor
There was a problem hiding this comment.
Add type hints.
Function lacks explicit type annotations for parameter and return value.
-def _extract_rls_user_email(query):
+def _extract_rls_user_email(query: str) -> str:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _extract_rls_user_email(query): | |
| """Extract the Deepnote user email from the audit comment embedded in *query*. | |
| Deepnote appends an audit comment such as ``/* {"user_email":"a@b.com",...} */`` to | |
| every SQL block. We reuse that identity for row-level security. Returns "" when no | |
| email is present (e.g. anonymous app viewers) so that the policy denies all rows. | |
| The value is restricted to a safe character set so it can be embedded directly into a | |
| ``SET`` statement without risk of SQL injection, regardless of the caller. | |
| """ | |
| match = re.search(r'"user_email"\s*:\s*"([^"]+)"', query or "") | |
| if not match: | |
| return "" | |
| return re.sub(r"[^A-Za-z0-9.+@_-]", "", match.group(1)) | |
| def _extract_rls_user_email(query: str) -> str: | |
| """Extract the Deepnote user email from the audit comment embedded in *query*. | |
| Deepnote appends an audit comment such as ``/* {"user_email":"a@b.com",...} */`` to | |
| every SQL block. We reuse that identity for row-level security. Returns "" when no | |
| email is present (e.g. anonymous app viewers) so that the policy denies all rows. | |
| The value is restricted to a safe character set so it can be embedded directly into a | |
| ``SET`` statement without risk of SQL injection, regardless of the caller. | |
| """ | |
| match = re.search(r'"user_email"\s*:\s*"([^"]+)"', query or "") | |
| if not match: | |
| return "" | |
| return re.sub(r"[^A-Za-z0-9.+@_-]", "", match.group(1)) |
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 629-629: Missing return type annotation for private function _extract_rls_user_email
(ANN202)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deepnote_toolkit/sql/sql_execution.py` around lines 629 - 643, The function
_extract_rls_user_email lacks explicit type annotations for its parameter and
return value. Add a type hint for the query parameter to indicate it accepts an
optional string (using Optional[str] or str | None based on the `query or ""`
usage pattern), and add a return type hint of str since the function always
returns a string.
Sources: Coding guidelines, Linters/SAST tools
|
🚀 Review App Deployment Started
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Sets a Snowflake session variable carrying the executing user's identity before each SQL query runs, so Snowflake-side row-level security (row access policies or secure views) can filter rows per user.
Deepnote already appends an audit comment containing the user's email to every SQL block. This change:
user_emailout of that comment (_extract_rls_user_email).SET DEEPNOTE_USER_EMAIL = '<email>'on the same connection inside the existingengine.begin()block, immediately before the query.A policy or secure view can then read it via
GETVARIABLE('DEEPNOTE_USER_EMAIL'):Why these choices
SETcould leak a previous user's identity to the next query on that connection.SETstatement.Testing
_extract_rls_user_email(normal, missing-email, and adversarial inputs — unsafe characters are stripped).Notes
Summary by CodeRabbit