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

Cognito hosted ui #585

Merged
merged 33 commits into from
Apr 13, 2018
Merged

Conversation

powerful23
Copy link
Contributor

@powerful23 powerful23 commented Apr 3, 2018

Issue #, if available:
Fixes #565 #480 #311 #45 #542 #376
Description of changes:
Integrate Cognito hosted ui into aws-amplify

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

@powerful23 powerful23 requested review from mbahar and mlabieniec April 3, 2018 20:46
Copy link
Contributor

@mlabieniec mlabieniec left a comment

Choose a reason for hiding this comment

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

Update withCognito -> withHostedCognito

Copy link
Contributor

@mlabieniec mlabieniec left a comment

Choose a reason for hiding this comment

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

👍

@codecov-io
Copy link

codecov-io commented Apr 6, 2018

Codecov Report

Merging #585 into master will increase coverage by 0.24%.
The diff coverage is 76.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #585      +/-   ##
==========================================
+ Coverage   86.93%   87.17%   +0.24%     
==========================================
  Files          71       72       +1     
  Lines        3345     3424      +79     
  Branches      634      648      +14     
==========================================
+ Hits         2908     2985      +77     
- Misses        420      422       +2     
  Partials       17       17
Impacted Files Coverage Δ
packages/aws-amplify/src/Common/Facet.ts 100% <100%> (ø) ⬆️
...ages/aws-amplify-react/src/Auth/Provider/index.jsx 100% <100%> (ø) ⬆️
packages/aws-amplify/src/Analytics/Analytics.ts 84.31% <100%> (ø) ⬆️
packages/aws-amplify/src/Common/Parser.ts 70.58% <55.55%> (+6.95%) ⬆️
...ges/aws-amplify-react/src/Auth/FederatedSignIn.jsx 72.97% <64.28%> (-19.03%) ⬇️
packages/aws-amplify/src/Auth/Auth.ts 87.58% <79.66%> (+2.87%) ⬆️
.../aws-amplify-react/src/Auth/Provider/withOAuth.jsx 82.35% <82.35%> (ø)
packages/aws-amplify/src/Common/index.ts 80.95% <0%> (-4.77%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2270af2...f3e65aa. Read the comment docs.

mbahar
mbahar previously requested changes Apr 10, 2018
Copy link
Contributor

@mbahar mbahar left a comment

Choose a reason for hiding this comment

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

please note the change;
withHostedCognito -> withHostedAuthenticator

@richardzcode richardzcode dismissed mbahar’s stale review April 11, 2018 18:34

The name has changed to withOAuth

Copy link
Contributor

@mlabieniec mlabieniec left a comment

Choose a reason for hiding this comment

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

👍

@powerful23 powerful23 merged commit 8eeef2f into aws-amplify:master Apr 13, 2018
@ceich
Copy link
Contributor

ceich commented Apr 24, 2018

I am glad to see this in a release, but surprised that the release minor number didn't bump for a feature add.

@lucaslc-sp
Copy link

hello @mlabieniec / @powerful23

Do you know if the method Auth.federatedSignIn() creates a facebook user in cognito user pool?
I receive a credentials on method response but facebook user is not created in cognito user pool ..

@powerful23
Copy link
Contributor Author

powerful23 commented May 16, 2018

@lucaslc-sp no you need to follow this doc, basically Auth.federatedSignIn() just get a credential from federated identity pool but has nothing to do with Cognito user pool

@lucaslc-sp
Copy link

@powerful23 Thank you! If you allow me, one more question: what i have to put in redirect_url parameter to return to my react native app?

@mlabieniec
Copy link
Contributor

@lucaslc-sp you would need to add a deep link for your app based on your platform:

https://facebook.github.io/react-native/docs/linking.html

@lucaslc-sp
Copy link

@mlabieniec Thank you!

@powerful23 powerful23 deleted the cognito-hosted-ui branch May 22, 2018 22:59
@reggie3
Copy link

reggie3 commented Jul 13, 2018

@powerful23 , In reference to @lucaslc-sp's question, do you know if there is a way to create a facebook user in cognito user pool? I have enabled Google and Facebook login on my user pool, and I have user pool groups for both types of users, but if what you say is true, what is the purpose of either of those groups? I am using a custom login UI that is totally separate from the Hosted Login UI, if that makes a difference.

@ceich
Copy link
Contributor

ceich commented Jul 13, 2018

@reggie3 In my experience, the only way to get a federated login to create a UP entry is to use hosted UI.

@powerful23
Copy link
Contributor Author

@reggie3 as I know for now the only way to make federated users sign into Cognito User Pool is to use the Hosted UI feature. @yuntuowang do you know if there is any way to use custom login UI to implement this?

@mainawycliffe
Copy link

@powerful23 did you find any solution for this?

@github-actions
Copy link

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants