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

Implements TOTP API #873

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

Implements TOTP API #873

wants to merge 4 commits into from

Conversation

xunzhou
Copy link

@xunzhou xunzhou commented Sep 12, 2022 •
edited by briantist

Implements TOTP API as in https://www.vaultproject.io/api/secret/totp
Including Tests and Docs

Closes #791

@xunzhou xunzhou requested a review from a team as a code owner September 12, 2022 17:09
colin-pm
colin-pm reviewed Sep 12, 2022
View reviewed changes
docs/usage/secrets_engines/totp.rst Outdated
'mykey',
url="otpauth://totp/Google:test@gmail.com?secret=Y64VEVMBTSXCYIWRSHRNDZW62MPGVU2G&issuer=Google"
)
print('New TOTP key response: {}'.format(create_key_response))
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: To simplify the docs a little bit, do you mind using f-strings in your examples? Since the next release will be 3.6 and greater, I'd like for HVAC to start using modern Python as much as possible.

print(f'New TOTP key response: {create_key_response}')

colin-pm
colin-pm requested changes Sep 12, 2022
View reviewed changes
Copy link
Member

@colin-pm colin-pm left a comment

Choose a reason for hiding this comment

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

Overall, the change looks good to me. I suggested several things that I would like to be addressed before approving.

Thanks again for working on adding support for this secrets engine!

hvac/constants/totp.py Outdated
DEFAULT_MOUNT_POINT = "totp"
ALLOWED_ALGORITHMS = ['SHA1', 'SHA256', 'SHA512']
ALLOWED_DIGITS = [6, 8]
ALLOWED_SKEW = [0, 1]
Copy link
Member

Choose a reason for hiding this comment

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

Add a newline at the end of the file here

hvac/api/secrets_engines/totp.py Outdated
)

return self._adapter.get(url=api_path)
pass
Copy link
Member

Choose a reason for hiding this comment

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

question: What is the pass for here?

hvac/api/secrets_engines/totp.py Outdated

if generate:
if issuer == "" or account_name == "":
error_msg = 'required issuer and account_name when generate is true, got "{issuer}", "{account_name}"'
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: format would no longer be necessary if f-strings were used on this line and the other error_msg lines below.

hvac/api/secrets_engines/totp.py Outdated
)
)
else:
if url == "":
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: I think it would be better to evaluate url for truthiness here.

if not url:

tests/integration_tests/api/secrets_engines/test_totp.py
from unittest import TestCase
from hvac.api.system_backend import mount

from parameterized import parameterized, param
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: Import order should follow PEP-8

  1. Standard library imports.
  2. Related third party imports.
  3. Local application/library specific imports.

https://peps.python.org/pep-0008/#imports

parameterized should go between lines 2 and 3. The hvac imports should be grouped together. All groups should be separated by one space.

Copy link
Author

Choose a reason for hiding this comment

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

mount auto imported, not needed, removed

@codecov
Copy link

codecov bot commented Sep 12, 2022 •
edited

Codecov Report

Merging #873 (76d0fc1) into main (31aca14) will decrease coverage by 4.20%.
The diff coverage is 96.07%.

❗ Current head 76d0fc1 differs from pull request most recent head d79410a. Consider uploading reports for the commit d79410a to get more accurate results

@@            Coverage Diff             @@
##             main     #873      +/-   ##
==========================================
- Coverage   81.98%   77.78%   -4.20%     
==========================================
  Files          65       66       +1     
  Lines        3019     3587     +568     
==========================================
+ Hits         2475     2790     +315     
- Misses        544      797     +253
Impacted Files Coverage Δ
hvac/api/secrets_engines/totp.py 95.55% <95.55%> (ø)
hvac/api/secrets_engines/__init__.py 100.00% <100.00%> (ø)
hvac/constants/totp.py 100.00% <100.00%> (ø)

... and 21 files with indirect coverage changes

@xunzhou
Copy link
Author

xunzhou commented Sep 12, 2022

all suggestions should be addressed 76d0fc1

@xunzhou xunzhou mentioned this pull request Sep 12, 2022
Open
@colin-pm
Copy link
Member

This PR needs to be formatted with Black. I would advise an interactive rebase of your commits, running black for each commit.

@briantist briantist force-pushed the develop branch 2 times, most recently from 085b858 to 9dbfa98 Compare May 10, 2023 06:41
@briantist briantist changed the base branch from develop to main June 17, 2023 21:47
@briantist
Copy link
Contributor

Hi @xunzhou ,

Similar to #874:

The hvac project is simplifying its developer workflow and removing the develop branch. Please be aware for future PRs that the project's default branch is now main.

Since your PR was opened before this change, I have:

  • updated your PR to merge into the project's main branch
  • rebased your branch against main and force-pushed it (please pull before making any further changes)
    (there were no conflicts so I was able to update the branch for you)

This PR and the other has been waiting on changes for a while, please let us know if you still interested in finishing them up!

@briantist briantist added waiting-reply waiting for more information (probably for a while) secrets engines generally related to a Vault secrets engine totp Time-based one-time password secrets engine labels Jun 17, 2023
@briantist briantist mentioned this pull request Jun 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@colin-pm colin-pm colin-pm requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
secrets engines generally related to a Vault secrets engine totp Time-based one-time password secrets engine waiting-reply waiting for more information (probably for a while)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@xunzhou @colin-pm @briantist