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: allow login via admin ui on safari #10407

Merged
merged 1 commit into from
Jun 7, 2022
Merged

Conversation

johnpc
Copy link
Contributor

@johnpc johnpc commented May 12, 2022

Description of changes

Currently, when pulling a project via amplify pull, it is possible to use Amplify Studio to fetch authentication tokens in browser and pass them off to the CLI's admin-login express server.

However, the local express server cannot be reached in certain browsers due to security settings (including Safari, Brave, and others).

[Warning] [blocked] The page at https://us-east-2.admin.amplifyapp.com/admin/d1v4tl0j988su7/dev/verify/ was not allowed to display insecure content from http://127.0.0.1:4242/amplifyadmin/. (main.aa9ff9bc.chunk.js, line 1)

Currently, this is "solved" with a warning banner, alerting the user to switch to Chrome, Firefox, or Edge (browsers that do not [yet] enforce this policy).

Screen Shot 2022-05-12 at 10 38 43 AM

This PR proposes a new solution this problem. First, the browser will hit a GET /ping endpoint to check whether communication with the local express server is possible from the given context, which can be used instead of a known list of working browsers. This also makes SSH authentication workflows possible.

If communication is possible, it will continue to work by passing tokens to the express server.

If communication is not possible, the browser will instead display a "copy code" button that copies the authorization context encoded as a base64 string of the json blob that would typically be passed to the express server. The end user would then paste that base64 string into the terminal prompt where Amplify CLI is running to fetch the tokens.

Before:

Screen Shot 2022-05-12 at 10 34 38 AM

After:

Screen Shot 2022-05-12 at 11 36 41 AM

If the tokens are passed off via the express server, the express server kills the prompt. If the prompt is used to pass the base64 tokens along, the prompt promise kills the express server.

Issue #, if available

N/A

Description of how you validated changes

  • Pulled project, authenticated by pasting base64 string
  • Pulled project, authenticated normally via express server

Both paths work as expected.

Checklist

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

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

@johnpc johnpc requested a review from a team as a code owner May 12, 2022 15:38
Copy link
Contributor

@lazpavel lazpavel left a comment

Choose a reason for hiding this comment

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

LGTM

"jsdoc",
"jwks",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: for this is probably better to add a lint ignore that adding to the dictionary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally our spell checker would allow names of dependencies or exports of dependencies

if (!req.body || req.body.error) {
this.shutdown();
if (req.body.error === 'CANCELLED') {
this.print.info('Login cancelled');
this.print.info('Login canceled');
Copy link
Contributor

Choose a reason for hiding this comment

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

yarn.lock Outdated Show resolved Hide resolved
@johnpc johnpc marked this pull request as draft May 12, 2022 20:28
@johnpc
Copy link
Contributor Author

johnpc commented May 12, 2022

This will require careful alignment with the Studio team. If they don't roll out at the same time, either the code will presented in the web ui (with nowhere to paste it) or the CLI will prompt for a code that does not appear anywhere in the web ui.

@johnpc johnpc force-pushed the safari-admin-login branch 2 times, most recently from 13c6986 to 1469b0c Compare May 13, 2022 15:05
@lgtm-com
Copy link

lgtm-com bot commented May 13, 2022

This pull request introduces 1 alert when merging 1469b0c into 59c6422 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented May 13, 2022

This pull request introduces 1 alert when merging 3820a94 into 59c6422 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented May 13, 2022

This pull request introduces 1 alert when merging af36419 into 59c6422 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented May 19, 2022

This pull request introduces 1 alert when merging e0eeff9 into d9bdcd8 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@johnpc johnpc marked this pull request as ready for review May 23, 2022 16:30
@lgtm-com
Copy link

lgtm-com bot commented May 24, 2022

This pull request introduces 1 alert when merging 83b0589 into 59089dd - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@johnpc johnpc changed the title feat: allow login via admin ui on safari feat: allow login via admin ui on safari 🚧 DO NOT MERGE 🚧 May 24, 2022
@johnpc
Copy link
Contributor Author

johnpc commented May 24, 2022

I will merge this when the backend is ready

@lgtm-com
Copy link

lgtm-com bot commented May 24, 2022

This pull request introduces 1 alert when merging ac06145 into 21ceba1 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@codecov-commenter
Copy link

Codecov Report

Merging #10407 (ac06145) into master (714f2fa) will decrease coverage by 0.01%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master   #10407      +/-   ##
==========================================
- Coverage   46.57%   46.56%   -0.02%     
==========================================
  Files         703      703              
  Lines       35246    35256      +10     
  Branches     7134     7134              
==========================================
+ Hits        16417    16418       +1     
- Misses      17042    17051       +9     
  Partials     1787     1787              
Impacted Files Coverage Δ
...vider-awscloudformation/src/utils/admin-helpers.ts 20.33% <0.00%> (ø)
...lify-provider-awscloudformation/src/admin-login.ts 14.28% <13.63%> (-4.24%) ⬇️
...-awscloudformation/src/utils/admin-login-server.ts 42.85% <62.50%> (+0.21%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@johnpc johnpc changed the title feat: allow login via admin ui on safari 🚧 DO NOT MERGE 🚧 feat: allow login via admin ui on safari Jun 7, 2022
@ammarkarachi
Copy link
Contributor

fixes #6965

@ammarkarachi ammarkarachi merged commit f6f563c into master Jun 7, 2022
lazpavel added a commit that referenced this pull request Jun 14, 2022
sachscode pushed a commit that referenced this pull request Jun 14, 2022
johnpc added a commit to johnpc/amplify-cli that referenced this pull request Jun 15, 2022
@johnpc johnpc mentioned this pull request Jun 15, 2022
3 tasks
sachscode pushed a commit that referenced this pull request Jun 23, 2022
* Revert "Revert "feat: allow login via admin ui on safari (#10407)" (#10602)"

This reverts commit 16104ea.

* fix: handle promise resolution to prevent hanging after Studio pull
@evcodes evcodes deleted the safari-admin-login branch February 18, 2023 03:03
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