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

Add UserAudit for user activation and deactivation #35134

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jingcheng16
Copy link
Contributor

@jingcheng16 jingcheng16 commented Sep 17, 2024

Product Description

As a follow up for SSO work, when we auto-deactivate web user, or when web user get reactivated, those action should be logged for future reference and for easy debug.

But UserHistory report still miss some records:

  1. Those auto-deactivation and reactivation actually won't have a for-domain or by-domain property, because they're operated at user level not domain level.
    UserHistory report only shows record that are for current domain, I made the change so we can see record for current domain and for no specific domain.

  2. auto-deactivation is not changed by any user but system; deactivation and reactivation can also be triggered by a super user, we want to include those records in UserHistory report too. However, UserHistory report will exclude records whose changed_by_user property is not from the current domain, I removed that restriction.

Technical Summary

Ticket: https://dimagi.atlassian.net/browse/SAAS-15968

Feature Flag

The change is specific to user_history_report flag.
wiki: https://dimagi.atlassian.net/wiki/spaces/saas/pages/2146603609/User+History+Report

Safety Assurance

Safety story

The change is behind the feature flag, and this feature flag is for internal use.
It is tested heavily on staging because I use it to debug the sso issue.

Automated test coverage

QA Plan

No QA

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

Copy link

sentry-io bot commented Sep 17, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: corehq/apps/sso/backends.py

Function Unhandled Issue
authenticate AttributeError: 'SsoBackend' object has no attribute 'user' /sso/{idp_slug}...
Event Count: 1
📄 File: corehq/apps/sso/tasks.py (Click to Expand)
Function Unhandled Issue
auto_deactivate_removed_sso_users TypeError: log_user_change() missing 1 required positional argument: 'changed_by_user' /hq...
Event Count: 1
---

Did you find this useful? React with a 👍 or 👎

@jingcheng16 jingcheng16 added the product/feature-flag Change will only affect users who have a specific feature flag enabled label Sep 17, 2024
@jingcheng16 jingcheng16 marked this pull request as ready for review September 17, 2024 22:55
@jingcheng16 jingcheng16 requested review from mkangia, biyeun and orangejenny and removed request for biyeun September 17, 2024 22:55
@orangejenny
Copy link
Contributor

UserHistory report only shows record that are for current domain, I made the change so we can see record for current domain and for no specific domain

Just verifying, did you discuss this change with product? Visibility into information that isn't "owned" by the domain is a grey area.

@jingcheng16
Copy link
Contributor Author

@orangejenny Hi Jenny, I haven't. I will ask in gtd-product.

Copy link
Contributor

@mkangia mkangia left a comment

Choose a reason for hiding this comment

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

Just verifying, did you discuss this change with product? Visibility into information that isn't "owned" by the domain is a grey area.

Yup. The report should not show data not "owned" by the domain i.e data that is not relevant to the project.
You could show the web user's membership activated/deactivated only for that domain.

Definitely to be discussed with product before continuing with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/feature-flag Change will only affect users who have a specific feature flag enabled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants