Skip to content
This repository was archived by the owner on May 14, 2025. It is now read-only.

Added verification of tasks/thinexecutions in DataflowOAuthIT.#5817

Closed
corneil wants to merge 3 commits intomainfrom
corneil/verify-endpoints-oauth-it
Closed

Added verification of tasks/thinexecutions in DataflowOAuthIT.#5817
corneil wants to merge 3 commits intomainfrom
corneil/verify-endpoints-oauth-it

Conversation

@corneil
Copy link
Contributor

@corneil corneil commented May 17, 2024

No description provided.

@corneil corneil requested review from cppwfs and onobc May 17, 2024 16:05
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
Copy link
Contributor

@onobc onobc May 22, 2024

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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).

Copy link
Contributor Author

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.

Copy link
Contributor

@onobc onobc left a 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.

Copy link
Contributor

@cppwfs cppwfs left a 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.

@corneil corneil requested a review from onobc May 23, 2024 09:40
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
Copy link
Contributor

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.

@corneil corneil requested review from cppwfs and onobc May 30, 2024 09:58
@corneil corneil requested a review from cppwfs June 6, 2024 08:38
@cppwfs cppwfs added this to the 2.11.4 milestone Jun 11, 2024
@cppwfs cppwfs modified the milestones: 2.11.4, 2.11.5 Jul 12, 2024
@corneil corneil force-pushed the main branch 2 times, most recently from c0462f2 to f0fb797 Compare August 8, 2024 11:55
@cppwfs cppwfs added the type/forwardport Is a issue to track forward, use with branch/xxx label Aug 23, 2024
@corneil
Copy link
Contributor Author

corneil commented Sep 6, 2024

Recreating from personal repo

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

type/forwardport Is a issue to track forward, use with branch/xxx

Development

Successfully merging this pull request may close these issues.

3 participants