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.

Sign up for GitHub

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

Jump to bottom

fix(auth): custom userPoolEndpoint cannot be applied on the server-side #13739

Merged
merged 15 commits into from
Sep 10, 2024
Merged

fix(auth): custom userPoolEndpoint cannot be applied on the server-side #13739

merged 15 commits into from
Sep 10, 2024

Conversation

HuiSF
Copy link
Member

@HuiSF HuiSF commented Aug 20, 2024

Description of changes

This PR re-organized the Cognito Identity service clients, and turn them into factory function. These factories expecting an endpointResolver to be injected as dependency. So they can be created within the Auth category API calls, and ensure it uses the always up-to-date endpoint information.

Issue #, if available

#13650

Description of how you validated changes

  • Updated and added unit tests
  • Manual testing calling the Auth category APIs and verify a custom user pool endpoint is applied

Checklist

  • PR description included
  • yarn test passes
  • Unit Tests are changed or added
  • Relevant documentation is changed or added (and PR referenced)

Checklist for repo maintainers

  • Verify E2E tests for existing workflows are working as expected or add E2E tests for newly added workflows
  • New source file paths included in this PR have been added to CODEOWNERS, if appropriate

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

@github-actions GitHub Actions
Copy link

⚠️ This PR includes changes to the "aws-amplify" package.json file, which can have library-wide implications.

Please ensure that this PR:

  • Does not manually change "@aws-amplify/*" dependency versions, which may misalign core dependencies across the library
  • Remove any export paths without a major version bump

A repository administrator is required to review this change.

@HuiSF HuiSF requested a review from israx August 20, 2024 23:28
@HuiSF HuiSF mentioned this pull request Aug 21, 2024
Open
6 tasks
cshfang
cshfang reviewed Aug 21, 2024
View reviewed changes
packages/auth/src/foundation/core/parsers/getRegion.ts Outdated Show resolved Hide resolved
packages/auth/src/foundation/core/cognitoUserPoolEndpointResolver.ts Outdated Show resolved Hide resolved
packages/auth/src/foundation/core/parsers/index.ts Outdated
@@ -0,0 +1,6 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

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

General question - how do we feel about index files? I think I still like them for selectively exposing functions from a group (subpath/folder)

Copy link
Member Author

Choose a reason for hiding this comment

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

It shortens import path on some occasions which is a nice way to organize code.
Though need to be care full import unexpected side effect?

e.g.

index.ts 
  module1.ts // <- has a side effect executes on module loads
  module2.ts

I want to use a function provided from module2, if import from index.ts, the side effect lives in module1 may get attached.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like a good reason to consider dropping the practice overall except for maybe exposing types and controlling top level exports 🤔

...ation/factories/serviceClients/cognitoIdentityProvider/createAssociateSoftwareTokenClient.ts Outdated Show resolved Hide resolved
.../auth/src/foundation/factories/serviceClients/cognitoIdentityProvider/types/ServiceClient.ts Outdated Show resolved Hide resolved
...ies/serviceClients/cognitoIdentityProvider/shared/serialization/buildUserPoolDeserializer.ts Outdated Show resolved Hide resolved
packages/auth/src/providers/cognito/utils/signInHelpers.ts Show resolved Hide resolved
packages/auth/src/providers/cognito/utils/signInHelpers.ts Show resolved Hide resolved
packages/auth/__tests__/foundation/core/parsers/getRegion.test.ts Outdated Show resolved Hide resolved
...serviceClients/cognitoIdentityProvider/shared/handler/cognitoUserPoolTransferHandler.test.ts Outdated Show resolved Hide resolved
cshfang
cshfang reviewed Aug 22, 2024
View reviewed changes
packages/auth/__tests__/foundation/core/parsers/getRegion.test.ts Outdated Show resolved Hide resolved
...eClients/cognitoIdentityProvider/shared/serialization/buildEmptyResponseDeserializer.test.ts Outdated Show resolved Hide resolved
...ories/serviceClients/cognitoIdentityProvider/shared/serde/createEmptyResponseDeserializer.ts Outdated Show resolved Hide resolved
packages/auth/src/foundation/core/parsers/index.ts Outdated
@@ -0,0 +1,6 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Seems like a good reason to consider dropping the practice overall except for maybe exposing types and controlling top level exports 🤔

packages/auth/src/providers/cognito/utils/signInHelpers.ts Show resolved Hide resolved
israx
israx reviewed Aug 22, 2024
View reviewed changes
packages/auth/src/providers/cognito/factories/createCognitoUserPoolEndpointResolver.ts
import { cognitoUserPoolEndpointResolver } from '../../../foundation/cognitoUserPoolEndpointResolver';

export const createCognitoUserPoolEndpointResolver =
({ endpointOverride }: { endpointOverride: string | undefined }) =>
Copy link
Member

Choose a reason for hiding this comment

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

why not just making the endpointOverride key optional instead of forcing to pass a value?

Copy link
Member Author

Choose a reason for hiding this comment

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

See this comment, it answers this.

cshfang
cshfang previously approved these changes Aug 22, 2024
View reviewed changes
israx
israx previously approved these changes Aug 23, 2024
View reviewed changes
AllanZhengYP
AllanZhengYP previously approved these changes Aug 23, 2024
View reviewed changes
ashika112
ashika112 previously approved these changes Aug 24, 2024
View reviewed changes
Samaritan1011001
Samaritan1011001 reviewed Sep 5, 2024
View reviewed changes
packages/auth/src/providers/cognito/apis/confirmSignIn.ts
@@ -27,12 +27,12 @@ import { assertValidationError } from '../../../errors/utils/assertValidationErr
import { AuthValidationErrorCode } from '../../../errors/types/validation';
import { AuthErrorCodes } from '../../../common/AuthErrorStrings';
import { cacheCognitoTokens } from '../tokenProvider/cacheTokens';
import { tokenOrchestrator } from '../tokenProvider';
Copy link
Member

Choose a reason for hiding this comment

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

nit: unused?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm pretty sure it's used at line 98

@HuiSF HuiSF merged commit a76b594 into main Sep 10, 2024
30 checks passed
@HuiSF HuiSF deleted the hui/fix/auth/endpoint-resolver branch September 10, 2024 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashika112 ashika112 ashika112 approved these changes

@israx israx israx approved these changes

@cshfang cshfang cshfang approved these changes

@ukhan-amazon ukhan-amazon Awaiting requested review from ukhan-amazon

@haverchuck haverchuck Awaiting requested review from haverchuck haverchuck is a code owner

@jimblanc jimblanc Awaiting requested review from jimblanc

@Samaritan1011001 Samaritan1011001 Awaiting requested review from Samaritan1011001

@AllanZhengYP AllanZhengYP Awaiting requested review from AllanZhengYP

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@HuiSF @cshfang @AllanZhengYP @Samaritan1011001 @israx @ashika112