-
Notifications
You must be signed in to change notification settings - Fork 26
feat: added support for optional audience param #423
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
feat: added support for optional audience param #423
Conversation
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.
We can chat about testing when you're back if you like. I think the approach will be to just assert that we're making the expected POST requests.
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.
Feedback in two major categories:
- Thoughts on the workflow generating a publisher-scoped client object, which depend upon how the
audience
parameter interacts withrequested-token-type
(which I'm not 100% sure about). - Comments on documentation / how we communicate the parameter.
I realize now in your Slack comment about the testing framework — the easiest way to bootstrap those responses you need is to capture them from a Connect server. Happy to pair on that if you want. When you capture them from a server, appropriately-named .json
files are created.
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 generally looks good to me, just a few comments about tidying up tests etc.
I'm holding off on hitting "Approve" because I think it'd be good to validate this on a real Connect server. I'm not sure how we could do that easily on an integration test; it might require coding a bit of toy content and deploying it to a dev Connect instance to ensure that the audience parameter works.
Perhaps I'm being overly cautious, and if so, @mconflitti-pbc or other reviewers can push back. I'm happy to help verify this tomorrow. :)
tests/testthat/2024.08.0/__api__/v1/oauth/integrations/credentials-224fa9-POST.json
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.
Oops, I meant my last review to go through as "Comment", not "Request changes".
[edit] I guess I did select comment. At least, I know I did for this review. Does GH automatically show your review as "Requested changes" if you leave a comment with suggestions and select "Comment" now?
Update: Verified on Connect. I deployed an app here: https://dogfood.team.pct.posit.it/connect/#/apps/bceccf63-a4c8-48de-b835-f681d04a87a0/environment There are two env vars:
These work together, i.e. there are four permutations. The app creates a client and prints out the authenticated username, plus whether a token is being used or not, and whether an audience guid is being used. Behaviors observed:
I do think it might be better on the Connect server side to only require the audience param if there is more than one Connect API integration added — right now it's required even if, say, you have a Connect API integration and a Snowflake auth integration selected. I would have thought that in that case, the requested token type ( |
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.
Approving after verifying on the server.
addressed concerns around version constraint error
Intent
Adds optional audience parameter to our oauth creds helpers. Unclear how to handle connect versions constraint since that relies on the oauth feature being there but this is an added param not a new function.
Need to address parity the python sdk through additional integration and association helpers as a follow up.
Approach
Add optional param
Checklist
NEWS.md
(referencing the connected issue if necessary)?devtools::document()
?