Skip to content

Commit be692d0

Browse files
seratchseratch
authored
Fix #913 user-scope only OAuth does not work since v1.10 (#914)
1 parent 004efab commit be692d0

File tree

8 files changed

+196
-31
lines changed

8 files changed

+196
-31
lines changed

‎bolt-helidon/src/test/java/test_locally/MainTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ public void oauthStart() throws Exception {
7575
assertEquals(302, conn.getResponseCode());
7676
String authorizationUrl = conn.getHeaderField("Location");
7777
assertTrue(authorizationUrl, authorizationUrl.startsWith(
78-
"https://slack.com/oauth/v2/authorize?client_id=123.123&scope=app_mention:read,chat:write,commands&user_scope=&state="));
78+
"https://slack.com/oauth/v2/authorize?client_id=123.123&scope=app_mention%3Aread%2Cchat%3Awrite%2Ccommands&user_scope=&state="));
7979
}
8080

8181
}

‎bolt-http4k/src/test/java/test_locally/Http4kSlackAppTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ public void oauth() {
154154
Response installResponse = oauthSlackApp.invoke(installRequest);
155155
assertThat(installResponse.getStatus().getCode(), equalTo(302));
156156
assertTrue(installResponse.header("Location").startsWith(
157-
"https://slack.com/oauth/v2/authorize?client_id=111.222&scope=commands,chat:write&user_scope=&state="));
157+
"https://slack.com/oauth/v2/authorize?client_id=111.222&scope=commands%2Cchat%3Awrite&user_scope=&state="));
158158

159159
assertThat(installResponse.getStatus().getCode(), equalTo(302));
160160

‎bolt/src/main/java/com/slack/api/bolt/App.java‎

Lines changed: 23 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -38,16 +38,14 @@
3838
import lombok.extern.slf4j.Slf4j;
3939

4040
import java.io.IOException;
41-
import java.io.UnsupportedEncodingException;
42-
import java.net.URLEncoder;
43-
import java.nio.charset.StandardCharsets;
4441
import java.util.*;
4542
import java.util.concurrent.ExecutorService;
4643
import java.util.concurrent.atomic.AtomicBoolean;
4744
import java.util.regex.Pattern;
4845

4946
import static com.slack.api.bolt.util.EventsApiPayloadParser.buildEventPayload;
5047
import static com.slack.api.bolt.util.EventsApiPayloadParser.getEventTypeAndSubtype;
48+
import static com.slack.api.bolt.util.UrlEncodingOps.urlEncode;
5149

5250
/**
5351
* A Slack App instance.
@@ -351,15 +349,26 @@ public String buildAuthorizeUrl(String state) {
351349
*/
352350
public String buildAuthorizeUrl(String state, String nonce) {
353351
AppConfig config = config();
354-
355-
if (config.getClientId() == null
356-
// OpenID Connect does not use require the bot scopes
357-
|| (!config.isOpenIDConnectEnabled() && config.getScope() == null)
358-
|| (config.isStateValidationEnabled() && state == null)) {
352+
if (config.getClientId() == null) {
353+
log.warn("To enable th Slack OAuth flow, set config#clientId and so on properly. " +
354+
"Refer to https://api.slack.com/authentication for more information.");
355+
return null;
356+
}
357+
if (config.isStateValidationEnabled() && state == null) {
358+
log.warn("Your OAuthStateService might generate a null value for some reason.");
359+
return null;
360+
}
361+
if (config.isOpenIDConnectEnabled() && config.getUserScope() == null) {
362+
log.warn("For OpenID Connect authorization, set config#userScope properly. " +
363+
"Refer to https://api.slack.com/authentication/sign-in-with-slack for more information.");
359364
return null;
360365
}
366+
if (config.isOpenIDConnectEnabled() && config.getScope() != null) {
367+
// The config#scope value will be ignored
368+
log.warn("For OpenID Connect authorization, use config#userScope() instead of #scope()");
369+
}
361370

362-
String scope = config.getScope() == null ? "" : config.getScope();
371+
String scope = config.getScope() == null ? "" : urlEncode(config.getScope());
363372
String redirectUriParam = redirectUriQueryParam(appConfig);
364373

365374
if (config.isClassicAppPermissionsEnabled()) {
@@ -373,14 +382,14 @@ public String buildAuthorizeUrl(String state, String nonce) {
373382
return "https://slack.com/openid/connect/authorize" +
374383
"?client_id=" + config.getClientId() +
375384
"&response_type=code" +
376-
"&scope=" + (config.getUserScope() != null ? config.getUserScope() : scope) +
385+
"&scope=" + (config.getUserScope() != null ? urlEncode(config.getUserScope()) : scope) +
377386
"&state=" + state +
378387
redirectUriParam +
379388
// The nonce parameter is an optional one in the OpenID Connect Spec
380389
// https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest
381390
(nonce != null ? "&nonce=" + nonce : "");
382391
} else {
383-
String userScope = config.getUserScope() == null ? "" : config.getUserScope();
392+
String userScope = config.getUserScope() == null ? "" : urlEncode(config.getUserScope());
384393
return "https://slack.com/oauth/v2/authorize" +
385394
"?client_id=" + config.getClientId() +
386395
"&scope=" + scope +
@@ -403,16 +412,8 @@ private String redirectUriQueryParam(AppConfig appConfig) {
403412
return "";
404413
}
405414

406-
try {
407-
String urlEncodedRedirectUri = URLEncoder.encode(
408-
appConfig.getRedirectUri(),
409-
StandardCharsets.UTF_8.name()
410-
);
411-
412-
return String.format("&redirect_uri=%s", urlEncodedRedirectUri);
413-
} catch (UnsupportedEncodingException e) {
414-
return "";
415-
}
415+
String urlEncodedRedirectUri = urlEncode(appConfig.getRedirectUri());
416+
return String.format("&redirect_uri=%s", urlEncodedRedirectUri);
416417
}
417418

418419
// -------------------------------------
@@ -1023,7 +1024,7 @@ protected Response runHandler(Request slackRequest) throws IOException, SlackApi
10231024
String nonce = openIDConnectNonceService.issueNewNonce(slackRequest, response);
10241025
String authorizeUrl = buildAuthorizeUrl(state, nonce);
10251026
if (authorizeUrl == null) {
1026-
log.error("App#buildAuthorizeUrl(String) returned null due to some missing settings");
1027+
log.error("App#buildAuthorizeUrl(String) returned null due to missing/invalid settings");
10271028
if (config().getOauthCancellationUrl() == null) {
10281029
response.setContentType("text/html; charset=utf-8");
10291030
String installPath = config().getOauthInstallRequestURI();

‎bolt/src/main/java/com/slack/api/bolt/util/UrlEncodingOps.java‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
package com.slack.api.bolt.util;
2+
3+
import lombok.extern.slf4j.Slf4j;
4+
5+
import java.io.UnsupportedEncodingException;
6+
import java.net.URLEncoder;
7+
import java.nio.charset.StandardCharsets;
8+
9+
@Slf4j
10+
public class UrlEncodingOps {
11+
12+
private UrlEncodingOps() {
13+
}
14+
15+
public static String urlEncode(String value) {
16+
if (value == null) {
17+
return null;
18+
}
19+
try {
20+
return URLEncoder.encode(value, StandardCharsets.UTF_8.name());
21+
} catch (UnsupportedEncodingException e) {
22+
log.warn("Unexpectedly UnsupportedEncodingException was thrown while URL-encoding a string value.");
23+
return value;
24+
}
25+
}
26+
}

‎bolt/src/test/java/test_locally/AppTest.java‎

Lines changed: 112 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ public void buildAuthorizeUrl_v1() {
5555
.build();
5656
App app = new App(config);
5757
String url = app.buildAuthorizeUrl("state-value");
58-
assertEquals("https://slack.com/oauth/authorize?client_id=123&scope=commands,chat:write&state=state-value", url);
58+
assertEquals("https://slack.com/oauth/authorize?client_id=123&scope=commands%2Cchat%3Awrite&state=state-value", url);
5959
}
6060

6161
@Test
@@ -69,7 +69,7 @@ public void buildAuthorizeUrl_v1_with_redirect_uri() {
6969
.build();
7070
App app = new App(config);
7171
String url = app.buildAuthorizeUrl("state-value");
72-
assertEquals("https://slack.com/oauth/authorize?client_id=123&scope=commands,chat:write&state=state-value&redirect_uri=https%3A%2F%2Fmy.app%2Foauth%2Fcallback", url);
72+
assertEquals("https://slack.com/oauth/authorize?client_id=123&scope=commands%2Cchat%3Awrite&state=state-value&redirect_uri=https%3A%2F%2Fmy.app%2Foauth%2Fcallback", url);
7373
}
7474

7575
@Test
@@ -82,7 +82,7 @@ public void buildAuthorizeUrl_v2() {
8282
.build();
8383
App app = new App(config);
8484
String url = app.buildAuthorizeUrl("state-value");
85-
assertEquals("https://slack.com/oauth/v2/authorize?client_id=123&scope=commands,chat:write&user_scope=search:read&state=state-value", url);
85+
assertEquals("https://slack.com/oauth/v2/authorize?client_id=123&scope=commands%2Cchat%3Awrite&user_scope=search%3Aread&state=state-value", url);
8686
}
8787

8888
@Test
@@ -96,7 +96,7 @@ public void buildAuthorizeUrl_v2_with_redirect_uri() {
9696
.build();
9797
App app = new App(config);
9898
String url = app.buildAuthorizeUrl("state-value");
99-
assertEquals("https://slack.com/oauth/v2/authorize?client_id=123&scope=commands,chat:write&user_scope=search:read&state=state-value&redirect_uri=https%3A%2F%2Fmy.app%2Foauth%2Fcallback", url);
99+
assertEquals("https://slack.com/oauth/v2/authorize?client_id=123&scope=commands%2Cchat%3Awrite&user_scope=search%3Aread&state=state-value&redirect_uri=https%3A%2F%2Fmy.app%2Foauth%2Fcallback", url);
100100
}
101101

102102
@Test
@@ -106,6 +106,114 @@ public void buildAuthorizeUrl_null() {
106106
assertNull(url);
107107
}
108108

109+
@Test
110+
public void buildAuthorizeUrl_bot_scope() {
111+
App app = new App(AppConfig.builder()
112+
.signingSecret("secret")
113+
.clientId("111.222")
114+
.clientSecret("secret")
115+
.scope("commands,chat:write")
116+
.build());
117+
String generatedUrl = app.buildAuthorizeUrl("state-value");
118+
assertEquals("https://slack.com/oauth/v2/authorize" +
119+
"?client_id=111.222&scope=commands%2Cchat%3Awrite&user_scope=&state=state-value", generatedUrl);
120+
}
121+
122+
@Test
123+
public void buildAuthorizeUrl_bot_and_user_scope() {
124+
App app = new App(AppConfig.builder()
125+
.signingSecret("secret")
126+
.clientId("111.222")
127+
.clientSecret("secret")
128+
.scope("commands,chat:write")
129+
.userScope("search:read,chat:write")
130+
.build());
131+
String generatedUrl = app.buildAuthorizeUrl("state-value");
132+
assertEquals("https://slack.com/oauth/v2/authorize" +
133+
"?client_id=111.222&scope=commands%2Cchat%3Awrite&user_scope=search%3Aread%2Cchat%3Awrite&state=state-value", generatedUrl);
134+
}
135+
136+
@Test
137+
public void buildAuthorizeUrl_user_scope() {
138+
App app = new App(AppConfig.builder()
139+
.signingSecret("secret")
140+
.clientId("111.222")
141+
.clientSecret("secret")
142+
.userScope("search:read,chat:write")
143+
.build());
144+
String generatedUrl = app.buildAuthorizeUrl("state-value");
145+
assertEquals("https://slack.com/oauth/v2/authorize?client_id=111.222&scope=&user_scope=search%3Aread%2Cchat%3Awrite&state=state-value", generatedUrl);
146+
}
147+
148+
@Test
149+
public void buildAuthorizeUrl_OpenID_Connect() {
150+
App app = new App(AppConfig.builder()
151+
.openIDConnectEnabled(true)
152+
.signingSecret("secret")
153+
.clientId("111.222")
154+
.userScope("openid,email")
155+
.build());
156+
String generatedUrl = app.buildAuthorizeUrl("state-value");
157+
assertEquals("https://slack.com/openid/connect/authorize?client_id=111.222&response_type=code&scope=openid%2Cemail&state=state-value", generatedUrl);
158+
}
159+
160+
@Test
161+
public void buildAuthorizeUrl_OpenID_Connect_invalid() {
162+
App app = new App(AppConfig.builder()
163+
.openIDConnectEnabled(true)
164+
.signingSecret("secret")
165+
.clientId("111.222")
166+
.scope("openid,email")
167+
.build());
168+
String generatedUrl = app.buildAuthorizeUrl("state-value");
169+
assertNull(generatedUrl);
170+
}
171+
172+
@Test
173+
public void buildAuthorizeUrl_bot_scope_classic() {
174+
App app = new App(AppConfig.builder()
175+
.classicAppPermissionsEnabled(true)
176+
.signingSecret("secret")
177+
.clientId("111.222")
178+
.clientSecret("secret")
179+
.scope("commands,chat:write")
180+
.build());
181+
String generatedUrl = app.buildAuthorizeUrl("state-value");
182+
// use_scope= does not exist in the Slack OAuth v1
183+
assertEquals("https://slack.com/oauth/authorize" +
184+
"?client_id=111.222&scope=commands%2Cchat%3Awrite&state=state-value", generatedUrl);
185+
}
186+
187+
@Test
188+
public void buildAuthorizeUrl_bot_and_user_scope_classic() {
189+
App app = new App(AppConfig.builder()
190+
.classicAppPermissionsEnabled(true)
191+
.signingSecret("secret")
192+
.clientId("111.222")
193+
.clientSecret("secret")
194+
.scope("commands,chat:write")
195+
.userScope("search:read,chat:write")
196+
.build());
197+
String generatedUrl = app.buildAuthorizeUrl("state-value");
198+
// use_scope= does not exist in the Slack OAuth v1
199+
assertEquals("https://slack.com/oauth/authorize" +
200+
"?client_id=111.222&scope=commands%2Cchat%3Awrite&state=state-value", generatedUrl);
201+
}
202+
203+
@Test
204+
public void buildAuthorizeUrl_user_scope_classic() {
205+
App app = new App(AppConfig.builder()
206+
.classicAppPermissionsEnabled(true)
207+
.signingSecret("secret")
208+
.clientId("111.222")
209+
.clientSecret("secret")
210+
.userScope("search:read,chat:write")
211+
.build());
212+
String generatedUrl = app.buildAuthorizeUrl("state-value");
213+
// use_scope= does not exist in the Slack OAuth v1
214+
assertEquals("https://slack.com/oauth/authorize?client_id=111.222&scope=&state=state-value", generatedUrl);
215+
}
216+
109217
@Test
110218
public void status() {
111219
App app = new App(AppConfig.builder()

‎bolt/src/test/java/test_locally/app/OAuthStartTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ public String issueNewState(Request request, Response response) {
4848
"</head>\n" +
4949
"<body>\n" +
5050
"<h2>Slack App Installation</h2>\n" +
51-
"<p><a href=\"https://slack.com/oauth/v2/authorize?client_id=111.222&scope=commands,chat:write&user_scope=&state=generated-state-value\"><img alt=\"\"Add to Slack\"\" height=\"40\" width=\"139\" src=\"https://platform.slack-edge.com/img/add_to_slack.png\" srcset=\"https://platform.slack-edge.com/img/add_to_slack.png 1x, https://platform.slack-edge.com/img/[email protected] 2x\" /></a></p>\n" +
51+
"<p><a href=\"https://slack.com/oauth/v2/authorize?client_id=111.222&scope=commands%2Cchat%3Awrite&user_scope=&state=generated-state-value\"><img alt=\"\"Add to Slack\"\" height=\"40\" width=\"139\" src=\"https://platform.slack-edge.com/img/add_to_slack.png\" srcset=\"https://platform.slack-edge.com/img/add_to_slack.png 1x, https://platform.slack-edge.com/img/[email protected] 2x\" /></a></p>\n" +
5252
"</body>\n" +
5353
"</html>", response.getBody());
5454
}
@@ -78,7 +78,7 @@ public void start_state_validation_disabled() throws Exception {
7878
"</head>\n" +
7979
"<body>\n" +
8080
"<h2>Slack App Installation</h2>\n" +
81-
"<p><a href=\"https://slack.com/oauth/v2/authorize?client_id=111.222&scope=commands,chat:write&user_scope=&state=\"><img alt=\"\"Add to Slack\"\" height=\"40\" width=\"139\" src=\"https://platform.slack-edge.com/img/add_to_slack.png\" srcset=\"https://platform.slack-edge.com/img/add_to_slack.png 1x, https://platform.slack-edge.com/img/[email protected] 2x\" /></a></p>\n" +
81+
"<p><a href=\"https://slack.com/oauth/v2/authorize?client_id=111.222&scope=commands%2Cchat%3Awrite&user_scope=&state=\"><img alt=\"\"Add to Slack\"\" height=\"40\" width=\"139\" src=\"https://platform.slack-edge.com/img/add_to_slack.png\" srcset=\"https://platform.slack-edge.com/img/add_to_slack.png 1x, https://platform.slack-edge.com/img/[email protected] 2x\" /></a></p>\n" +
8282
"</body>\n" +
8383
"</html>", response.getBody());
8484
}

‎bolt/src/test/java/test_locally/app/OpenIDConnectTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ public void deleteNonceFromDatastore(String nonce) {
6666
"</head>\n" +
6767
"<body>\n" +
6868
"<h2>Slack App Installation</h2>\n" +
69-
"<p><a href=\"https://slack.com/openid/connect/authorize?client_id=111.222&response_type=code&scope=openid,email,profile&state=generated-state-value&nonce=generated-nonce-value\"><img alt=\"\"Add to Slack\"\" height=\"40\" width=\"139\" src=\"https://platform.slack-edge.com/img/add_to_slack.png\" srcset=\"https://platform.slack-edge.com/img/add_to_slack.png 1x, https://platform.slack-edge.com/img/[email protected] 2x\" /></a></p>\n" +
69+
"<p><a href=\"https://slack.com/openid/connect/authorize?client_id=111.222&response_type=code&scope=openid%2Cemail%2Cprofile&state=generated-state-value&nonce=generated-nonce-value\"><img alt=\"\"Add to Slack\"\" height=\"40\" width=\"139\" src=\"https://platform.slack-edge.com/img/add_to_slack.png\" srcset=\"https://platform.slack-edge.com/img/add_to_slack.png 1x, https://platform.slack-edge.com/img/[email protected] 2x\" /></a></p>\n" +
7070
"</body>\n" +
7171
"</html>";
7272

‎bolt/src/test/java/test_locally/util/UrlEncodingOpsTest.java‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
package test_locally.util;
2+
3+
import org.junit.Test;
4+
5+
import static com.slack.api.bolt.util.UrlEncodingOps.urlEncode;
6+
import static org.hamcrest.CoreMatchers.is;
7+
import static org.hamcrest.CoreMatchers.nullValue;
8+
import static org.hamcrest.MatcherAssert.assertThat;
9+
10+
public class UrlEncodingOpsTest {
11+
12+
@Test
13+
public void testUrlEncode() {
14+
String result = urlEncode("foo,bar,baz");
15+
assertThat(result, is("foo%2Cbar%2Cbaz"));
16+
}
17+
18+
@Test
19+
public void testUrlEncode_CJK() {
20+
String result = urlEncode("日本語");
21+
assertThat(result, is("%E6%97%A5%E6%9C%AC%E8%AA%9E"));
22+
}
23+
24+
@Test
25+
public void testUrlEncode_null() {
26+
String result = urlEncode(null);
27+
assertThat(result, is(nullValue()));
28+
}
29+
30+
}

0 commit comments

Comments
 (0)