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.

Sign up for GitHub

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

Jump to bottom

fix(webauthn): delete doesn't require challenge and use enroll wrapper #10084

Merged
merged 1 commit into from Oct 10, 2023
Merged

fix(webauthn): delete doesn't require challenge and use enroll wrapper #10084

merged 1 commit into from Oct 10, 2023

Conversation

Alkorin
Copy link
Member

@Alkorin Alkorin commented Oct 8, 2023 •
edited

Question Answer
Branch? master / release/** / develop
Bug fix? yes
New feature? no
Breaking change? no
License BSD 3-Clause
  • Try to keep pull requests small so they can be easily reviewed.
  • Commits are signed-off
  • Only FR translations have been updated
  • Branch is up-to-date with target branch
  • Lint has passed locally
  • Standalone app was ran and tested locally
  • Ticket reference is mentioned in linked commits (internal only)
  • Breaking change is mentioned in relevant commits

Description

Fix security keys

Related

@Alkorin Alkorin requested a review from a team as a code owner October 8, 2023 20:53
@Alkorin Alkorin force-pushed the dev/tsoete/webauthn branch 3 times, most recently from f96c55d to ba2e1ff Compare October 9, 2023 10:29
This was linked to issues Oct 9, 2023
U2F Support on safari #4024
Open
2FA Hardware keys cannot be added on macOS #9632
Open
Cannot remove U2F security key #9705
Closed
lizardK
lizardK previously approved these changes Oct 9, 2023
View reviewed changes
lizardK
lizardK previously approved these changes Oct 9, 2023
View reviewed changes
darsene
darsene previously approved these changes Oct 9, 2023
View reviewed changes
@Alkorin Alkorin dismissed stale reviews from darsene and lizardK via 405ae24 October 9, 2023 15:51
anooparveti
anooparveti reviewed Oct 10, 2023
View reviewed changes
...s/manager/apps/dedicated/client/app/account/user/security/u2f/user-security-u2f.constants.js Outdated
US: 'https://auth.us.ovhcloud.com/webauthn/enroll',
};

export default WEBAUTHN_URL;
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: Export an object, so that we can add more properties in future.

Suggested change
export default WEBAUTHN_URL;
export default {
WEBAUTHN_URL,
};

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

anooparveti
anooparveti reviewed Oct 10, 2023
View reviewed changes
...dedicated/client/app/account/user/security/u2f/delete/user-security-u2f-delete.controller.js Outdated
$scope.u2f.isLoading = false;
});
};
$scope.init = () => {};
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: Empty function can be removed.
Also don't forget to remove it from Line number 10 on packages/manager/apps/dedicated/client/app/account/user/security/u2f/delete/user-security-u2f-delete.html

Suggested change
$scope.init = () => {};

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@Alkorin
fix(webauthn): delete doesn't require challenge and use enroll wrapper
6a34dbc 
Signed-off-by: Thomas SOËTE <thomas.soete@ovhcloud.com>
@sonarcloud
Copy link

sonarcloud bot commented Oct 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ghyenne ghyenne changed the base branch from master to release/incidents-w41 October 10, 2023 11:49
@ghyenne ghyenne merged commit 78b216f into release/incidents-w41 Oct 10, 2023
15 of 16 checks passed
@ghyenne ghyenne deleted the dev/tsoete/webauthn branch October 10, 2023 11:57
@ghyenne ghyenne mentioned this pull request Oct 10, 2023
Merged
6 tasks
@jonathandhn
Copy link

Hi, i just want to let you know that iCloud is showing twice under the same name at the login prompt, and a single time at the dashboard/security panel view, and as you now have to select the right key at the login prompt, it makes unpredictable iCloud selection (iCloud 1 : Browser prompt key or QR code, iCloud 2 : Browser prompt in device passkey), device name is not enough, it misses like parenthesis next to with the workflow involved or better an icon to show us), at least internal auth / external auth

Capture d’écran 2023-11-07 à 14 34 21

@jonathandhn jonathandhn mentioned this pull request Nov 7, 2023
Open
@gegtor
Copy link

gegtor commented Nov 7, 2023 •
edited

Screenshot 2023-11-07 at 15 05 00

First time i tried registering a YubiKey it failed to register (account page with key registration didn't respond)
Then I tried a second time and it worked and I named it YubiKey 5C 1

Now two keys show up and one of them doesn't work

Account shows only 1 key and I can't delete that first one that doesn't work

@jonathandhn
Copy link

@gegtor weird, actually both 001 and 002 on my screenshot are Yubikey 5C NFC, You are allowed to rename your key on the dashboard / security ! Try to do that :)

@gegtor
Copy link

gegtor commented Nov 7, 2023

@gegtor weird, actually both 001 and 002 on my screenshot are Yubikey 5C NFC, You are allowed to rename your key on the dashboard / security ! Try to do that :)

It doesn't show up in dashboard but does while logging in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anooparveti anooparveti anooparveti left review comments

@ghyenne ghyenne ghyenne approved these changes

@darsene darsene darsene approved these changes

@lizardK lizardK lizardK left review comments

@antonymarion antonymarion Awaiting requested review from antonymarion antonymarion was automatically assigned from ovh/team-manager-dev

@sachinrameshn sachinrameshn Awaiting requested review from sachinrameshn sachinrameshn was automatically assigned from ovh/team-manager-dev

@BrunoMarquesOVH BrunoMarquesOVH Awaiting requested review from BrunoMarquesOVH BrunoMarquesOVH was automatically assigned from ovh/team-manager-dev

Assignees
No one assigned
Labels
universe-bare-metal-cloud
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2FA Hardware keys cannot be added on macOS U2F Support on safari Cannot remove U2F security key
7 participants
@Alkorin @jonathandhn @gegtor @lizardK @anooparveti @ghyenne @darsene