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

PXP-7447 audit-service integration #885

Merged
merged 13 commits into from
Mar 24, 2021
Merged

PXP-7447 audit-service integration #885

merged 13 commits into from
Mar 24, 2021

Conversation

paulineribeyre
Copy link
Contributor

@paulineribeyre paulineribeyre commented Mar 15, 2021

Jira Ticket: PXP-7447

Audit Service integration:

  • add audit service client class, configuration to enable audit logs, and create audit logs when a user downloads data or logs in (during post_login)
  • add post_login calls to all IDPs' callback endpoints, and add a unit test to make sure all future IDP implementations call it too
  • if audit logs are enabled, Fence will not come up if it can't reach audit-service. However if audit-service crashes while Fence is already up, Fence will crash if a user logs in or attempts to download data. PXP-7805 will improve this
  • only recording successful logins and successful download (not upload) presigned URL requests for now

And other changes:

  • remove unused handle_login function and mock_get, authentication_headers fixtures
  • the FenceLogin (/login/fence), FenceCallback (/login/fence/login), ShibbolethLogin (/login/shib) and ShibbolethCallback (/login/shib/login) classes now inherit from the same base classes as other IDP endpoints
  • the /login/fence/login endpoint now ignores mismatched states if MOCK_AUTH == True to facilitate testing
  • add error handling to the /login/ras/callback endpoint so we get logs if refresh_token or idp_token are missing before crashing
  • remove use of deprecated DEFAULT_LOGIN_URL in the /oauth2 endpoint; check DEFAULT_LOGIN_IDP and use this IDP's URL instead
  • add fence_idp and shib_idp to userinfo endpoint

New Features

  • Audit Service integration: Fence creates audit logs for data downloads and user logins

Deployment changes

  • Audit logs for user logins and data downloads can be enabled by deploying the audit-service and adding setting ENABLE_AUDIT_LOGS to the fence config (see
    ENABLE_AUDIT_LOGS:
    presigned_url: false
    login: false
    )

@coveralls
Copy link

coveralls commented Mar 15, 2021

Pull Request Test Coverage Report for Build 10605

  • 115 of 155 (74.19%) changed or added relevant lines in 14 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+1.2%) to 70.664%

Changes Missing Coverage Covered Lines Changed/Added Lines %
fence/resources/userdatamodel/userdatamodel_project.py 0 1 0.0%
fence/auth.py 4 6 66.67%
fence/resources/user/init.py 3 5 60.0%
fence/blueprints/login/base.py 4 7 57.14%
fence/blueprints/login/fence_login.py 11 15 73.33%
fence/blueprints/oauth2.py 13 19 68.42%
fence/resources/audit_service_client.py 40 62 64.52%
Files with Coverage Reduction New Missed Lines %
fence/blueprints/oauth2.py 1 77.78%
fence/blueprints/login/fence_login.py 2 73.91%
Totals Coverage Status
Change from base Build 10593: 1.2%
Covered Lines: 5899
Relevant Lines: 8348

💛 - Coveralls

@github-actions
Copy link

The style in this PR agrees with black. ✔️

This formatting comment was generated automatically by a script in uc-cdis/wool.

fence/__init__.py Outdated Show resolved Hide resolved
fence/auth.py Outdated Show resolved Hide resolved
@ocshawn
Copy link

ocshawn commented Mar 18, 2021

looks good to me

create_login_log(self.idp_name)


def create_login_log(idp_name):
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a thought (no need to take any action), but I wonder if we could make our own @audit decorator where needed to generically (more or less) allow for auditable functions 🤔

# The audit-service returns 201 before inserting the log in the DB.
# This request should only error if the input is incorrect (status
# code 422) or if the service is unreachable.
if resp.status_code != 201:
Copy link
Contributor

Choose a reason for hiding this comment

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

If the audit service becomes unavailable for any reason or an unexpected error occurs, is that acceptable for the user story? In that case might we lose some fidelity on auditing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm not sure i understand your question but yes, fence crashing in case of audit-service failure is fine for this ticket, there's a separate ticket to improve this

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I worded that poorly, and thanks for confirming! I was wondering if there were some cases where maybe we'd send an audit log request before an action like...

# 1 Track an audit entry
POST /audit
# 2 Perform some action
POST /login

And if it was possible for the /audit action to fail, but the /login action to succeed. And so maybe we'd log in but not track it properly, and I was wondering if that was a problem. I wasn't sure if it could be a problem that we didn't have 100% confidence on how well we logged all our auditable behavior.

But it sounds like things should crash for now if that happens and then we'll enhance in the future, so nevermind! 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gotcha, yeah that would be a problem. the current plan is to use a queueing service that Fence can post to and the audit service can read from

Copy link
Contributor

@williamhaley williamhaley left a comment

Choose a reason for hiding this comment

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

Just a few questions, but lgtm! :shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants