Skip to content

remove user auth for query in list filters #1294

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

Conversation

nikhilsinhaparseable
Copy link
Contributor

@nikhilsinhaparseable nikhilsinhaparseable commented Apr 10, 2025

current: server validates if user is authorized
for the streams in the filter_query in the filters list

change: server lists all available saved filters

reason: in saved search, filter_query is not a valid sql string it is just a key-value pair
also, unauthorized user cannot view the saved filter as he is restricted in the prism UI
hence, removed the check

Summary by CodeRabbit

  • Refactor
    • Streamlined the process for retrieving and displaying filter options, ensuring a more consistent user experience.
    • Simplified filter data handling by removing redundant session checks, which helps reduce potential delays or errors.

current: server validates if user is authorized
for the streams in the `filter_query` in the filters list

change: server lists all available saved filters

reason: in saved search, `filter_query` is not a valid sql string
it is just a key-value pair
also, unauthorized user cannot view the saved filter as he is restricted
in the prism UI
hence, removed the check
Copy link
Contributor

coderabbitai bot commented Apr 10, 2025

Walkthrough

The pull request removes session key extraction and related authorization checks from the filter retrieval processes. The changes update multiple function signatures to no longer require a session key, simplifying the HTTP handler, home response generation, and core filter service. The modified functions call the filter listing methods directly and omit error handling for session key extraction, leading to a more straightforward control flow.

Changes

File(s) Change Summary
src/handlers/.../filters.rs & src/users/filters.rs Removed the session key parameter from list and list_filters functions; eliminated session key extraction and user authorization checks, simplifying filter retrieval.
src/prism/home/mod.rs Updated get_filter_titles by removing the key parameter and adjusted its call in generate_home_response to reflect the new simplified signature.

Sequence Diagram(s)

HTTP Handler and Filter Service Flow

sequenceDiagram
    participant C as Client
    participant H as HTTP Handler (list)
    participant F as Filters Service (list_filters)
    
    C->>H: Send HTTP request for filters
    H->>F: Call list_filters()
    F-->>H: Return list of filters
    H-->>C: Respond with filters list
Loading

Home Response Flow

sequenceDiagram
    participant U as Home Controller
    participant G as get_filter_titles
    participant F as Filters Service (list_filters)
    
    U->>G: Call get_filter_titles()
    G->>F: Call list_filters()
    F-->>G: Return filter list
    G-->>U: Provide filter titles
    U-->>C: Return home response
Loading

Possibly related PRs

  • Updates for Prism #1224: Aligns with changes that remove session key parameters and simplify the filter retrieval process for both authorization and data fetching.

Suggested reviewers

  • de-sh

Poem

I'm a little rabbit, hopping in the code,
Leaving behind session keys on the road.
The filters now dance in a simpler space,
With no key to chase, we quicken our pace.
Hop along, dear peer, in the garden of commits!
🐇🌸
CodeRabbit leads our joyful hops!


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2793d81 and 90a9437.

