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(Auth): Implementation of getCurrentUser api #455

Merged
merged 1 commit into from
May 21, 2020

Conversation

royjit
Copy link
Contributor

@royjit royjit commented May 18, 2020

Description of changes:

This PR is dependent on userSub api in AWSMobileClient - aws-amplify/aws-sdk-ios#2599

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

@royjit royjit self-assigned this May 18, 2020
@royjit royjit added the auth Issues related to the Auth category label May 18, 2020
@royjit royjit requested a review from palpatim May 18, 2020 18:40
@@ -16,13 +16,14 @@ class AuthenticationProviderAdapter: AuthenticationProviderBehavior {
self.awsMobileClient = awsMobileClient
}

func signInUsername() -> Result<String, AuthError> {
func currentUser() -> AuthUser? {
Copy link
Member

Choose a reason for hiding this comment

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

rename to getCurrentUser, or make this a property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Comment on lines 14 to 15
/// Value maps to the username of a user in AWS Cognito User Pool. This value is set by AWS Cognito and not by the
/// user and does not always map with the username used to signIn.
Copy link
Member

Choose a reason for hiding this comment

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

As noted offline, we need to figure out this behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keeping this for now as discussed

@royjit royjit force-pushed the royjit/AuthCurrentUser branch from cbb4aff to c7bd293 Compare May 20, 2020 00:23
@royjit royjit requested a review from palpatim May 20, 2020 00:27
Copy link
Member

@palpatim palpatim left a comment

Choose a reason for hiding this comment

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

Approved w/comments, but in general, we need to pay better attention to our naming, even internally. If a method is returning a value, name it 'get' (for retrievals) or 'make' (for creation). If a value can be reasonably modeled as a property (synchronous, no throwing conditions, no parameters required), favor properties rather than getter methods.

@@ -107,6 +107,10 @@ class AWSMobileClientAdapter: AWSMobileClientBehavior {
return awsMobileClient.username
}

func userSub() -> String? {
Copy link
Member

Choose a reason for hiding this comment

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

Make this getUserSub() or a property

@@ -52,6 +52,8 @@ protocol AWSMobileClientBehavior {

func username() -> String?

func userSub() -> String?
Copy link
Member

Choose a reason for hiding this comment

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

make this getUserSub() or a userSub property.

@@ -52,6 +52,8 @@ protocol AWSMobileClientBehavior {

func username() -> String?
Copy link
Member

Choose a reason for hiding this comment

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

Make this getUsername() or a username property

@royjit royjit force-pushed the royjit/AuthCurrentUser branch 2 times, most recently from be2d637 to 1a50870 Compare May 20, 2020 15:25
@royjit royjit force-pushed the royjit/AuthCurrentUser branch from 1a50870 to 6e09cc4 Compare May 21, 2020 15:21
@royjit royjit merged commit 59f6b18 into master May 21, 2020
@royjit royjit deleted the royjit/AuthCurrentUser branch May 21, 2020 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth Issues related to the Auth category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants