Feature/support oauth2 authentication #23
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@s7clarke10 can you think of anyway to setup unit tests for these auth items? I'm thinking of creating a new |
|
Hi Josh,
Yes the tests for the auth are challenging in that we really need some reliable public domain resource to test the various auth methods against. All my data sources were private end-points so I couldn’t use them for any generic tests.
I am aware of some FHIR Public Server and did a bit of a google. It looks like there are a few with different authentication mechanisms including OAuth2 - https://confluence.hl7.org/display/FHIR/Public+Test+Servers .
I wonder if that might be helpful. Being a public server however it is probably subject to change.
There maybe some other examples for different types of API’s like FHIR out there as well.
Cheers
Steve
From: jlloyd-widen ***@***.***>
Sent: Wednesday, 17 August 2022 11:07 AM
To: Widen/tap-rest-api-msdk ***@***.***>
Cc: Steve Clarke ***@***.***>; Mention ***@***.***>
Subject: Re: [Widen/tap-rest-api-msdk] Feature/support oauth2 authentication (PR #23)
@s7clarke10 <https://github.com/s7clarke10> can you think of anyway to setup unit tests for these auth items? I'm thinking of creating a new tests/test_auth.py and doing it there.
—
Reply to this email directly, view it on GitHub <#23 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AUDU42VVKREXWKZRKD7VVKDVZQNKBANCNFSM53RFYKUQ> .
You are receiving this because you were mentioned. <https://github.com/notifications/beacon/AUDU42SS4P2KPUTKOCDPJALVZQNKBA5CNFSM53RFYKU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOJCG6FSA.gif> Message ID: ***@***.*** ***@***.***> >
|
|
Hi Josh,
Just touching base with you regard the Pull Request #23 which was built onto of pull request #22 .
How are you going with my Pull Request, I was quite keen to get the changes out for the wider community as I can see a lot of benefits being able to support multiple forms of authentication and other request / response types.
Thanks
Steve
From: ***@***.*** ***@***.***>
Sent: Wednesday, 17 August 2022 1:12 PM
To: 'Widen/tap-rest-api-msdk' ***@***.***>; 'Widen/tap-rest-api-msdk' ***@***.***>
Cc: 'Mention' ***@***.***>
Subject: RE: [Widen/tap-rest-api-msdk] Feature/support oauth2 authentication (PR #23)
Hi Josh,
Yes the tests for the auth are challenging in that we really need some reliable public domain resource to test the various auth methods against. All my data sources were private end-points so I couldn’t use them for any generic tests.
I am aware of some FHIR Public Server and did a bit of a google. It looks like there are a few with different authentication mechanisms including OAuth2 - https://confluence.hl7.org/display/FHIR/Public+Test+Servers .
I wonder if that might be helpful. Being a public server however it is probably subject to change.
There maybe some other examples for different types of API’s like FHIR out there as well.
Cheers
Steve
From: jlloyd-widen ***@***.*** ***@***.***> >
Sent: Wednesday, 17 August 2022 11:07 AM
To: Widen/tap-rest-api-msdk ***@***.*** ***@***.***> >
Cc: Steve Clarke ***@***.*** ***@***.***> >; Mention ***@***.*** ***@***.***> >
Subject: Re: [Widen/tap-rest-api-msdk] Feature/support oauth2 authentication (PR #23)
@s7clarke10 <https://github.com/s7clarke10> can you think of anyway to setup unit tests for these auth items? I'm thinking of creating a new tests/test_auth.py and doing it there.
—
Reply to this email directly, view it on GitHub <#23 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AUDU42VVKREXWKZRKD7VVKDVZQNKBANCNFSM53RFYKUQ> .
You are receiving this because you were mentioned. <https://github.com/notifications/beacon/AUDU42SS4P2KPUTKOCDPJALVZQNKBA5CNFSM53RFYKU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOJCG6FSA.gif> Message ID: ***@***.*** ***@***.***> >
|
|
@s7clarke10 thanks for the reminder. I think I'm just waiting on unit tests (pytests) relating to both PRs I haven't had time to do it myself yet. So feel free to add some, preferably they wouldn't hit an external API but instead would just mock an external api call. If you can't get to it, then I think I might get back around to in a few weeks from now. Wish I could get to it sooner ... :( |
|
Hi Josh,
Thanks for the update. Yeah, like you no doubt I’m really struggling for time at the moment. I’m working on four other taps which haven’t been performing that great over the WAN.
I’ll let you know if I do have a chance to look at the tests.
I had to use the tap for a new FHIR API recently which was setup to expect a X-API-KEY and OAuth authentication too. I was pleased that when the X-API-KEY was placed in the header parameter and the rest of the authentication parameters were setup for OAuth it worked well and didn’t require any code changes.
Cheers
Steve
… On 27/09/2022, at 3:55 AM, jlloyd-widen ***@***.***> wrote:
@s7clarke10 thanks for the reminder. I think I'm just waiting on unit tests (pytests) relating to both PRs I haven't had time to do it myself yet. So feel free to add some, preferably they wouldn't hit an external API but instead would just mock an external api call. If you can't get to it, then I think I might get back around to in a few weeks from now. Wish I could get to it sooner ... :(
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
|
|
Closed: This logic is now incorporated in this PR - #35 |
|
Closed: This logic is now incorporated in this PR - #36 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This feature introduces additional authentication options that weren't possible with just headers in the request (specifically OAuth).
The Meltano sdk supports a number of authentication method which have been supported with this feature. The feature utilizes the available SDK Authenticators https://github.com/meltano/sdk/blob/main/singer_sdk/authenticators.py.
There are many forms of Authentication supported by this feature. By default for legacy support, you can still pass Authentication via headers it should have no breaking changes as part of this feature. If you want to use the built in support for Authentication, this tap supports :
Please note that OAuthJWTAuthentication has not been developed.
It should be noted that while the Authenticators normally go in the stream.py, I have implemented this in the tap.py because we need the Authenticator to discover the schema which is part of the tap.py logic. The authentication has also just been applied at the top-level, it has not be applied as a individual stream by stream form of authentication.
Finally Note: This branch was built on top of the previous feature "Feature/support_link_list_pagination_tokens" so include the logic for this feature which introduced support for HATEOAS/FHIR API request/reponse styles. There is a previous PR which included this.
Cheers
Steve