Skip to content

Fix/more audit logs#1322

Merged
Avantol13 merged 5 commits intomasterfrom
fix/more-audit-logs
Mar 18, 2026
Merged

Fix/more audit logs#1322
Avantol13 merged 5 commits intomasterfrom
fix/more-audit-logs

Conversation

@k-burt-uch
Copy link
Contributor

@k-burt-uch k-burt-uch commented Jan 13, 2026

Link to JIRA ticket if there is one:

New Features

Breaking Changes

Bug Fixes

  • Adding missing audit bytes, duration and user-agent for presigned-urls audit logs

Improvements

Dependency updates

Deployment changes

@github-actions
Copy link

filepath passed failed skipped SUBTOTAL
tests/test_oauth2.py 15 0 0 15
tests/test_centralized_auth.py 16 0 0 16
tests/test_audit_service.py 2 1 3 6
tests/test_data_upload.py 8 0 1 9
tests/test_user_token.py 5 0 0 5
tests/test_drs_endpoint.py 4 0 0 4
tests/test_dbgap.py 4 0 1 5
tests/test_register_user.py 2 0 0 2
tests/test_google_data_access.py 1 0 0 1
tests/test_oidc_client.py 2 0 0 2
tests/test_client_credentials.py 1 0 0 1
tests/test_presigned_url.py 7 0 0 7
tests/test_ras_authn.py 0 0 3 3
TOTAL 67 1 8 76

Please find the detailed integration test report here

Please find the Github Action logs here

Add bytes and duration to presigned url audit log
@k-burt-uch k-burt-uch marked this pull request as ready for review January 13, 2026 22:52
@k-burt-uch k-burt-uch requested a review from Avantol13 January 13, 2026 22:52
@coveralls
Copy link

coveralls commented Jan 13, 2026

Pull Request Test Coverage Report for Build 23218434397

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 36 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.02%) to 75.033%

Files with Coverage Reduction New Missed Lines %
resources/audit/utils.py 4 94.94%
blueprints/data/indexd.py 32 94.56%
Totals Coverage Status
Change from base Build 23211795271: 0.02%
Covered Lines: 8442
Relevant Lines: 11251

💛 - Coveralls

@github-actions
Copy link

filepath passed failed skipped SUBTOTAL
tests/test_oauth2.py 15 0 0 15
tests/test_centralized_auth.py 15 1 0 16
tests/test_audit_service.py 3 0 3 6
tests/test_data_upload.py 8 0 1 9
tests/test_presigned_url.py 7 0 0 7
tests/test_dbgap.py 4 0 1 5
tests/test_user_token.py 5 0 0 5
tests/test_drs_endpoint.py 4 0 0 4
tests/test_register_user.py 2 0 0 2
tests/test_google_data_access.py 1 0 0 1
tests/test_client_credentials.py 1 0 0 1
tests/test_oidc_client.py 2 0 0 2
tests/test_ras_authn.py 0 0 3 3
TOTAL 67 1 8 76

Please find the detailed integration test report here

Please find the Github Action logs here

@github-actions
Copy link

filepath passed SUBTOTAL
tests/test_centralized_auth.py 16 16
TOTAL 16 16

Please find the detailed integration test report here

Please find the Github Action logs here

@k-burt-uch
Copy link
Contributor Author

k-burt-uch commented Jan 20, 2026

Example from BDC Staging

 /ga4gh/drs/v1/objects/SYCVOIZBRY/access/TTCPCNXELZPQIRIKP?{"object_id":"SYCVOIZBRY","access_id":"TTCPCNXELZPQIRIKP"}                         |         404 | 2026-01-19 05:06:50 | UCtestuser999               |  1452 | SYCVOIZBRY                                   |                                                  | download |          | 311567 | ["X-Forwarded-For:<redacted>", "X-Userid:uid:1452,UCtestuser999", "X-Reqid:b54b361a58efb79cc797e9fd4443e8d3", "X-Sessionid:c2a32aa64ef837b245f9d23df0f063c9", "X-Visitorid:c2a32aa64ef837b245f9d23df0f063c9", "X-Forwarded-Proto:https", "X-Forwarded-Port:443", "X-Amzn-Trace-Id:Root=1-696dbbea-75b48dc33ecce21877509b81", "User-Agent:Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/122.0.6261.171 Safari/537.36", "duration:0.14856767654418945", "bytes:3036"]

Specifically

"User-Agent:Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/122.0.6261.171 Safari/537.36", "duration:0.14856767654418945", "bytes:3036"

Copy link
Contributor

@Avantol13 Avantol13 left a comment

Choose a reason for hiding this comment

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

There are still some fields we should try to populate automatically if possible:

    server_name = request.environ.get('SERVER_NAME')
    server_port = request.environ.get('SERVER_PORT')
    # TODO: construct and add to audit logs
  • user_id_provider (e.g. RAS)
    • Perhaps from user.identity_provider.name ?
  • user_permission_group (e.g. dbGaP)
    • This one's a little trickier. Maybe check if config.dbGaP has any entry with nih.gov in the hostname? A little messy, but we allow non-NIH dbgap servers to be configured here (like in DCF), so we'll want something a little more foolproof. So if it's there, put "dbGaP". If it's not, put: "non-dbGaP"?
  • http_method
    • This isn't required but it seems like we should just add it
    • base it on the endpoint or do it automagically if this works:
    method = request.environ.get('REQUEST_METHOD')
    # TODO: add to audit logs

