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

Merge main into branch vc-mvp-2 #1931

Merged
merged 49 commits into from
Sep 25, 2023
Merged

Conversation

frederikrothenberger
Copy link
Contributor

@frederikrothenberger frederikrothenberger commented Sep 25, 2023

This is the merge of main into vc-mvp-2 (with showcase conflict resolution).


🟢 Some screens were added
🟡 Some screens were changed
🔴 Some screens were removed

Frederik Rothenberger and others added 30 commits September 13, 2023 13:00
This is a PR in preparation for adding the PIN auth feature, which
will require support for indexeddb in tests.
* Add support for dynamic error keys to pin input component

Support for dynamic keys is required to make the pin input compatible
with the error texts provided by `LoginFlowError`.

* Import type
* Always autosubmit when PIN input is filled

On error, there is no way currently to change the input and resubmit
because there is no submit button and the autosubmit only submits once.

After quick check-back with Artem it was decided that the autosubmit should
submit whenever the input is filled.

* Fix test
* Provide verify function externally to usePin flow

This PR expands the usePin function to take a PIN verification
function as an argument. This will later be used to immediately check
whether the identity can be decrypted using the provided PIN.

* Fix lint error

* Implement review feedback
This PR adds support for pin identities to the authentication box.
The feature won't become active yet, as users cannot register pin
identities.

The flow/functionality however can be tested using the showcase flow.

Basically all of the code is taken from the `nm-set-pin` branch by @nmattia.
This PR is in preparation for pin auth registration.
For this feature, we need to have more control over the
key type submitted to II.
This PR enables registration of PIN protected browser keys when
signing up for II. The feature is enabled on Apple devices only,
because it is meant as a workaround for the forced Apple iCloud
integration.

When adding a device later, the PIN protected browser key is not
allowed, even on Apple devices.

The feature is still very basic. In particular, still missing are:
- the temporary key info screen
- updates to the management page to correctly list temporary keys
  separate from passkeys
- warnings on the management screen

Almost all of the code is taken from the `nm-set-pin` branch by @nmattia.
)

This allows users to distinguish between the temporary keys and
passkeys when visiting the management page.
This PR introduces the information page for temporary keys
to the showcase. It is not yet part of the pin registration flow.
This PR adds the pin info page to the pin registration flow.
Additional warnings, etc. need to be added.
Extracting copy and separating everything related to temp keys into its
own file makes it easier to modify later.
This adds the warning as shown in the figmas to the PIN info screen.
* Add TempKey Security Warning to Management Screen

This PR adds a security warning to the management page if you have
signed in using a PIN protected browser key, if there is no recovery
phrase and/or passkey.

* Remove unnecessary tooltip classes

* Fix misleading id

* Extract button duplication

* Improve variable naming

* Implement review feedback
…1887)

This PR extracts the stepper on the finish page and adjusts it to
show the appropriate steps depending on whether a passkey or a pin
protected storage key was registered.
It also renames the first step from "set" to "set_pin"
because `set` is a reserved keyword in js.
According to the figmans, the temp keys need to be shown first.
* Highlight recovery box on no recoveries

If there are no recovery methods, the recovery section is now highlighted
with a warning.

* Implement review feedback
* Highlight passkey warning only on 0 passkeys

This changes the warning highlight around passkeys to only
appear if the number of passkeys is zero. Also, there is now a
text shown for passkeys in the non-warning state.

Together with #1888 this shifts the warning box highlight on an
identity with just a single passkey from the passkey box to the
recovery box.

* Implement review feedback

* Don't push classes
This PR updates the docker selenium container. In addition, it changes
the resolution within the container so that the whole browser can be seen
when connecting via noVNC.
Fix dapps update not job not creating a PR

The update action skipped on a variable that was never set, thus never
actually updating the dapps file.
The dapps.json update could trigger the formatter on badly formatted
json. This PR fixes the job to create the PR with acceptable formatting
in the first place.
The formatter needs to be installed first before it can be run
in the dapps update workflow. Fixes oversight in #1895.
This automatically converts all the JPG icons to webp when
updating dapps.
SVGs with a smaller size than 256*256 were resized in a way
that just put the original image in the top-left corner.
This PR fixes this and resizes the icons to appropriately fill
the space.
Remove openssl install step in canister tests CI step
Update dapps

Co-authored-by: gix-bot <[email protected]>
This adds the first basic e2e tests for the PIN registration feature.
* Add PIN login e2e test

This PR expands the previous test to also
include a login scenario. In addtion, the asserts are
improved to now check that the temp key / passkey is listed
in the correct section.

* Add separate recovery section
The recovery phrase warning banner is no longer justified as the
domain migration has been put on hold. In addition, a bug (the banner
being added twice) has been reported.

The simplest solution to address this is to simply remove the banner.
There are other nag screens / warnings still in place.
gix-bot and others added 19 commits September 19, 2023 11:16
* clean up  pinInput component CSS

* change the semantics of pinInfo

* add more space on top of button

* make the mainwindow narrow instead of restricting its content using max-width

* re-add missing error color on pin

* add a bunch of spaces before buttons

* adds icon to temporary keys

* authenticatorItem now takes an optional icon
When deleting the temporary key from the identity, the key remains
in browser storage. This leads to the awkward situation of the browser
still prompting for the PIN even if subsequent authentication will fail.

This PR introduces a check, that the browser will only use the PIN protected
key if the public key is still present on the identity.
Previously, if the pin on the confirmation page did not
match the previous input the only option was to cancel and
start over. This is a very bad user experience.

The behaviour is replaced with a mechanism that replaces the
cancel button with a retry button that sends the user to the
previous page.
* Add more e2e test for non-passkey auth

This PR adds more e2e tests for the PIN auth feature:
* Login attempt with wrong PIN first
* PIN registration during auth flow
* auth into client application after PIN registration

* Implement review feedback

* Extract wrongPin variable
Extends the `warnBox` helper with a slot to add some template
next to the exclamation mark icon.

The PR will make the helper usable in more contexts relate to
the non-passkey auth feature.
This PR replaces the custom markup of the temp key warning with
the `warnBox` helper.
This allows using the temp key warning in contexts that
do not offer a button / action to take.
* Add temp key warning to registration success screen

This adds the temp key warning also to the registration success page.

* Rename slot to marketingIntroSlot
The chrome version on the GitHub runners has changed. We need to update
the chromedriver accordingly.
This PR changes the recovery method card to be consistent with
the passkeys and temp keys cards:
* always show card border
* have a small explanatory text below the title
Revert "Update rust version (#1919)"

This reverts commit 7c57602.
This PR extracts a flow for recovery nag skipping in the e2e tests,
thus simplifying the code.
This makes CI check the `verify-hash` script so that we get notified
if we break it.
It is not added to the canister tests workflow because it would increase
runtime.
This extracts the common recovery with seed phrase flow for the e2e tests.
This refactoring is done in preparation to changes to that flow
(so it requires change in only one place).
* Improve confusing comments

This PR improves confusing comments surfaced in #1926.

* Improve comment
Copy link
Collaborator

@przydatek przydatek left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@frederikrothenberger frederikrothenberger merged commit cc8b1d9 into vc-mvp-2 Sep 25, 2023
@frederikrothenberger frederikrothenberger deleted the frederik/merge-main branch September 25, 2023 15:09
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.

4 participants