Skip to content
This repository has been archived by the owner on Jun 6, 2024. It is now read-only.

Add ssh public keys on user-profile page #5223

Merged
merged 8 commits into from
Feb 4, 2021
Merged

Conversation

Lijiaoa
Copy link
Contributor

@Lijiaoa Lijiaoa commented Jan 5, 2021

TODO:

  • check input format together

image

image

image

improve SSH layout:

image

@coveralls
Copy link

coveralls commented Jan 5, 2021

Coverage Status

Coverage remained the same at 33.896% when pulling 35ce9f5 on Lijiaoa:dev-ssh into 65df0d7 on microsoft:master.

@@ -0,0 +1,159 @@
// Copyright (c) Microsoft Corporation
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use this header:

// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will modifiy it. And looks like that still have many files write that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, they are the old headers. We haven't modified them. This one is the new header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

</Hint>
<Stack horizontal gap='l1'>
<TextField
Lable='Add additional ssh public key'
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change ssh to SSH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

<Stack horizontal gap='l1'>
<TextField
Lable='Add additional ssh public key'
placeholder='Additional ssh public key'
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change ssh to SSH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@@ -210,6 +250,29 @@ const UserProfile = () => {
>
<TokenList tokens={tokens} onRevoke={onRevokeToken} />
</UserProfileCard>
<UserProfileCard
Copy link
Contributor

@hzy46 hzy46 Feb 3, 2021

Choose a reason for hiding this comment

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

Please move this UserProfileCard of SSH Public Keys before the UserProfileCard of Tokens.

SSH is supposed to be used more frequently than tokens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

}}
minWidth={400}
>
<div>Are you sure you want to delete this ssh key?</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change ssh to SSH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

if (title.trim() === '') {
setInputTitleError('Please input title');
} else if (value.trim() === '') {
setInputValueError('Please input ssk value');
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change ssk to SSH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

setInputTitleError('Please input title');
} else if (value.trim() === '') {
setInputValueError('Please input ssk value');
} else {
Copy link
Contributor

@hzy46 hzy46 Feb 3, 2021

Choose a reason for hiding this comment

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

Please validate the value matches SSH key's format.

You can use this regex: /^ssh-rsa AAAA[0-9A-Za-z+/]+[=]{0,3}.*$/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// click `add public ssh keys button` -> open dialog
const onAddPublicKeys = useCallback(async sshPublicKeys => {
setSSHProcessing(true);
let updatedSSHPublickeys = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Please validate here: the user cannot add ssh keys with duplicate titles. (Title should be unique for one user)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}
updatedSSHPublickeys = updatedSSHPublickeys.filter(
item =>
item.title !== sshPublicKeys.title &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Please only filter with item.title !== sshPublicKeys.title.

Title should be unique and delete means delete an ssh key with a certain title.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

<div>
<div className={t.mt1}>
<TextField
label='title'
Copy link
Contributor

@hzy46 hzy46 Feb 3, 2021

Choose a reason for hiding this comment

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

Better to give the user some hints.

title -> Title (Please give the SSH key a name):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

</div>
<div className={t.mt1}>
<TextField
label='value'
Copy link
Contributor

@hzy46 hzy46 Feb 3, 2021

Choose a reason for hiding this comment

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

Better to give the user some hints.

value -> Value (SSH Public key, starts with ssh-rsa):

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

data: {
username: username,
extension: {
sshKeys: sskMessage,
Copy link
Contributor

Choose a reason for hiding this comment

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

sskMessage -> sshMessage?

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

@@ -179,6 +179,26 @@ export const getUserRequest = async username => {
});
};

export const updateUserRequest = async (username, sskMessage) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

sskMessage -> sshMessage?

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

@Lijiaoa Lijiaoa closed this Feb 4, 2021
@Lijiaoa Lijiaoa reopened this Feb 4, 2021
}
}

// if (title.trim() === '') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove these comments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@hzy46 hzy46 merged commit 7008caa into microsoft:master Feb 4, 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.

3 participants