Skip to content

[PLUGIN-692] Add Service Account auth to http - #80

Merged
sumengwang merged 16 commits into
developfrom
PLUGIN-692
Apr 14, 2022
Merged

[PLUGIN-692] Add Service Account auth to http#80
sumengwang merged 16 commits into
developfrom
PLUGIN-692

Conversation

@sumengwang

@sumengwangsumengwang commented Mar 25, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

[PLUGIN-692] Add Service Account auth to http

Description

Added Service Account auth to http batch source. Currently the unit tests won't pass

Links

PLUGIN-692

Screenshots

image

tivv
tivv reviewed Mar 25, 2022
View reviewed changes
Comment threadsrc/main/java/io/cdap/plugin/http/source/common/BaseHttpSourceConfig.java
tivv
tivv reviewed Mar 29, 2022
View reviewed changes
Comment threadsrc/main/java/io/cdap/plugin/http/source/batch/HttpBatchSourceConfig.java Outdated
Comment threadsrc/main/java/io/cdap/plugin/http/source/common/BaseHttpSourceConfig.java Outdated
Comment threadsrc/main/java/io/cdap/plugin/http/source/common/BaseHttpSourceConfig.java Outdated
Comment threadsrc/main/java/io/cdap/plugin/http/source/common/exceptions/InvalidPropertyTypeException.java Outdated
tivv
tivv reviewed Mar 30, 2022
View reviewed changes
Comment threadsrc/main/java/io/cdap/plugin/http/source/common/http/HttpClient.java
tivv
tivv reviewed Mar 30, 2022
View reviewed changes
Comment threadsrc/main/java/io/cdap/plugin/http/source/common/http/HttpClient.java Outdated
tivv
tivv reviewed Mar 31, 2022
View reviewed changes
Comment threadsrc/main/java/io/cdap/plugin/http/source/common/http/AuthType.java Outdated
tivv
tivv reviewed Mar 31, 2022
View reviewed changes
Comment threadsrc/main/java/io/cdap/plugin/http/source/common/http/AuthType.java
* @return The AuthType
*/
public static AuthType fromValue(String value) {
return Arrays.stream(AuthType.values()).filter(authtype -> authtype.getValue().equals(value))

@tivvtivvMar 31, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why not simply valueOf?

@sumengwangsumengwangMar 31, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I copied this file from the google drive plugin. I didn't really look at it into details

@tivvtivvApr 4, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please use valueOf, this looks strange

@tivvtivvApr 4, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Discussed offline

Comment threadsrc/main/java/io/cdap/plugin/http/source/common/http/OAuthUtil.java Outdated
GoogleCredentials credential;
String jwtToken = "";
if (config.isServiceAccountJson()) {
InputStream jsonInputStream = new ByteArrayInputStream(config.getServiceAccountJson().getBytes());

@tivvtivvMar 31, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ensure you pass UTF-8 otherwise it's current encoding-dependent. Also it's a good habit to put any InputStreams into try with resources block to ensure it's closed.

@sumengwangsumengwangMar 31, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The function itself throws IOException so I did not add the try block

@tivvtivvApr 4, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not try-catch, but try-with-resources

Comment threadsrc/main/java/io/cdap/plugin/http/source/common/http/OAuthUtil.java Outdated
if (config.isServiceAccountJson()) {
InputStream jsonInputStream = new ByteArrayInputStream(config.getServiceAccountJson().getBytes());
credential = GoogleCredentials.fromStream(jsonInputStream);
jwtToken = generateJwt(5000, credential);

@tivvtivvMar 31, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

How did you decide on 5 seconds? Also it's better to have it at least as a constant as it makes code more readable

@sumengwangsumengwangMar 31, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This is 5000 seconds based on the documentation. I'm not sure what time range I should choose

@tivvtivvMar 31, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Oh, I see, you convert it to millis. 5000 seconds is probably fine, but please make a constant that clearly says it's seconds in it's name

@fernstfernstMar 31, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Also 5000 seconds is an odd time. I would expect it to be aligned to an hour (3600 seconds) or a day (86400 seconds).

I also think this value should be in a configuration option for the user to configure. Some users might not feel comfortable with long living JWT tokens.

@fernst
fernst requested a review from tivvApril 4, 2022 16:59
tivv
tivv reviewed Apr 4, 2022
View reviewed changes
Comment threadpom.xml Outdated
<common.codec.version>1.12</common.codec.version>
<gson.version>2.8.5</gson.version>
<googleauth.version>1.6.0</googleauth.version>
<guava.version>31.1-jre</guava.version>

@tivvtivvApr 4, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd try to use guava version from hydrator. Otherwise strange things may happen

@tivvtivvApr 4, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

And BTW, where are you using it? I don't see guava references in the source

@sumengwangsumengwangApr 4, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The GoogleCredentials is asking for one of the package from guava. It won't let me build before I include it.

@tivvtivvApr 4, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hm. Why don't they include it themselves? Usually it's up to an artifact to include any dependency needed

Comment threadsrc/main/java/io/cdap/plugin/http/source/common/http/AuthType.java
* @return The AuthType
*/
public static AuthType fromValue(String value) {
return Arrays.stream(AuthType.values()).filter(authtype -> authtype.getValue().equals(value))

@tivvtivvApr 4, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please use valueOf, this looks strange

Comment threadsrc/main/java/io/cdap/plugin/http/source/common/http/OAuthUtil.java Outdated

public static String getAccessTokenByServiceAccount(BaseHttpSourceConfig config) throws IOException {
GoogleCredentials credential;
Integer expiryLength = (config.getJwtTokenExpiryLength() != null) ? config.getJwtTokenExpiryLength() : 3600;

@tivvtivvApr 4, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this var a can be int

@sumengwangsumengwangApr 4, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The reason I use Integer is because it can be compared with null. I can't do that with int

@tivvtivvApr 4, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I understand for config, but I don't think you need to do it for expiryLength var

tivv
tivv approved these changes Apr 4, 2022
View reviewed changes
@sumengwangsumengwang changed the title [PLUGIN-692] Modify http batch source authentication UI[PLUGIN-692] Add Service Account auth to httpApr 6, 2022
@sumengwang
sumengwang merged commit f37f36f into developApr 14, 2022
@bdmogalbdmogal mentioned this pull request Nov 4, 2022
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tivvtivvtivv approved these changes
@fernstfernstAwaiting requested review from fernst
@CuriousViniCuriousViniAwaiting requested review from CuriousVini

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sumengwang@tivv@fernst