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

PROD-1214 Redact cli, database, and redis info from CONFIG_KEY_ALLOWLIST #4379

Merged
merged 4 commits into from
Nov 3, 2023

Conversation

daveqnet
Copy link
Contributor

@daveqnet daveqnet commented Nov 3, 2023

Partially closes PROD-1214

Description Of Changes

  • Removes cli, database, and redis configuration information from the allowable values returned by a GET api/v1/config API request.

Code Changes

  • See commit (very simple changes)

Steps to Confirm

  • Perform an authorized GET api/v1/config request and ensure that the response contains no cli, database, or redis data.

Pre-Merge Checklist

Copy link

cypress bot commented Nov 3, 2023

Passing run #5005 ↗︎

0 4 0 0 Flakiness 0

Details:

Merge 54f61d4 into fe74eca...
Project: fides Commit: 40bd75c820 ℹ️
Status: Passed Duration: 00:32 💡
Started: Nov 3, 2023 7:32 PM Ended: Nov 3, 2023 7:32 PM

Review all test suite changes for PR #4379 ↗︎

@daveqnet
Copy link
Contributor Author

daveqnet commented Nov 3, 2023

Local test results:

~/projects/github/ethyca/fides daveqnet/PRO…EY_ALLOWLIST ❯ curl -i -X 'GET' \
  'http://localhost:8080/api/v1/config' \
  -H 'accept: application/json' \
  -H 'Authorization: Bearer eyJh...'
HTTP/1.1 200 OK
date: Fri, 03 Nov 2023 15:49:52 GMT
server: uvicorn
content-length: 571
content-type: application/json
x-ratelimit-limit: 1000
x-ratelimit-remaining: 998
x-ratelimit-reset: 1699026629
retry-after: Fri, 03 Nov 2023 15:50:29 -0000
{
    "user":
    {
        "analytics_opt_out": false
    },
    "logging":
    {
        "level": "DEBUG"
    },
    "notifications":
    {
        "send_request_completion_notification": false,
        "send_request_receipt_notification": false,
        "send_request_review_notification": false,
        "notification_service_type": null
    },
    "security":
    {
        "cors_origins":
        [
            "http://localhost:8080",
            "http://localhost:3001",
            "http://localhost:3000"
        ],
        "encoding": "UTF-8",
        "oauth_access_token_expire_minutes": 11520
    },
    "execution":
    {
        "task_retry_count": 0,
        "task_retry_delay": 1,
        "task_retry_backoff": 1,
        "require_manual_request_approval": true,
        "subject_identity_verification_required": false
    }
}

@adamsachs
Copy link
Contributor

thank you for being proactive and getting this change in @daveqnet! it looks good to me, just needs a quick update to the failing test which i'll get up right now.

i don't think we're relying on these values being returned via the API in any way, but i'll confirm quickly with the team that's the case just to not pull the rug out from under anyone without notice 👍

Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

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

ok - updated test and confirmed it's passing locally, so i'm going to approve this then!

let's wait for CI to run green and it'll also give some time for folks to chime in with any objections. assuming that all goes well, then let's merge this in a bit! if any strong objections come in after the merge, we should always be able to revert before an official release anyway 👍

Copy link

codecov bot commented Nov 3, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (775e582) 87.72% compared to head (c0168ac) 87.04%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4379      +/-   ##
==========================================
- Coverage   87.72%   87.04%   -0.69%     
==========================================
  Files         335      328       -7     
  Lines       21161    20266     -895     
  Branches     2753     2608     -145     
==========================================
- Hits        18564    17641     -923     
- Misses       2127     2162      +35     
+ Partials      470      463       -7     
Files Coverage Δ
src/fides/config/utils.py 100.00% <ø> (ø)

... and 17 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adamsachs adamsachs merged commit 2d0d7e4 into main Nov 3, 2023
17 checks passed
@adamsachs adamsachs deleted the daveqnet/PROD-1214/redact-CONFIG_KEY_ALLOWLIST branch November 3, 2023 19: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