Skip to content

Commit 12b0c50

Browse files
author
Krystian Brazulewicz
committed
#432 performance - use UserKey instead of UserProfile
1 parent 1f54e53 commit 12b0c50

File tree

20 files changed

+101
-142
lines changed

20 files changed

+101
-142
lines changed

‎bitbucket-slack-server-integration-plugin/src/main/java/com/atlassian/bitbucket/plugins/slack/notification/configuration/NotificationConfigurationContextBuilder.java

+4-4
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@
1111
import com.atlassian.plugins.slack.link.SlackLinkManager;
1212
import com.atlassian.plugins.slack.spi.SlackRoutesProviderFactory;
1313
import com.atlassian.plugins.slack.user.SlackUserManager;
14+
import com.atlassian.sal.api.user.UserKey;
1415
import com.atlassian.sal.api.user.UserManager;
15-
import com.atlassian.sal.api.user.UserProfile;
1616
import com.google.common.collect.ImmutableMap;
1717
import org.apache.commons.lang3.StringUtils;
1818
import org.springframework.beans.factory.annotation.Autowired;
@@ -70,10 +70,10 @@ public ImmutableMap.Builder<String, Object> addSlackViewContext(
7070

7171
if (link != null) {
7272
contextBuilder.put("link", link);
73-
UserProfile userProfile = userManager.getRemoteUser();
74-
if (userProfile != null) {
73+
UserKey userKey = userManager.getRemoteUserKey();
74+
if (userKey != null) {
7575
SlackClient client = slackClientProvider.withLink(link);
76-
slackUserManager.getByTeamIdAndUserKey(link.getTeamId(), userProfile.getUserKey().getStringValue())
76+
slackUserManager.getByTeamIdAndUserKey(link.getTeamId(), userKey.getStringValue())
7777
.flatMap(slackUser -> {
7878
contextBuilder.put("slackUserId", slackUser.getSlackUserId());
7979
return client.getUserInfo(slackUser.getSlackUserId()).toOptional();

‎bitbucket-slack-server-integration-plugin/src/test/java/com/atlassian/bitbucket/plugins/slack/notification/configuration/NotificationConfigurationContextBuilderTest.java

+4-7
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313
import com.atlassian.plugins.slack.user.SlackUserManager;
1414
import com.atlassian.sal.api.user.UserKey;
1515
import com.atlassian.sal.api.user.UserManager;
16-
import com.atlassian.sal.api.user.UserProfile;
1716
import com.github.seratch.jslack.api.model.User;
1817
import com.google.common.collect.ImmutableMap;
1918
import io.atlassian.fugue.Either;
@@ -56,8 +55,6 @@ public class NotificationConfigurationContextBuilderTest {
5655
@Mock
5756
SlackLink slackLink;
5857
@Mock
59-
UserProfile userProfile;
60-
@Mock
6158
SlackClient slackClient;
6259
@Mock
6360
SlackUser slackUser;
@@ -75,16 +72,16 @@ public class NotificationConfigurationContextBuilderTest {
7572
void addSlackViewContext() {
7673
List<SlackLink> links = new ArrayList<>();
7774
String teamId = "someTeamId";
78-
String userKey = "someUserKey";
75+
String userKeyStr = "someUserKey";
7976
String slackUserId = "someSlackUserId";
8077
String slackUserName = "someSlackUserRealName";
78+
UserKey stubUserKey = new UserKey(userKeyStr);
8179
when(slackLinkManager.getLinks()).thenReturn(links);
8280
when(slackLinkManager.getLinkByTeamId(teamId)).thenReturn(Either.right(slackLink));
8381
when(slackLink.getTeamId()).thenReturn(teamId);
84-
when(userManager.getRemoteUser()).thenReturn(userProfile);
85-
when(userProfile.getUserKey()).thenReturn(new UserKey(userKey));
82+
when(userManager.getRemoteUserKey()).thenReturn(stubUserKey);
8683
when(slackClientProvider.withLink(slackLink)).thenReturn(slackClient);
87-
when(slackUserManager.getByTeamIdAndUserKey(teamId, userKey)).thenReturn(Optional.of(slackUser));
84+
when(slackUserManager.getByTeamIdAndUserKey(teamId, userKeyStr)).thenReturn(Optional.of(slackUser));
8885
when(slackUser.getSlackUserId()).thenReturn(slackUserId);
8986
when(slackClient.getUserInfo(slackUserId)).thenReturn(Either.right(user));
9087
when(user.getRealName()).thenReturn(slackUserName);

‎confluence-slack-integration/confluence-slack-server-integration-plugin/src/main/java/com/atlassian/confluence/plugins/slack/spi/impl/ConfluenceSlackLinkAccessManager.java

+11-11
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,8 @@
55
import com.atlassian.confluence.spaces.SpaceManager;
66
import com.atlassian.confluence.user.UserAccessor;
77
import com.atlassian.plugins.slack.spi.impl.AbstractSlackLinkAccessManager;
8+
import com.atlassian.sal.api.user.UserKey;
89
import com.atlassian.sal.api.user.UserManager;
9-
import com.atlassian.sal.api.user.UserProfile;
1010
import com.atlassian.user.User;
1111
import org.springframework.beans.factory.annotation.Autowired;
1212
import org.springframework.beans.factory.annotation.Qualifier;
@@ -40,34 +40,34 @@ public ConfluenceSlackLinkAccessManager(@Qualifier("salUserManager") final UserM
4040
this.userAccessor = userAccessor;
4141
}
4242

43-
private boolean hasAccess(UserProfile userProfile, Optional<Space> space) {
43+
private boolean hasAccess(UserKey userKey, Optional<Space> space) {
4444
if (!space.isPresent()) {
4545
return false;
4646
}
4747

48-
User user = getUserByProfile(userProfile);
48+
User user = getUserByProfile(userKey);
4949
return isSpaceAdmin(user, space.get());
5050
}
5151

5252
@Override
53-
public boolean hasAccess(UserProfile userProfile, ContainerRequestContext request) {
54-
if (super.hasAccess(userProfile, request)) {
53+
public boolean hasAccess(UserKey userKey, ContainerRequestContext request) {
54+
if (super.hasAccess(userKey, request)) {
5555
return true;
5656
}
5757

5858
Optional<Space> space = getSpace(request);
5959

60-
return hasAccess(userProfile, space);
60+
return hasAccess(userKey, space);
6161
}
6262

6363
@Override
64-
public boolean hasAccess(UserProfile userProfile, HttpServletRequest request) {
65-
if (super.hasAccess(userProfile, request)) {
64+
public boolean hasAccess(UserKey userKey, HttpServletRequest request) {
65+
if (super.hasAccess(userKey, request)) {
6666
return true;
6767
}
6868
Optional<Space> space = getSpace(request);
6969

70-
return hasAccess(userProfile, space);
70+
return hasAccess(userKey, space);
7171
}
7272

7373
private Optional<Space> getSpace(ContainerRequestContext request) {
@@ -104,7 +104,7 @@ private boolean isSpaceAdmin(User user, Space space) {
104104
return permissionManager.hasPermission(user, ADMINISTER, space);
105105
}
106106

107-
private User getUserByProfile(UserProfile userProfile) {
108-
return userAccessor.getExistingUserByKey(userProfile.getUserKey());
107+
private User getUserByProfile(UserKey userKey) {
108+
return userAccessor.getExistingUserByKey(userKey);
109109
}
110110
}

‎confluence-slack-integration/confluence-slack-server-integration-plugin/src/test/java/com/atlassian/confluence/plugins/slack/spi/impl/ConfluenceSlackLinkAccessManagerTest.java

+11-25
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
import com.atlassian.confluence.user.UserAccessor;
99
import com.atlassian.sal.api.user.UserKey;
1010
import com.atlassian.sal.api.user.UserManager;
11-
import com.atlassian.sal.api.user.UserProfile;
1211
import com.atlassian.user.User;
1312
import org.junit.jupiter.api.Test;
1413
import org.junit.jupiter.api.extension.ExtendWith;
@@ -46,8 +45,6 @@ public class ConfluenceSlackLinkAccessManagerTest {
4645
@Mock
4746
private UserAccessor userAccessor;
4847

49-
@Mock
50-
private UserProfile userProfile;
5148
@Mock
5249
private ConfluenceUser user;
5350
@Mock
@@ -67,9 +64,8 @@ public class ConfluenceSlackLinkAccessManagerTest {
6764
@Test
6865
public void hasAccess_containerRequest_grantAccessForAdmin() {
6966
when(userManager.isAdmin(userKey)).thenReturn(true);
70-
when(userProfile.getUserKey()).thenReturn(userKey);
7167

72-
boolean result = target.hasAccess(userProfile, containerRequest);
68+
boolean result = target.hasAccess(userKey, containerRequest);
7369

7470
assertThat(result, is(true));
7571
}
@@ -78,9 +74,8 @@ public void hasAccess_containerRequest_grantAccessForAdmin() {
7874
public void hasAccess_containerRequest_grantAccessForSysAdmin() {
7975
when(userManager.isAdmin(userKey)).thenReturn(false);
8076
when(userManager.isSystemAdmin(userKey)).thenReturn(true);
81-
when(userProfile.getUserKey()).thenReturn(userKey);
8277

83-
boolean result = target.hasAccess(userProfile, containerRequest);
78+
boolean result = target.hasAccess(userKey, containerRequest);
8479

8580
assertThat(result, is(true));
8681
}
@@ -89,7 +84,6 @@ public void hasAccess_containerRequest_grantAccessForSysAdmin() {
8984
public void hasAccess_containerRequest_grantAccessForSpaceAdmin() {
9085
when(userManager.isAdmin(userKey)).thenReturn(false);
9186
when(userManager.isSystemAdmin(userKey)).thenReturn(false);
92-
when(userProfile.getUserKey()).thenReturn(userKey);
9387
UriInfo mockUriInfo = mock(UriInfo.class);
9488
when(containerRequest.getUriInfo()).thenReturn(mockUriInfo);
9589
when(mockUriInfo.getQueryParameters()).thenReturn(map);
@@ -98,7 +92,7 @@ public void hasAccess_containerRequest_grantAccessForSpaceAdmin() {
9892
when(userAccessor.getExistingUserByKey(userKey)).thenReturn(user);
9993
when(permissionManager.hasPermission(any(User.class), eq(Permission.ADMINISTER), eq(space))).thenReturn(true);
10094

101-
boolean result = target.hasAccess(userProfile, containerRequest);
95+
boolean result = target.hasAccess(userKey, containerRequest);
10296

10397
assertThat(result, is(true));
10498
}
@@ -107,7 +101,6 @@ public void hasAccess_containerRequest_grantAccessForSpaceAdmin() {
107101
public void hasAccess_containerRequest_notGrantAccessForNonSpaceAdmin() {
108102
when(userManager.isAdmin(userKey)).thenReturn(false);
109103
when(userManager.isSystemAdmin(userKey)).thenReturn(false);
110-
when(userProfile.getUserKey()).thenReturn(userKey);
111104
UriInfo mockUriInfo = mock(UriInfo.class);
112105
when(containerRequest.getUriInfo()).thenReturn(mockUriInfo);
113106
when(mockUriInfo.getQueryParameters()).thenReturn(map);
@@ -116,7 +109,7 @@ public void hasAccess_containerRequest_notGrantAccessForNonSpaceAdmin() {
116109
when(userAccessor.getExistingUserByKey(userKey)).thenReturn(user);
117110
when(permissionManager.hasPermission(any(User.class), eq(Permission.ADMINISTER), eq(space))).thenReturn(false);
118111

119-
boolean result = target.hasAccess(userProfile, containerRequest);
112+
boolean result = target.hasAccess(userKey, containerRequest);
120113

121114
assertThat(result, is(false));
122115
}
@@ -125,24 +118,22 @@ public void hasAccess_containerRequest_notGrantAccessForNonSpaceAdmin() {
125118
public void hasAccess_containerRequest_notGrantAccessForNonExistingSpace() {
126119
when(userManager.isAdmin(userKey)).thenReturn(false);
127120
when(userManager.isSystemAdmin(userKey)).thenReturn(false);
128-
when(userProfile.getUserKey()).thenReturn(userKey);
129121
UriInfo mockUriInfo = mock(UriInfo.class);
130122
when(containerRequest.getUriInfo()).thenReturn(mockUriInfo);
131123
when(mockUriInfo.getQueryParameters()).thenReturn(map);
132124
when(map.getFirst("key")).thenReturn(SPACE_KEY);
133125
when(spaceManager.getSpace(SPACE_KEY)).thenReturn(null);
134126

135-
boolean result = target.hasAccess(userProfile, containerRequest);
127+
boolean result = target.hasAccess(userKey, containerRequest);
136128

137129
assertThat(result, is(false));
138130
}
139131

140132
@Test
141133
public void hasAccess_servletRequest_grantAccessForAdmin() {
142134
when(userManager.isAdmin(userKey)).thenReturn(true);
143-
when(userProfile.getUserKey()).thenReturn(userKey);
144135

145-
boolean result = target.hasAccess(userProfile, httpServletRequest);
136+
boolean result = target.hasAccess(userKey, httpServletRequest);
146137

147138
assertThat(result, is(true));
148139
}
@@ -151,9 +142,8 @@ public void hasAccess_servletRequest_grantAccessForAdmin() {
151142
public void hasAccess_servletRequest_grantAccessForSysAdmin() {
152143
when(userManager.isAdmin(userKey)).thenReturn(false);
153144
when(userManager.isSystemAdmin(userKey)).thenReturn(true);
154-
when(userProfile.getUserKey()).thenReturn(userKey);
155145

156-
boolean result = target.hasAccess(userProfile, httpServletRequest);
146+
boolean result = target.hasAccess(userKey, httpServletRequest);
157147

158148
assertThat(result, is(true));
159149
}
@@ -162,13 +152,12 @@ public void hasAccess_servletRequest_grantAccessForSysAdmin() {
162152
public void hasAccess_servletRequest_grantAccessForSpaceAdmin() {
163153
when(userManager.isAdmin(userKey)).thenReturn(false);
164154
when(userManager.isSystemAdmin(userKey)).thenReturn(false);
165-
when(userProfile.getUserKey()).thenReturn(userKey);
166155
when(httpServletRequest.getParameter("key")).thenReturn(SPACE_KEY);
167156
when(spaceManager.getSpace(SPACE_KEY)).thenReturn(space);
168157
when(userAccessor.getExistingUserByKey(userKey)).thenReturn(user);
169158
when(permissionManager.hasPermission(any(User.class), eq(Permission.ADMINISTER), eq(space))).thenReturn(true);
170159

171-
boolean result = target.hasAccess(userProfile, httpServletRequest);
160+
boolean result = target.hasAccess(userKey, httpServletRequest);
172161

173162
assertThat(result, is(true));
174163
}
@@ -177,13 +166,12 @@ public void hasAccess_servletRequest_grantAccessForSpaceAdmin() {
177166
public void hasAccess_servletRequest_notGrantAccessForNonSpaceAdmin() {
178167
when(userManager.isAdmin(userKey)).thenReturn(false);
179168
when(userManager.isSystemAdmin(userKey)).thenReturn(false);
180-
when(userProfile.getUserKey()).thenReturn(userKey);
181169
when(httpServletRequest.getParameter("key")).thenReturn(SPACE_KEY);
182170
when(spaceManager.getSpace(SPACE_KEY)).thenReturn(space);
183171
when(userAccessor.getExistingUserByKey(userKey)).thenReturn(user);
184172
when(permissionManager.hasPermission(any(User.class), eq(Permission.ADMINISTER), eq(space))).thenReturn(false);
185173

186-
boolean result = target.hasAccess(userProfile, httpServletRequest);
174+
boolean result = target.hasAccess(userKey, httpServletRequest);
187175

188176
assertThat(result, is(false));
189177
}
@@ -192,11 +180,10 @@ public void hasAccess_servletRequest_notGrantAccessForNonSpaceAdmin() {
192180
public void hasAccess_servletRequest_notGrantAccessForNonExistingSpace() {
193181
when(userManager.isAdmin(userKey)).thenReturn(false);
194182
when(userManager.isSystemAdmin(userKey)).thenReturn(false);
195-
when(userProfile.getUserKey()).thenReturn(userKey);
196183
when(httpServletRequest.getParameter("key")).thenReturn(SPACE_KEY);
197184
when(spaceManager.getSpace(SPACE_KEY)).thenReturn(null);
198185

199-
boolean result = target.hasAccess(userProfile, httpServletRequest);
186+
boolean result = target.hasAccess(userKey, httpServletRequest);
200187

201188
assertThat(result, is(false));
202189
}
@@ -205,7 +192,6 @@ public void hasAccess_servletRequest_notGrantAccessForNonExistingSpace() {
205192
public void hasAccess_servletRequest_grantAccessForSpaceKeyInSession() {
206193
when(userManager.isAdmin(userKey)).thenReturn(false);
207194
when(userManager.isSystemAdmin(userKey)).thenReturn(false);
208-
when(userProfile.getUserKey()).thenReturn(userKey);
209195
when(httpServletRequest.getParameter("key")).thenReturn(null);
210196
when(httpServletRequest.getSession()).thenReturn(session);
211197
when(session.getAttribute(FROM_SPACE_ATTRIBUTE_KEY)).thenReturn(true);
@@ -214,7 +200,7 @@ public void hasAccess_servletRequest_grantAccessForSpaceKeyInSession() {
214200
when(userAccessor.getExistingUserByKey(userKey)).thenReturn(user);
215201
when(permissionManager.hasPermission(any(User.class), eq(Permission.ADMINISTER), eq(space))).thenReturn(true);
216202

217-
boolean result = target.hasAccess(userProfile, httpServletRequest);
203+
boolean result = target.hasAccess(userKey, httpServletRequest);
218204

219205
assertThat(result, is(true));
220206
}

‎jira-slack-server-integration/jira-slack-server-integration-plugin/src/main/java/com/atlassian/jira/plugins/slack/spi/impl/JiraSlackLinkAccessManager.java

+11-11
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@
77
import com.atlassian.jira.user.ApplicationUser;
88
import com.atlassian.plugins.slack.spi.SlackLinkAccessManager;
99
import com.atlassian.plugins.slack.spi.impl.AbstractSlackLinkAccessManager;
10+
import com.atlassian.sal.api.user.UserKey;
1011
import com.atlassian.sal.api.user.UserManager;
11-
import com.atlassian.sal.api.user.UserProfile;
1212
import org.springframework.beans.factory.annotation.Autowired;
1313
import org.springframework.beans.factory.annotation.Qualifier;
1414
import org.springframework.stereotype.Component;
@@ -40,29 +40,29 @@ public JiraSlackLinkAccessManager(
4040
this.jiraUserManager = jiraUserManager;
4141
}
4242

43-
private boolean hasAccess(final UserProfile userProfile, final Optional<Project> project) {
43+
private boolean hasAccess(final UserKey userKey, final Optional<Project> project) {
4444
return project
45-
.filter(project1 -> isProjectAdmin(getUserByProfile(userProfile), project1))
45+
.filter(project1 -> isProjectAdmin(getUserByProfile(userKey), project1))
4646
.isPresent();
4747

4848
}
4949

5050
@Override
51-
public boolean hasAccess(final UserProfile userProfile, final ContainerRequestContext request) {
52-
if (super.hasAccess(userProfile, request)) {
51+
public boolean hasAccess(final UserKey userKey, final ContainerRequestContext request) {
52+
if (super.hasAccess(userKey, request)) {
5353
return true;
5454
}
5555

56-
return hasAccess(userProfile, getProject(request));
56+
return hasAccess(userKey, getProject(request));
5757
}
5858

5959
@Override
60-
public boolean hasAccess(final UserProfile userProfile, final HttpServletRequest request) {
61-
if (super.hasAccess(userProfile, request)) {
60+
public boolean hasAccess(final UserKey userKey, final HttpServletRequest request) {
61+
if (super.hasAccess(userKey, request)) {
6262
return true;
6363
}
6464

65-
return hasAccess(userProfile, getProject(request));
65+
return hasAccess(userKey, getProject(request));
6666
}
6767

6868
private Optional<Project> getProject(final ContainerRequestContext request) {
@@ -92,7 +92,7 @@ private boolean isProjectAdmin(final ApplicationUser user, final Project project
9292
return permissionManager.hasPermission(ProjectPermissions.ADMINISTER_PROJECTS, project, user);
9393
}
9494

95-
private ApplicationUser getUserByProfile(final UserProfile userProfile) {
96-
return jiraUserManager.getUserByKeyEvenWhenUnknown(userProfile.getUserKey().getStringValue());
95+
private ApplicationUser getUserByProfile(final UserKey userKey) {
96+
return jiraUserManager.getUserByKeyEvenWhenUnknown(userKey.getStringValue());
9797
}
9898
}

‎jira-slack-server-integration/jira-slack-server-integration-plugin/src/main/java/com/atlassian/jira/plugins/slack/web/rest/IssuePanelResource.java

+7-5
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import com.atlassian.plugins.slack.rest.model.LimitedSlackLinkDto;
1919
import com.atlassian.plugins.slack.spi.SlackLinkAccessManager;
2020
import com.atlassian.plugins.slack.user.SlackUserManager;
21+
import com.atlassian.sal.api.user.UserKey;
2122
import com.atlassian.sal.api.user.UserManager;
2223
import com.fasterxml.jackson.annotation.JsonProperty;
2324
import org.springframework.beans.factory.annotation.Autowired;
@@ -85,7 +86,7 @@ public IssuePanelResource(final PluginConfigurationManager pluginConfigurationMa
8586
@Path("/hide")
8687
@Consumes({MediaType.APPLICATION_JSON})
8788
public Response hidePanel(@Context HttpServletRequest httpRequest) {
88-
if (!slackLinkAccessManager.hasAccess(userManager.getRemoteUser(), httpRequest)) {
89+
if (!slackLinkAccessManager.hasAccess(userManager.getRemoteUserKey(), httpRequest)) {
8990
return Response.status(Response.Status.FORBIDDEN).build();
9091
}
9192

@@ -105,16 +106,17 @@ public Response showPanel() {
105106
@Path("/data/{issueId}")
106107
@Consumes({MediaType.APPLICATION_JSON})
107108
public Response panelData(@PathParam("issueId") String issueId) {
108-
if (userManager.getRemoteUser() == null) {
109+
final UserKey userKey = userManager.getRemoteUserKey();
110+
if (userKey == null) {
109111
return Response.status(Response.Status.NOT_FOUND).build();
110112
}
111-
final ApplicationUser applicationUser = jiraUserManager.getUserByKey(
112-
userManager.getRemoteUser().getUserKey().getStringValue());
113-
final Issue issue = issueManager.getIssueByKeyIgnoreCase(issueId);
114113

114+
final Issue issue = issueManager.getIssueByKeyIgnoreCase(issueId);
115115
if (issue == null) {
116116
return Response.status(Response.Status.NOT_FOUND).build();
117117
}
118+
119+
final ApplicationUser applicationUser = jiraUserManager.getUserByKey(userKey.getStringValue());
118120
if (!permissionManager.hasPermission(ProjectPermissions.BROWSE_PROJECTS, issue, applicationUser)) {
119121
return Response.status(Response.Status.FORBIDDEN).build();
120122
}

0 commit comments

Comments
 (0)