Co-authored-by: Alexander VanTol <Avantol13@users.noreply.github.com>
@github-actions
Copy link

filepath SUBTOTAL
TOTAL 0

Please find the detailed integration test report here

Please find the Github Action logs here

@k-burt-uch
Copy link
Contributor Author

  • dest_ip

uc-cdis/gen3-helm#492 This will get picked up by the X- header capturing

  • http_method
    This is easy so done - need to update unit tests.
  • user_id_provider

This is a tough one -

def _get_auth_info_for_id_or_from_request(
is called from
auth_info = _get_auth_info_for_id_or_from_request(
so we won't have the full user object at audit time. I could add a db lookup to get it in that method, but this will add another DB call to the presigned-url generation. I'm not sure how to handle a situation where the token is valid but we don't have the user saved to the fence user table though - is this even possible? IDK - what do you think? Extra db call to presigned-url generation or forgo it for now?

  • user_permission_group

I can add the config check but I don't think its a good solution for this. The best we have is the authzProvider here: https://github.com/uc-cdis/arborist/blob/master/arborist/user.go#L162 but we'll need to reach out to arborist or have this returned during the authz check and refactor fence code to ensure its present at time of audit (similar to the user_id_provider issue)

tl;dr
destIp can be covered by the gen3-helm PR, user_id_provider and user_permission_group would require refactoring so that complete user information is present at the time of presigned-url generation which may be beyond the scope of what we want right now.

@github-actions
Copy link

filepath passed SUBTOTAL
tests/test_centralized_auth.py 16 16
TOTAL 16 16

Please find the detailed integration test report here

Please find the Github Action logs here

@Avantol13
Copy link
Contributor

  • dest_ip

uc-cdis/gen3-helm#492 This will get picked up by the X- header capturing

  • http_method
    This is easy so done - need to update unit tests.
  • user_id_provider

This is a tough one -

def _get_auth_info_for_id_or_from_request(

is called from

auth_info = _get_auth_info_for_id_or_from_request(

so we won't have the full user object at audit time. I could add a db lookup to get it in that method, but this will add another DB call to the presigned-url generation. I'm not sure how to handle a situation where the token is valid but we don't have the user saved to the fence user table though - is this even possible? IDK - what do you think? Extra db call to presigned-url generation or forgo it for now?

  • user_permission_group

I can add the config check but I don't think its a good solution for this. The best we have is the authzProvider here: https://github.com/uc-cdis/arborist/blob/master/arborist/user.go#L162 but we'll need to reach out to arborist or have this returned during the authz check and refactor fence code to ensure its present at time of audit (similar to the user_id_provider issue)

tl;dr destIp can be covered by the gen3-helm PR, user_id_provider and user_permission_group would require refactoring so that complete user information is present at the time of presigned-url generation which may be beyond the scope of what we want right now.

We can/should dump necessary user info into the token itself then. We already have some contextual user information embedded in access tokens (in context.user) - we could add user_id_provider and user_permission_group there, and then avoid db calls (just decode the token)? For RAS Passports, we can "hard-code" RAS and dbGaP for audit logs

@github-actions
Copy link

filepath passed SUBTOTAL
tests/test_centralized_auth.py 16 16
TOTAL 16 16

Please find the detailed integration test report here

Please find the Github Action logs here

@github-actions
Copy link

Test summary after running integration tests

filepath passed failed skipped SUBTOTAL
tests/test_oauth2.py 15 0 0 15
tests/test_centralized_auth.py 15 1 0 16
tests/test_data_upload.py 7 1 1 9
tests/test_audit_service.py 2 1 3 6
tests/test_presigned_url.py 7 0 0 7
tests/test_dbgap.py 3 1 1 5
tests/test_drs_endpoint.py 4 0 0 4
tests/test_user_token.py 5 0 0 5
tests/test_register_user.py 2 0 0 2
tests/test_google_data_access.py 1 0 0 1
tests/test_client_credentials.py 1 0 0 1
tests/test_oidc_client.py 2 0 0 2
tests/test_ras_authn.py 0 0 3 3
TOTAL 64 4 8 76

Test summary after rerunning failed integration tests

filepath passed SUBTOTAL
tests/test_dbgap.py 1 1
tests/test_centralized_auth.py 1 1
tests/test_audit_service.py 1 1
tests/test_data_upload.py 1 1
TOTAL 4 4

Please find the detailed integration test report here

Please find the detailed integration test report after rerunning failed tests here

Please find the Github Action logs here

Copy link
Contributor

@Avantol13 Avantol13 left a comment

Choose a reason for hiding this comment

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

added PD-141 to address user_id_provider and user_permission_group

@Avantol13 Avantol13 merged commit 6d9f1bc into master Mar 18, 2026
11 checks passed
@Avantol13 Avantol13 deleted the fix/more-audit-logs branch March 18, 2026 16:08
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.

4 participants