-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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: pipe auth user agent details through to service call #11755
feat: pipe auth user agent details through to service call #11755
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.
Overall looks good, just a few questions related to undefined
values
Thanks @erinleigh90
this._storage.setItem( | ||
'aws-amplify-federatedUserAgent', | ||
getAuthUserAgentValue( | ||
AuthAction.FederatedSignIn, | ||
customUserAgentDetails | ||
) | ||
); | ||
|
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.
Why do you need this?
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 is used to get the user agent value passed to federatedSignIn to the OAuth handleAuthResponse (see this snippet here for where this is used): since this is triggered by a url listener, this seemed like the most sure-fire way of maintaining that information using existing functionality.
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.
make sense
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.
Looks good, pending integ test results. I'm curious about the use of aws-amplify-federatedUserAgent
, what's the use-case? Also I'm probably missing something, but shouldn't there be corresponding Auth
class changes to use InternalAuth
?
The Auth class changes to use InternalAuth are already merged to main via this PR |
I'm confused what this test failure in GitHub actions is about: it's showing that none of the AuthActions in core/src/Platform/types.ts exist, which is not the case. Tests are passing in the circleci run of unit tests |
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## main #11755 +/- ##
==========================================
+ Coverage 84.03% 84.11% +0.08%
==========================================
Files 348 349 +1
Lines 21037 21155 +118
Branches 4445 4449 +4
==========================================
+ Hits 17678 17795 +117
- Misses 3098 3099 +1
Partials 261 261
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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 package.json
updates for api
, api-graphql
, and datastore
.
Description of changes
Sends userAgentValue from Auth to Cognito, incorporating customUserAgentDetails received from UI team.
NOTE: still working on integ test failures, but am looking for early feedback.
Description of how you validated changes
Checklist
yarn test
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.