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

MethodsClientImpl oauthV2Access: redirect_uri should be optional #474

Closed
natevaughan opened this issue May 27, 2020 · 3 comments · Fixed by #475
Closed

MethodsClientImpl oauthV2Access: redirect_uri should be optional #474

natevaughan opened this issue May 27, 2020 · 3 comments · Fixed by #475
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented project:slack-api-client project:slack-api-client
Milestone

Comments

@natevaughan
Copy link

natevaughan commented May 27, 2020

Issue Type

[ x ] Bug
[ ] Enhancement / Feature request
[ ] Question
[ ] Documentation

Description

Hi @seratch !

I'm seeing this error when sending a null redirect_uri in OauthV2AccessRequestBuilder:

java.lang.IllegalArgumentException: Parameter specified as non-null is null: method okhttp3.FormBody$Builder.add, parameter value

It seems OKHTTP's form builder specifies that all param values must be non-null.

I think simply wrapping this line in MethodsClientImpl with a null check should suffice:

if (req.getRedirectUri() != null) {
    form.add("redirect_uri", req.getRedirectUri());
}

Filing this as a bug due to a mismatch between Slack's Oauth V2 api documentation, which specifies redirect_uri as optional.

I should mention that I previously was using a hand-cooked Slack API client and Oauth worked fine without redirect_uri.

The issue is reproducible in:

  • Module Version: 1.0.7
  • JDK Version: Zulu 1.8.0_232

The steps to reproduce are:

  1. Use the following sample (Kotlin) code with a valid code, clientId, and clientSecret (do not set redirect_uri):
    val builder = OAuthV2AccessRequest.builder()
        .code(code)				
        .clientId(clientId)
        .clientSecret(clientSecret)

    val response = methods.oauthV2Access(builder.build())
  1. The expected result is the request should succeed.

  2. The actual result is the following exception from OKHTTP:

java.lang.IllegalArgumentException: Parameter specified as non-null is null: method okhttp3.FormBody$Builder.add, parameter value
        at okhttp3.FormBody$Builder.add(FormBody.kt) ~[okhttp-4.6.0.jar!/:na]
        at com.slack.api.methods.impl.MethodsClientImpl.oauthV2Access(MethodsClientImpl.java:1638) ~[slack-api-client-1.0.7.jar!/:1.0.7]

Thank you for your amazing work!

  • [ x ] I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • [ x ] I've read and agree to the Code of Conduct.
  • [ x ] I've searched for any related issues and avoided creating a duplicate issue.
@natevaughan natevaughan changed the title MethodsClientImpl oauthV2Access: redirect_uri should be optional MethodsClientImpl oauthV2Access: redirect_uri should be optional May 27, 2020
@seratch seratch added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented project:slack-api-client project:slack-api-client labels May 27, 2020
@seratch seratch added this to the 1.0.x milestone May 27, 2020
@seratch
Copy link
Member

seratch commented May 27, 2020

@natevaughan Thanks for reporting this. You're right. I will be fixing this very soon.

seratch added a commit to seratch/java-slack-sdk that referenced this issue May 27, 2020
seratch added a commit that referenced this issue May 27, 2020
Fix #474 by making redirect_uri optional
@seratch
Copy link
Member

seratch commented May 27, 2020

Thanks again. I will be releasing a new patch version today (+09:00)

seratch added a commit that referenced this issue May 28, 2020
* [slack-api-model] Add missing fields in objects (confirm.style in blocks, user.is_invited_user: boolean, message.hidden, Slack post related fields in file objects) - thanks @seratch
* [slack-api-client etc] #466 #462 Calls API support - thanks @seratch
* [slack-api-client] #475 #474 Make redirect_uri for oauth.access / oauth.v2.access optional - thanks @natevaughan @seratch
* [slack-api-client] #476 Bump dependencies (okhttp, micronaut, tyrus-standalone-client) - thanks @seratch
* [slack-api-client] #459 Adding ping message and pong event to RTM client - thanks @gaspardpetit
* [slack-api-client] #451 Add support and check for the event subtypes in RTM client - thanks @gaspardpetit
* [bolt] #455 Improve OAuth flow module to consider the cases where team is missing in oauth.v2.access responses - thanks @seratch
* [bolt] #476 Bump dependencies (aws-java-sdk-s3) - thanks @seratch
@natevaughan
Copy link
Author

Thank you @seratch, amazing work at amazing speed!

emanguy pushed a commit to emanguy/java-slack-sdk that referenced this issue Jun 22, 2020
emanguy pushed a commit to emanguy/java-slack-sdk that referenced this issue Jun 22, 2020
* [slack-api-model] Add missing fields in objects (confirm.style in blocks, user.is_invited_user: boolean, message.hidden, Slack post related fields in file objects) - thanks @seratch
* [slack-api-client etc] slackapi#466 slackapi#462 Calls API support - thanks @seratch
* [slack-api-client] slackapi#475 slackapi#474 Make redirect_uri for oauth.access / oauth.v2.access optional - thanks @natevaughan @seratch
* [slack-api-client] slackapi#476 Bump dependencies (okhttp, micronaut, tyrus-standalone-client) - thanks @seratch
* [slack-api-client] slackapi#459 Adding ping message and pong event to RTM client - thanks @gaspardpetit
* [slack-api-client] slackapi#451 Add support and check for the event subtypes in RTM client - thanks @gaspardpetit
* [bolt] slackapi#455 Improve OAuth flow module to consider the cases where team is missing in oauth.v2.access responses - thanks @seratch
* [bolt] slackapi#476 Bump dependencies (aws-java-sdk-s3) - thanks @seratch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented project:slack-api-client project:slack-api-client
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants