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

Introduce security log. #4675

Merged
merged 13 commits into from
Jul 17, 2020
Merged

Introduce security log. #4675

merged 13 commits into from
Jul 17, 2020

Conversation

maliming
Copy link
Member

@maliming maliming commented Jul 8, 2020

How to write security logs.

You can inject and use ISecurityLogManager to write security logs. It will create a log object by default and fill in some common values, such as CreationTime, ClientIpAddress, BrowserInfo, current user/tenant, etc. Of course, you can override them.

await SecurityLogManager.SaveAsync(securityLog =>
{
	securityLog.Identity = "IdentityServer";
	securityLog.Action = "ResourceOwnerPassword";
});

The security log store is implemented in Identity module.

Configure AbpSecurityLogOptions to provide the application name for the log or disable this feature. enabed by default.

Configure<AbpSecurityLogOptions>(x =>
{
	x.ApplicationName = "AbpSecurityTest";
});

@maliming maliming requested a review from hikalkan July 10, 2020 07:35
@hikalkan hikalkan merged commit cce1610 into dev Jul 17, 2020
@hikalkan hikalkan deleted the maliming/UserSecurityLog branch July 17, 2020 12:38
securityLog.UserName = eventData.UserName;
}

if (securityLog.ClientId.IsNullOrWhiteSpace())
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be vice verse? I mean a filled eventData.ClientId should override the securityLog.ClientId. What do you think?
I didn't think much, but it seems like that. If that's true, no problem.

return;
}

using (var uow = UnitOfWorkManager.Begin(requiresNew: true))
Copy link
Member

Choose a reason for hiding this comment

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

This can also be a "non transactional" UOW since it contains only one insert. It will reduce the dedlock probability.

Copy link
Member Author

Choose a reason for hiding this comment

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

Begin(requiresNew: true) is extension method.

public static IUnitOfWork Begin(
[NotNull] this IUnitOfWorkManager unitOfWorkManager,
bool requiresNew = false,
bool isTransactional = false,
IsolationLevel? isolationLevel = null,
int? timeout = null)
{

@hikalkan
Copy link
Member

I accepted the PR. Added two notes, so you can check and fix theme if necessary.

But the essentional design question is that: Why we are using event bus to save a security log. We should be able to directly use the ISecurityLogManager. Using evenbus is not a good way if we can use direct method calls.
If you want to decouple something, you can simply create an interface and implement in the Identity module.

Please re-consider this design and drop using the event bus.

@maliming
Copy link
Member Author

Open a new PR for the above comment. #4810

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.

2 participants