Skip to content

feat: Transfer ownership #396

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

feat: Transfer ownership #396

wants to merge 4 commits into from

Conversation

LazyCat2
Copy link

@LazyCat2 LazyCat2 commented Mar 3, 2025

Please make sure to check the following tasks before opening and submitting a PR

  • I understand and have followed the contribution guide
  • I have tested my changes locally and they are working as intended

Closes #196
RFC PR: #14

Screenshot with curl commands that uses this feature

Signed-off-by: LazyCat2 <68156188+LazyCat2@users.noreply.github.com>
MCausc78 added a commit to MCausc78/pyvolt that referenced this pull request Mar 3, 2025
Signed-off-by: LazyCat2 <68156188+LazyCat2@users.noreply.github.com>
@github-project-automation github-project-automation bot moved this from 🆕 Untriaged to 💡 Ready to merge in Pull Request Overview Mar 7, 2025
@StupidRepo
Copy link
Contributor

StupidRepo commented Mar 7, 2025

As mentioned in the comment I put in the RFC PR, it would be better to check if the user has 2FA enabled before allowing them to transfer servers. That way any account which does not have 2FA enabled will not be allowed to transfer servers, preventing future abuse/malicious activity.

@LazyCat2
Copy link
Author

LazyCat2 commented Mar 7, 2025

As mentioned in the comment I put in the RFC PR, it would be better to check if the user has 2FA enabled before allowing them to transfer servers. That way any account which does not have 2FA enabled will not be allowed to transfer servers, preventing future abuse/malicious activity.

If hacker gets access to account, they can just enable mfa, if it is not enabled already, so i do not think this check will add any security

@StupidRepo
Copy link
Contributor

StupidRepo commented Mar 7, 2025

If hacker gets access to account, they can just enable mfa

You need the account's password to enable 2FA/MFA. If a hacker gains access to an account, it would most likely be via token rather than password. How can someone enable 2FA if they don't have the password?

@LazyCat2
Copy link
Author

LazyCat2 commented Mar 7, 2025

If hacker gets access to account, they can just enable mfa

You need the account's password to enable 2FA/MFA. If a hacker gains access to an account, it would most likely be via token rather than password. How can someone enable 2FA if they don't have the password?

Ill try to add it later then

@StupidRepo
Copy link
Contributor

If hacker gets access to account, they can just enable mfa

You need the account's password to enable 2FA/MFA. If a hacker gains access to an account, it would most likely be via token rather than password. How can someone enable 2FA if they don't have the password?

Ill try to add it later then

👍🏻

Signed-off-by: LazyCat2 <68156188+LazyCat2@users.noreply.github.com>
@insertish insertish moved this from 💡 Ready to merge to 🆕 Untriaged in Pull Request Overview Mar 9, 2025
@LazyCat2
Copy link
Author

LazyCat2 commented Mar 9, 2025

image
seems to work I think

@LazyCat2 LazyCat2 marked this pull request as ready for review March 9, 2025 17:13
Signed-off-by: LazyCat2 <68156188+LazyCat2@users.noreply.github.com>
@github-project-automation github-project-automation bot moved this from 🆕 Untriaged to 💡 Ready to merge in Pull Request Overview Apr 2, 2025
@insertish insertish marked this pull request as draft April 2, 2025 10:57
@insertish
Copy link
Member

Just going to draft this while RFC is pending.

@insertish insertish assigned insertish and unassigned insertish Apr 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 💡 Ready to merge
Development

Successfully merging this pull request may close these issues.

feature request: Server Ownership Transfer
5 participants