📒 Files selected for processing (3)
  • src/handlers/http/users/filters.rs (2 hunks)
  • src/prism/home/mod.rs (2 hunks)
  • src/users/filters.rs (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
  • GitHub Check: Quest Smoke and Load Tests for Standalone deployments
  • GitHub Check: Quest Smoke and Load Tests for Distributed deployments
  • GitHub Check: Build Default x86_64-pc-windows-msvc
  • GitHub Check: Build Default x86_64-apple-darwin
  • GitHub Check: Build Default aarch64-unknown-linux-gnu
  • GitHub Check: Build Default aarch64-apple-darwin
  • GitHub Check: Build Default x86_64-unknown-linux-gnu
  • GitHub Check: Build Kafka aarch64-apple-darwin
  • GitHub Check: Build Kafka x86_64-unknown-linux-gnu
  • GitHub Check: coverage
🔇 Additional comments (5)
src/handlers/http/users/filters.rs (1)

36-38:

❓ Verification inconclusive

Authorization model simplified for filter listing.

The list function has been modified to no longer require a session key from the HTTP request, which removes the user authorization check for filter queries. This aligns with the PR objective to simplify filter access since:

  1. Filter queries are simple key-value pairs, not executable SQL
  2. UI already restricts unauthorized users from viewing saved filters

🏁 Script executed:

#!/bin/bash
# Verify that the UI restricts unauthorized users from viewing saved filters
# by checking for authorization checks in frontend code

rg -A 5 "filter.*auth|auth.*filter" --type js --type jsx --type ts --type tsx

Length of output: 107


Action: Further Verify UI Authorization Checks for Filter Queries

The backend changes (removing the session key requirement in the list function) align with the PR’s objective, given that filter queries are simple key-value pairs and the UI is expected to restrict unauthorized accesses. However, our initial search for authorization checks in the frontend failed due to an issue recognizing JSX file types. To address this, please run the following updated script—which adds custom file type detection for .jsx files—to verify that the frontend indeed restricts access:

#!/bin/bash
# Add custom file type for JSX files and search for authorization logic related to filter queries in frontend code
rg --type-add "jsx:*.jsx" -A 5 "filter.*auth|auth.*filter" --type js --type jsx --type ts --type tsx

Once executed, review the output to ensure that the frontend code includes the necessary authorization logic. This additional manual verification will confirm that removing the backend session key check does not compromise overall security.

src/prism/home/mod.rs (2)

107-108: Function call updated to match new signature.

The call to get_filter_titles has been updated to remove the session key parameter, which is consistent with the changes in the filter authorization model.


261-278:

❓ Verification inconclusive

Authorization check removed from filter title retrieval.

The get_filter_titles function has been modified to no longer require a session key and doesn't perform authorization filtering. This means all saved filters will be displayed in the home response regardless of user permissions.

This change is consistent with the PR objective to simplify the authorization model for filters, but ensure this doesn't create any security issues by exposing sensitive filter information.


🏁 Script executed:

#!/bin/bash
# Check if there are any sensitive data in filter titles or if they're just user-friendly labels

# Look for filter title handling in the code
rg -A 3 "filter_name|filter.*title" --type rust

Length of output: 1642


Action Required: Confirm Filter Data Safety Post-Authorization Removal

The get_filter_titles function in src/prism/home/mod.rs (lines 261–278) has been updated to remove the session key requirement and the associated authorization filtering. As a result, the home response now includes all saved filters regardless of user permissions. This change aligns with the PR’s objective to simplify the authorization model for filter handling.

  • Verification:

    • The shell script output indicates that the filter data (e.g., filter_name and filter_id defined in src/users/filters.rs) are simple, user-friendly labels and identifiers.
    • No additional sensitive details appear to be exposed in the filter titles.
  • Action:
    Please review and confirm that exposing all filter entries is acceptable and that none of the filter fields inadvertently reveal sensitive information.

src/users/filters.rs (2)

26-28: Import for user authorization removed.

The import for authorization-related functionality has been removed, which is consistent with the PR objective to remove user authorization checks for filter queries.


155-157:

❓ Verification inconclusive

Authorization check removed from filter listing.

The core change in this PR - the list_filters method no longer requires a session key and returns all filters without performing authorization filtering. This simplifies the code and removes a layer of authorization that was deemed unnecessary since:

  1. Filter queries contain simple key-value pairs, not executable SQL
  2. UI already restricts unauthorized users from viewing saved filters

This change is intentional as per the PR objective, but it's worth verifying that no other components rely on this authorization check for security.


🏁 Script executed:

#!/bin/bash
# Check for any other components that might rely on filter authorization
# Look for places where list_filters is called with security context

rg "list_filters.*key|key.*list_filters" --type rust

Length of output: 54



Attention: Verify Authorization Removal in Filter Listing

After running additional searches for calls to list_filters, there is no evidence of any direct usage that still relies on a session key or authorization context. The removal of the authorization check in src/users/filters.rs (lines 155-157) appears intentional and aligns with the PR’s objective—since filters consist of simple key-value pairs and the UI already restricts unauthorized users.

However, please manually verify across the codebase to ensure that no indirect dependencies remain on the removed authorization logic.


🏁 Script executed:

#!/bin/bash
# Searching for any direct invocations of list_filters to check if session keys are referenced
rg "list_filters\\s*\\(" --type rust --line-number

Length of output: 265


Authorization Check Removal Verification

The list_filters method in src/users/filters.rs now returns all filters without performing session key-based authorization. Searches show that this method is invoked in both src/prism/home/mod.rs:263 and src/handlers/http/users/filters.rs:37 without any accompanying session key or authorization context. This aligns with the PR objective—filters are simple key-value pairs, and the UI already limits unauthorized access.

Please manually review these invocation points to ensure no hidden security dependencies remain.

✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

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.

1 participant