Skip to content
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

Merged

Conversation

erinleigh90
Copy link
Contributor

@erinleigh90 erinleigh90 commented Aug 8, 2023

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

  • unit tests passing
  • integ tests passing
  • smoke tested in sample apps

Checklist

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@erinleigh90 erinleigh90 marked this pull request as ready for review August 8, 2023 22:59
@erinleigh90 erinleigh90 requested review from a team as code owners August 8, 2023 22:59
@erinleigh90 erinleigh90 requested a review from a team as a code owner August 8, 2023 23:03
Copy link
Contributor

@elorzafe elorzafe left a 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

.circleci/config.yml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Comment on lines +2831 to +2838
this._storage.setItem(
'aws-amplify-federatedUserAgent',
getAuthUserAgentValue(
AuthAction.FederatedSignIn,
customUserAgentDetails
)
);

Copy link
Contributor

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?

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make sense

packages/auth/src/internals/InternalAuth.ts Show resolved Hide resolved
packages/auth/src/internals/InternalAuth.ts Show resolved Hide resolved
packages/auth/src/internals/InternalAuth.ts Show resolved Hide resolved
packages/auth/src/internals/InternalAuth.ts Show resolved Hide resolved
packages/auth/src/internals/InternalAuth.ts Outdated Show resolved Hide resolved
packages/auth/src/internals/InternalAuth.ts Show resolved Hide resolved
packages/auth/src/internals/InternalAuth.ts Show resolved Hide resolved
Copy link
Contributor

@jimblanc jimblanc left a 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?

packages/auth/src/internals/InternalAuth.ts Show resolved Hide resolved
packages/auth/src/internals/InternalAuth.ts Outdated Show resolved Hide resolved
@erinleigh90
Copy link
Contributor Author

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

@erinleigh90
Copy link
Contributor Author

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?

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-commenter
Copy link

codecov-commenter commented Aug 9, 2023

Codecov Report

Merging #11755 (10b14ca) into main (57127c5) will increase coverage by 0.08%.
Report is 9 commits behind head on main.
The diff coverage is 91.53%.

❗ 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              
Files Changed Coverage Δ
packages/auth/src/internals/InternalAuth.ts 88.43% <90.00%> (+0.59%) ⬆️
packages/auth/src/OAuth/OAuth.ts 87.97% <100.00%> (+0.07%) ⬆️
packages/auth/src/utils.ts 100.00% <100.00%> (ø)
packages/core/src/Platform/types.ts 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

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

@erinleigh90 erinleigh90 merged commit 9497c28 into aws-amplify:main Aug 14, 2023
31 checks passed
@erinleigh90 erinleigh90 deleted the feat/auth-to-internal-cognito branch August 14, 2023 18:21
stocaaro added a commit that referenced this pull request Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants