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.

Sign up for GitHub

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

Synchronize user role retrieval from DB and adding it to user role cache #2899

Draft
wants to merge 3 commits into
base: 4.6.x-old
Choose a base branch
from
Draft

Synchronize user role retrieval from DB and adding it to user role cache #2899

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ba2f6da
Synchronize user role retrieval from DB and adding it to cache to avo…
DinikaSen Jan 18, 2021
43e0269
Reformat synchronized methods
DinikaSen Jan 18, 2021
579eb5e
Add debug logs
DinikaSen Jan 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 79 additions & 57 deletions ...on.user.core/src/main/java/org/wso2/carbon/user/core/common/AbstractUserStoreManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8418,47 +8418,61 @@ public List<String> doGetRoleListOfUserWithID(String userID, String filter) thro

private List<String> getUserRolesWithID(String userID, String filter) throws UserStoreException {

List<String> internalRoles = doGetInternalRoleListOfUserWithID(userID, filter);
Set<String> modifiedInternalRoles = new HashSet<>();
String[] modifiedExternalRoleList = new String[0];

if (readGroupsEnabled && doCheckExistingUserWithID(userID)) {
List<String> roles = new ArrayList<>();
String[] externalRoles = doGetExternalRoleListOfUserWithID(userID, "*");
roles.addAll(Arrays.asList(externalRoles));
if (isSharedGroupEnabled()) {
String[] sharedRoles = doGetSharedRoleListOfUserWithID(userID, null, "*");
if (sharedRoles != null) {
roles.addAll(Arrays.asList(sharedRoles));
List<String> userRoleList;
String username = getUserNameFromUserID(userID);
synchronized (userID.intern()) {
if (username != null) {
String[] roleListOfUserFromCache = getRoleListOfUserFromCache(this.tenantId, username);
if (roleListOfUserFromCache != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to start synchronized block only if if (roleListOfUserFromCache == null) { right?

Copy link
Author

Choose a reason for hiding this comment

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

Here the if (roleListOfUserFromCache == null) check is done inside the synchronized block for double-checked locking purpose. There is another if (roleListOfUserFromCache == null) check in the calling method right before this method is called.

Copy link

Choose a reason for hiding this comment

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

@DinikaSen There is a pattern to follow when using synchronise block. Always advisable to follow the pattern so that code and logic is simpler and less error prone

if (! checkCondition() ) {
syncronized (lock) {
if (checkCondition() ) {exit;}

List<String> roleList = Arrays.asList(roleListOfUserFromCache);
if (!roleList.isEmpty()) {
return roleList;
}
}
}
modifiedExternalRoleList = UserCoreUtil.addDomainToNames(roles.toArray(new String[0]), getMyDomainName());

// Get the associated internal roles of the groups.
if (isRoleAndGroupSeparationEnabled()) {
Set<String> rolesOfGroups = getUniqueSet(getHybridRoleListOfGroups(roles, getMyDomainName()));
modifiedInternalRoles.addAll(rolesOfGroups);
List<String> internalRoles = doGetInternalRoleListOfUserWithID(userID, filter);
Set<String> modifiedInternalRoles = new HashSet<>();
String[] modifiedExternalRoleList = new String[0];

if (readGroupsEnabled && doCheckExistingUserWithID(userID)) {
List<String> roles = new ArrayList<>();
String[] externalRoles = doGetExternalRoleListOfUserWithID(userID, "*");
roles.addAll(Arrays.asList(externalRoles));
if (isSharedGroupEnabled()) {
String[] sharedRoles = doGetSharedRoleListOfUserWithID(userID, null, "*");
if (sharedRoles != null) {
roles.addAll(Arrays.asList(sharedRoles));
}
}
modifiedExternalRoleList = UserCoreUtil.addDomainToNames(roles.toArray(new String[0]), getMyDomainName());

// Get the associated internal roles of the groups.
if (isRoleAndGroupSeparationEnabled()) {
Set<String> rolesOfGroups = getUniqueSet(getHybridRoleListOfGroups(roles, getMyDomainName()));
modifiedInternalRoles.addAll(rolesOfGroups);
}
}
}
modifiedInternalRoles.addAll(internalRoles);
String[] roleList = UserCoreUtil.combine(modifiedExternalRoleList, new ArrayList<>(modifiedInternalRoles));
modifiedInternalRoles.addAll(internalRoles);
String[] roleList = UserCoreUtil.combine(modifiedExternalRoleList, new ArrayList<>(modifiedInternalRoles));
userRoleList = Arrays.asList(roleList);

for (UserOperationEventListener userOperationEventListener : UMListenerServiceComponent
.getUserOperationEventListeners()) {
if (userOperationEventListener instanceof AbstractUserOperationEventListener) {
if (!((AbstractUserOperationEventListener) userOperationEventListener)
.doPostGetRoleListOfUserWithID(userID, filter, roleList, this)) {
break;
for (UserOperationEventListener userOperationEventListener : UMListenerServiceComponent
.getUserOperationEventListeners()) {
if (userOperationEventListener instanceof AbstractUserOperationEventListener) {
if (!((AbstractUserOperationEventListener) userOperationEventListener)
.doPostGetRoleListOfUserWithID(userID, filter, roleList, this)) {
break;
}
}
}
}

// Add to user role cache uisng username.
String username = getUserNameFromUserID(userID);
if (username != null) {
addToUserRolesCache(this.tenantId, username, roleList);
// Add to user role cache using username.
if (username != null) {
addToUserRolesCache(this.tenantId, username, roleList);
}
}
return Arrays.asList(roleList);
return userRoleList;
}

/**
Expand Down Expand Up @@ -8487,40 +8501,48 @@ public final String[] doGetRoleListOfUser(String userName, String filter) throws

private String[] getUserRoles(String username, String filter) throws UserStoreException {

String[] internalRoles = doGetInternalRoleListOfUser(username, filter);
String[] modifiedExternalRoleList = new String[0];
String[] roleList;
String usernameWithTenantDomain = username + "@" + this.getTenantDomain(this.tenantId);
synchronized (usernameWithTenantDomain.intern()) {
roleList = getRoleListOfUserFromCache(this.tenantId, username);
if (roleList != null && roleList.length > 0) {
return roleList;
}
String[] internalRoles = doGetInternalRoleListOfUser(username, filter);
String[] modifiedExternalRoleList = new String[0];

if (readGroupsEnabled && doCheckExistingUser(username)) {
String[] externalRoles = doGetExternalRoleListOfUser(username, "*");
List<String> roles = Arrays.asList(externalRoles);
if (isSharedGroupEnabled()) {
String[] sharedRoles = doGetSharedRoleListOfUser(username, null, "*");
if (sharedRoles != null) {
roles.addAll(Arrays.asList(sharedRoles));
if (readGroupsEnabled && doCheckExistingUser(username)) {
String[] externalRoles = doGetExternalRoleListOfUser(username, "*");
List<String> roles = Arrays.asList(externalRoles);
if (isSharedGroupEnabled()) {
String[] sharedRoles = doGetSharedRoleListOfUser(username, null, "*");
if (sharedRoles != null) {
roles.addAll(Arrays.asList(sharedRoles));
}
}
}
modifiedExternalRoleList = UserCoreUtil
.addDomainToNames(roles.toArray(new String[0]), getMyDomainName());
modifiedExternalRoleList = UserCoreUtil
.addDomainToNames(roles.toArray(new String[0]), getMyDomainName());

// Get the associated internal roles of the groups.
if (isRoleAndGroupSeparationEnabled()) {
Set<String> rolesOfGroups = getUniqueSet(getHybridRoleListOfGroups(roles, getMyDomainName()));
internalRoles = UserCoreUtil.combine(internalRoles, new ArrayList<>(rolesOfGroups));
// Get the associated internal roles of the groups.
if (isRoleAndGroupSeparationEnabled()) {
Set<String> rolesOfGroups = getUniqueSet(getHybridRoleListOfGroups(roles, getMyDomainName()));
internalRoles = UserCoreUtil.combine(internalRoles, new ArrayList<>(rolesOfGroups));
}
}
}

String[] roleList = UserCoreUtil.combine(internalRoles, Arrays.asList(modifiedExternalRoleList));
roleList = UserCoreUtil.combine(internalRoles, Arrays.asList(modifiedExternalRoleList));

for (UserOperationEventListener userOperationEventListener : UMListenerServiceComponent
.getUserOperationEventListeners()) {
if (userOperationEventListener instanceof AbstractUserOperationEventListener) {
if (!((AbstractUserOperationEventListener) userOperationEventListener)
.doPostGetRoleListOfUser(username, filter, roleList, this)) {
break;
for (UserOperationEventListener userOperationEventListener : UMListenerServiceComponent
.getUserOperationEventListeners()) {
if (userOperationEventListener instanceof AbstractUserOperationEventListener) {
if (!((AbstractUserOperationEventListener) userOperationEventListener)
.doPostGetRoleListOfUser(username, filter, roleList, this)) {
break;
}
}
}
addToUserRolesCache(this.tenantId, username, roleList);
}
addToUserRolesCache(this.tenantId, username, roleList);
return roleList;
}

Expand Down