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

Extend withIdentityPoolId signature to allow usage of authenticated identities #30

Merged
merged 6 commits into from
Mar 5, 2024

Conversation

mirgj
Copy link
Contributor

@mirgj mirgj commented Feb 14, 2024

Issue #, if available:
NA

Description of changes:

The current implementation of the withIdentityPoolId method does not allow to use authenticated identities to perform actions against the Amazon Location APIs (thus forcing to use only unauthenticated identities). This PR updates the signature of the method mentioned above to expose props that allow to customise the behaviour of the fromCognitoIdentityPool underlying API ref.

With this change is now possible to use authenticated identities ref to provide access to Amazon Location Service

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@mirgj mirgj requested a review from a team as a code owner February 14, 2024 11:36
Copy link

@sperka sperka left a comment

Choose a reason for hiding this comment

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

lgtm.

only tiny change req is to fix that typo

README.md Outdated Show resolved Hide resolved
@mirgj mirgj requested a review from sperka February 16, 2024 03:42
sperka
sperka previously approved these changes Feb 16, 2024
Copy link

@sperka sperka left a comment

Choose a reason for hiding this comment

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

🚀

Comment on lines +13 to +16
export async function withIdentityPoolId(
identityPoolId: string,
options?: Partial<FromCognitoIdentityPoolParameters>,
): Promise<MapAuthHelper & SDKAuthHelper> {
Copy link
Contributor

Choose a reason for hiding this comment

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

It deserve a new function than extending withIdentityPoolId.

I would propose a new function like fromCognitoIdentityPool, and call @aws-sdk/credential-providers's fromCognitoIdentityPool with the FromCognitoIdentityPoolParameters without modification. Since this change basically merges everything inside options and customer can supply identityPoolId and region inside FromCognitoIdentityPoolParameters

Copy link
Contributor Author

@mirgj mirgj Feb 22, 2024

Choose a reason for hiding this comment

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

Since this change basically merges everything inside options and customer can supply identityPoolId and region inside FromCognitoIdentityPoolParameters

Yes, with the current implementation they could but the information provided in the first param identityPoolId (which is required) will be the one used to invoke the underlying fromCognitoIdentityPool method.

I can make the change you're suggesting but it will require more work and I don't see the point of providing an interface with two different methods name that are doing the same thing. This can be done with an overload (which is what I've proposed in this PR).

I've added a test case that cover the scenario you mention

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a use case that customer can use identity pool without identity pool id (for example, by specifying other parameters)? If there is no suce use case I am OK with overloading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, that's not possible. The underlying method (fromCognitoIdentityPool) would require to provide identityPoolId in the options. Any other field is optional (which is when the overload comes handy).

@eashuang eashuang merged commit 2705022 into aws-geospatial:main Mar 5, 2024
1 check passed
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.

3 participants