-
Notifications
You must be signed in to change notification settings - Fork 590
Added verification of tasks/thinexecutions in DataflowOAuthIT. #5817
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
Conversation
log.debug("Response is {}", response); | ||
ok = !JsonPath.parse(response).read("$._links.self.href", String.class).isEmpty(); | ||
|
||
// TODO add checks for new endpoints to check security |
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.
This does add coverage for the thin/executions
endpoint but what is the expectation for newly added endpoints? How will we know to abide by this comment?
The issue that we ran into was that PRO was out of sync w/ OSS endpoint mappings. This does not fill that gap.
I would recommend removing this 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.
This test is executed as part of ci-it-security.
The Pro will be covered by the endpoint test running in CF ATs.
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.
The Pro will be covered by the endpoint test running in CF ATs.
Which endpoint test? The point of https://github.com/pivotal/scdf-pro/issues/192 is to find out issues prior to CF ATs (in the PRO tests).
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.
I added ci-it-security to scdf-pro to perform similar checks on added endpoints to ensure they are protected.
...src/test/java/org/springframework/cloud/dataflow/integration/test/oauth/DataflowOAuthIT.java
Outdated
Show resolved
Hide resolved
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.
Thx @corneil - a few comments to address.
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.
I agree with Chris' requests. No further additions from my side.
response = cmdResult.getStdout(); | ||
log.debug("Response is {}", response); | ||
ok = !JsonPath.parse(response).read("$._links.self.href", String.class).isEmpty(); | ||
// TODO add checks for new endpoints to check security |
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.
I don't think this test is the place to add endpoints to. I still would like to remove this comment.
...src/test/java/org/springframework/cloud/dataflow/integration/test/oauth/DataflowOAuthIT.java
Show resolved
Hide resolved
c0462f2
to
f0fb797
Compare
Recreating from personal repo |
No description provided.