-
Notifications
You must be signed in to change notification settings - Fork 368
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 audit logs for oauth application management #2104
Conversation
@@ -0,0 +1,55 @@ | |||
/* | |||
* Copyright (c) 2023, WSO2 Inc. (https://www.wso2.org) All Rights Reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update the license header to LLC one. Check other places as well
private final String eventDescription; | ||
|
||
private EventCatalog(String eventId, String eventDescription) { | ||
this.eventId = eventId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing new line after the method signature
check other places as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this class and moved to string constant. Using enum might give some additional restrictions. In future, we need to have an osgi service to register all the suppported events. Using that, we can have some REST API to get the supported events
return; | ||
} | ||
String secret = oauthApp.get("oauthConsumerSecret").toString(); | ||
String maskedSecret = secret.replaceAll(MASKING_REGEX, MASKING_CHARACTER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to use LoggerUtils.getMaskedContent
https://github.com/sahandilshan/carbon-identity-framework/blob/c265d493f395c0937e56a9e84c9625126341a6df/components/central-logger/org.wso2.carbon.identity.central.log.mgt/src/main/java/org/wso2/carbon/identity/central/log/mgt/utils/LoggerUtils.java#L195 here?
It has been used in maskAppOwnerUsername
method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
433100e
to
d446651
Compare
PR builder started |
2beebd1
to
0b90f97
Compare
PR builder completed |
PR builder started |
PR builder completed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/5654380136
Repo build https://github.com/wso2/product-is/suites/14543169080/artifacts/823235542 and integration tests builds are successful |
private static void maskAppOwnerUsername(JSONObject oauthAppJSONObject) throws IdentityException { | ||
|
||
JSONObject appOwner = oauthAppJSONObject.optJSONObject("appOwner"); | ||
if (!LoggerUtils.isLogMaskingEnable || appOwner == null) { | ||
return; | ||
} | ||
String username = (String) appOwner.get("userName"); | ||
if (StringUtils.isNotBlank(username)) { | ||
appOwner.put("userName", LoggerUtils.getMaskedContent(username)); | ||
} | ||
} | ||
|
||
private static void maskClientSecret(JSONObject oauthApp) { | ||
|
||
if (oauthApp.get("oauthConsumerSecret") == null) { | ||
return; | ||
} | ||
String secret = oauthApp.get("oauthConsumerSecret").toString(); | ||
oauthApp.put("oauthConsumerSecret", LoggerUtils.getMaskedContent(secret)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we merge these two and have a single method like maskField
?
eg: maskField(JSONObject jsonObject, String... path)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and call that in the maskSPData method like;
maskField(oauthAppJSONObject, "appOwner", "userName");
maskField(oauthAppJSONObject, "oauthConsumerSecret");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can have a single method, but still the logic will be different. For example, clientSecret is available as first class attribute in the path. But username is available as "appOwner:{"username":"xxx}". The path of the masking object is not a flat one. In some cases, it is nested.
String tenantDomain = IdentityTenantUtil.getTenantDomainFromContext(); | ||
if (StringUtils.isBlank(tenantDomain)) { | ||
tenantDomain = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain(); | ||
} | ||
return tenantDomain; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
String tenantDomain = IdentityTenantUtil.getTenantDomainFromContext(); | |
if (StringUtils.isBlank(tenantDomain)) { | |
tenantDomain = PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain(); | |
} | |
return tenantDomain; | |
return Optional.ofNullable(IdentityTenantUtil.getTenantDomainFromContext()) | |
.filter(StringUtils::isNotBlank) | |
.orElseGet(() -> PrivilegedCarbonContext.getThreadLocalCarbonContext().getTenantDomain()); | |
} |
PR builder started |
PR builder completed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/5668712309
Repo PR builder is failing with the error:
|
Proposed changes in this pull request
Fixes part of wso2/product-is#5037
This PR adds new audit logs for oauth application management component.
To enable these audit logs, we need to add below config in the deployment.toml file
By adding below config, you can enable new audit logs only for application management component, and the legacy audit logs will be available for other components.
By adding below config, you can enable new audit logs , and the legacy audit logs will not available for other components. These new audit logs are currently inprogress only for application-mgt component
Sample audit logs
Delete OAuth application
Create OAuth application
Update OAuth application protocol configs
Regenerate client secret
Revoke OAuth application
Activate the OAuth application
When activating a revoked oauth application, client secret is also regenerated
When should this PR be merged
[Please describe any preconditions that need to be addressed before we
can merge this pull request.]
Follow up actions
[List any possible follow-up actions here; for instance, testing data
migrations, software that we need to install on staging and production
environments.]
Checklist (for reviewing)
General
Functionality
Code
Tests
Security
Documentation