-
Notifications
You must be signed in to change notification settings - Fork 29
[PLUGIN-1872] Added GrantType and Client Authentication for the HttpOauth2 #187
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
[PLUGIN-1872] Added GrantType and Client Authentication for the HttpOauth2 #187
Conversation
7a2c650 to
d549089
Compare
42e5466 to
2143566
Compare
itsankit-google
left a comment
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.
How does UI looks like for existing pipelines?
I think we should handle the case where oauth2GrantType is null for older pipelines.
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.
please add unit tests
Add screenshots & test evidences in PR description.
|
please create a JIRA and add it to PR title |
ea40601 to
571bf36
Compare
itsankit-google
left a comment
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.
May I know the reference documentation used to implement this authentication flow?
AFAIK, in client_credentials flow there is only one request to get access token from oauth server:
Request
curl -X POST "https://auth.example.com/oauth/token" \
-H "Content-Type: application/x-www-form-urlencoded" \
-d "grant_type=client_credentials" \
-d "client_id=your-client-id" \
-d "client_secret=your-client-secret" \
-d "scope=read:data"
Response
{
"access_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9...",
"token_type": "Bearer",
"expires_in": 3600
}
I am unable to understand & find oauth2ClientAuthentication method used in this PR.
Hi Ankit, It was addressed in Design doc also for one of your comment. Also we got this from customer side, that in some cases they are passing details in query params and in some cases as part of the body. Postman interface also has both the options. |
src/test/java/io/cdap/plugin/http/source/common/HttpBatchSourceConfigTest.java
Outdated
Show resolved
Hide resolved
src/test/java/io/cdap/plugin/http/source/common/HttpBatchSourceConfigTest.java
Outdated
Show resolved
Hide resolved
itsankit-google
left a comment
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.
At some places we are using OAuth2 and in some places we are using OAuth interchangeably. Please ensure we either use OAuth2 or OAuth so that it does not confuse people looking at the code.
src/main/java/io/cdap/plugin/http/sink/batch/HTTPSinkConfig.java
Outdated
Show resolved
Hide resolved
itsankit-google
left a comment
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.
please fix build failures.
Success. It failed sometime, due to connection timeout.. |
dead76f to
e6c3cfa
Compare
e6c3cfa to
7ca628d
Compare
https://cdap.atlassian.net/browse/PLUGIN-1872
Update UI:
1. Grant Type: Refresh Token


2. Grant Type: Client Credentials and Client Authentication: Body


3. Grant Type: Client Credentials and Client Authentication: Request parameter


Older UI vs When pipeline upgraded to latest build
Older Pipeline:

Older pipeline upgraded to latest UI:

