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

Store user state in Audit Trail on certain user events #17507

Open
sarahelsaig opened this issue Feb 21, 2025 · 8 comments · May be fixed by #17518
Open

Store user state in Audit Trail on certain user events #17507

sarahelsaig opened this issue Feb 21, 2025 · 8 comments · May be fixed by #17518

Comments

@sarahelsaig
Copy link
Contributor

Is your feature request related to a problem?

Currently if you have attached user settings (name, address, etc) they are not stored in Audit Trail events. (e.g. mentioned here) It would be good to store them on events that trigger when the User is changed.

Describe the solution you'd like

It would be good to either add an AuditTrailUserEvent.User property (for parity with AuditTrailContentEvent.ContentItem) or to create a separate AuditTrailUserStateEvent. Whichever makes more sense.

It should trigger on IUserEventHandler's past tense methods (e.g. on UpdatedAsync but not on UpdatingAsync). Unlike the current UserEventHandler it should not trigger on ILoginFormEvent to avoid cluttering.

Describe alternatives you've considered

I can't think of any.

@hishamco
Copy link
Member

Are you to submit a PR?

@sarahelsaig
Copy link
Contributor Author

Most likely yes. Though first I want to see if there are any comments or considerations beyond what I've written above.

Also @domonkosgabor , any thoughts?

@domonkosgabor
Copy link
Contributor

I like this idea!

@Piedone
Copy link
Member

Piedone commented Feb 23, 2025

Is this related to having richer user profiles? Because that BTW can be achieved via content items (which have Audit Trail diff support already): https://docs.orchardcore.net/en/latest/reference/modules/Users/CustomUserSettings/

@sarahelsaig
Copy link
Contributor Author

Yes, that would be the goal. To clarify, are you talking about content types with the CustomUserSettings stereotype getting an Audit Trail "content" event, right? Because that doesn't happen for either custom user settings or custom site settings (or really any other kind of embedded content item). For example:

  1. Enable Custom User Settings ,Audit Trail, and Users Audit Trail features.
  2. Create a new content definition called User Profile with some fields and give it CustomUserSettings stereotype.
  3. Go to Admin > Configuration > Settings > Audit Trail.
  4. Select Content Tab and tick the box for User Profile and also for some other content type (e.g. Page) for reference.
  5. Create a new content item for that other content type to ensure that the content events are logged.
  6. Go to Admin > Security > Users and edit a user.
  7. Fill in the User Profile tab.
  8. Save user.
  9. Go to Admin > Audit Trail.
  10. Observe that there was no content event for User Profile (but the data would be available in the User event if the User object was snapshotted as suggested above):

Image

@Piedone
Copy link
Member

Piedone commented Feb 23, 2025

I was thinking about this, yes. I see, so the issue is that such content items are not individual items but part of the User entity. Hmm. Then yes, I suppose what you're asking about here is valid.

@sebastienros
Copy link
Member

Security considerations mentioned on the PR

@sebastienros sebastienros added this to the 3.x milestone Feb 27, 2025
Copy link
Contributor

We triaged this issue and set the milestone according to the priority we think is appropriate (see the docs on how we triage and prioritize issues).

This indicates when the core team may start working on it. However, if you'd like to contribute, we'd warmly welcome you to do that anytime. See our guide on contributions here.

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

Successfully merging a pull request may close this issue.

5 participants