559-endpoint-log-events - Adds middleware for calling analytics events for each endpoint #622
Conversation
… for each endpoint
eastandwestwind
changed the title
| @@ -15,6 +15,7 @@ module.exports = { | |||
| // causes bug in re-exporting default exports, see | |||
| // https://github.com/eslint/eslint/issues/15617 | |||
| 'no-restricted-exports': [0], | |||
| 'import/prefer-default-export': 'off', | |||
There was a problem hiding this comment.
allows us to directly export function here- https://github.com/ethyca/fidesops/pull/622/files#diff-adfcb02b29653b385133da876800f29f50b8c5bc39875695ebdf2324c63525f7R4
We also have other instances throughout admin-ui where we don't use default exports, so I'm unsure why this wasn't caught before.
src/fidesops/main.py
Outdated
| async def dispatch_log_request(request: Request, call_next: Callable) -> Response: | ||
| fides_source: Optional[str] = request.headers.get("X-Fides-Source") | ||
| now: datetime = datetime.now(tz=timezone.utc) | ||
| endpoint = f"{request.method}: {request.url.path}" |
There was a problem hiding this comment.
right now HTTP method gets filtered out by a validator in fideslog, but this will be fixed in ethyca/fideslog#66
src/fidesops/main.py
Outdated
| docker=in_docker_container(), | ||
| event=Event.endpoint_call.value, | ||
| event_created_at=event_created_at, | ||
| local_host=running_on_local_host(), |
There was a problem hiding this comment.
we're always running on localhost from the perspective of 1. always either running within the docker container or 2. running locally e.g. using pyenv or something. In this way, all ports/addresses are referencing internal/local addresses.
tests/test_main.py
Outdated
| f = asyncio.Future() | ||
| f.set_result(Response(status_code=200)) | ||
|
|
||
| def mock_call_next(request: Request) -> Any: |
There was a problem hiding this comment.
@pattisdr I'm unsure if you'd have any pointers here, but I'm struggling to mock the return of an async function, while also preserving the BackgroundTask that is added to the response here- 53f414e#diff-51868caf44d288309a19f7b59360f148adfa01226f70bf528441cef723439ffcR57
Currently, this code will fail at send_analytics_request_mock.assert_called_once_with() because the background task is not getting added properly using this async mock.
There was a problem hiding this comment.
have you tried AsyncMocks? https://docs.python.org/3.8/library/unittest.mock.html#unittest.mock.AsyncMock
There was a problem hiding this comment.
checking this out!
…place for hostname validation
|
@pattisdr this is ready for another pass. Thanks for being patient! Couple things to note:
|
|
thanks @eastandwestwind, I'll review this AM |
|
@eastandwestwind what about unit tests of this format? just testing that class TestPatchConnections:
...
@mock.patch('fidesops.main.prepare_and_log_request')
def test_patch_connections_incorrect_scope_analytics(
self, mocked_prepare_and_log_request, api_client: TestClient, generate_auth_header, payload
) -> None:
url = V1_URL_PREFIX + CONNECTIONS
auth_header = generate_auth_header(scopes=[STORAGE_DELETE])
response = api_client.patch(url, headers=auth_header, json=payload)
assert 403 == response.status_code
assert mocked_prepare_and_log_request.called
call_args = mocked_prepare_and_log_request._mock_call_args[0]
assert call_args[0] == 'PATCH: http://testserver/api/v1/connection'
assert call_args[1] == 403
assert isinstance(call_args[2], datetime)
assert call_args[3] is None
assert call_args[4] == 'HTTPException'
@mock.patch('fidesops.main.prepare_and_log_request')
def test_patch_http_connection_successful_analytics(
self, mocked_prepare_and_log_request, api_client, db: Session, generate_auth_header, url
):
auth_header = generate_auth_header(scopes=[CONNECTION_CREATE_OR_UPDATE])
payload = [
{
"name": "My Post-Execution Webhook",
"key": "webhook_key",
"connection_type": "https",
"access": "read",
}
]
response = api_client.patch(url, headers=auth_header, json=payload)
assert 200 == response.status_code
body = json.loads(response.text)
assert body["succeeded"][0]["connection_type"] == "https"
http_config = ConnectionConfig.get_by(db, field="key", value="webhook_key")
http_config.delete(db)
call_args = mocked_prepare_and_log_request._mock_call_args[0]
assert call_args[0] == 'PATCH: http://testserver/api/v1/connection'
assert call_args[1] == 200
assert isinstance(call_args[2], datetime)
assert call_args[3] is None
assert call_args[4] is None |
There was a problem hiding this comment.
nice work @eastandwestwind
Let's also make sure that the landing page that @TheAndrewJackson is working on is adding this X-Fides-Source to the request headers there too
…s for each endpoint (#622)
Purpose
Adds middleware for analytics events on endpoints
Changes
fidesopsendpoints that callsfideslogwith endpoint path, response code, source, and error classX-Fides-Sourceheader to all endpoint calls from admin ui and privacy center uiprefer-default-exportfor eslint in admin-uiChecklist
CHANGELOG.mdfileCHANGELOG.mdfile is being appended toUnreleasedsection in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.Run Unsafe PR Checkslabel has been applied, and checks have passed, if this PR touches any external servicesTicket
Fixes #559