Conversation
Co-authored-by: Sofi <imsofi@users.noreply.github.com>
Original project idea did not turn out as expected. So we are resetting the repo to start on a new idea.
- Cleaned up 'hello world' code - Added prompt-toolkit to poetry setup - Created minimal TUI for grand geckos.
- Create functions `generate_password` and `generate_passphrase` - Fully test functions `generate_password` and `generate_passphrase` - Enable Github workflow for automated testing - Update pre-commit hook to ignore tests and static files - Add pytest as dev dependency
Added password generation functions
Merge pull request #16 from imsofi/password-strength
Add SQLAlchemy to the poetry enviroment, and create `User` `Credential`, and `Platform` Tables.
Add encryption, and create back reference to User object in `Credential` table. Create custom exceptions, errors to specifiy custom DB related ones.
Add `Auth` and `Delete` methods to DBWorker. Add additional DB enhancements, new DB related exceptions.
Integrate the DB and Operations into the UI - Rework the Login/Register Panel - Add dynamic data from the DB to the layout
- Add main menu: unlock the vault, or add new Credential - Fix Layout, and give vault key to decrypt data when the vault is opened to show dynamically the data. - Add password strength check on Registration, and show useful information if the password is weak.
Fix problem when user could not return to the main menu after canceling certain actions
From now on, users can decide to use their passwords regardless of password-strength instead of using a strong password that passes the checks
From now on when the user gets into the vault they can navigate with TAB between the Password fields making it easier to navigate by keyboard
From now on the user will be asked whether they want a strong Password generated by the app or want to supply their own password
Add team logo images
Added poetry troubleshooting note to README
fisher60
left a comment
There was a problem hiding this comment.
Howdy, howdy. Overall I really loved this project. I feel the UI was done really well. The UI was well organized, easy and clear to navigate, and just overall a nice experience. Your team appears to have set a reasonable and tangible goal and executed it nicely. Your project appears very complete and well thought out.
Readme
Your readme is quite nice for getting the project set up. It contains all the info needed to start up the project. I would have loved to see some examples (with screenshots) of the project though.
Use of Python and Prompt Toolkit
Overall the quality of code ranged from really nice to a bit less nice. There were several modules that were missing docstrings altogether, as well as type and return hints. This type of documentation contributes very largely to code readability and should not be skipped over.
Some functions were quite large as well. This resulted in very unreadable code that became a bit spaghetti-like. Sometimes it is more important to write clear and readable code than writing compact and concise code. This is especially true in a team environment where it is possible that most team members are not familiar with 100% of the codebase.
You seemed to make really good use of python prompt toolkit. The UI was very clean and very functional. I did see some warnings for arguments not matching type-hints, which makes me feel the module may have been fought a bit. But overall it worked out quite nicely it appears.
Commit quality
For the most part the commit messages are ok. Most of them are missing bodies, but the titles are fairly descriptive. Commit bodies can be incredibly important, especially in a team environment since it helps track changes and also often offers insights for why a change occurred. The reasoning for a change should not be in a commit title, but adding reasoning can be very helpful in a commit. Clarifying the details of a change is also very important.
Style
Your code style was overall decent. Flake8 is almost a complete pass, except for one file. The most impactful changes, in my opinion, would be to break up some of the larger functions, adding logical line breaks, more explicit variable naming, and choosing to define more variables in some places, rather than a lot of additional definitions as function arguments. (pointed out in the more detailed code review).
Things you did well
- Some really great docstrings, good use of type-hints, and return-hints where they were used.
- Logical project structure, logically separating modules into an organized directory structure
- Setting a project goal and achieving it. Your project appears very complete, not at all as if it was written in one week for a code jam.
Things that could use improvement
- Docstrings, type-hints, and return-hints were completely missing from some modules.
- Large functions with little to no logical line breaks and a lot of larger definitions in function args made the code fairly unreadable in some places.
- Some duplicate code should have been defined as a function
- Commit bodies are very sparse, would like to see more of these. It can do a lot to help manage a project like this. Sometimes it is not worth it to directly communicate with a teammate asking
why was this implementedorhow was this implementedwhen a commit body could have answered this question easily.
| engine = create_engine("sqlite:///worker.db") | ||
|
|
||
| def __init__(self, user: User): | ||
| self.session = sessionmaker(bind=DatabaseWorker.engine)() |
There was a problem hiding this comment.
This seems it would be better off returned from a function. It is used multiple times, writing a function for it would keep it DRY and make it easier to change if needed.
| session.commit() | ||
| return cls(user) | ||
|
|
||
| def vault_key(self, user: User, password: str) -> Fernet: |
There was a problem hiding this comment.
This might get down more to preference, but generally this should be defined with @staticmethod if left in the class for organization/structure, or defined outside of the class.
| kdf = PBKDF2HMAC(algorithm=hashes.SHA256(), length=32, salt=salt, iterations=10000) | ||
| key = urlsafe_b64encode(kdf.derive((password.encode("utf-8")))) | ||
| f = Fernet(key) | ||
| return f |
There was a problem hiding this comment.
The variable naming here should probably be more explicit. I am not very versed in cryptography and when working in a team environment it is usually more important to communicate clearly, rather than just concisely. It would be very helpful for someone else reading this code to be able to mostly understand what a variable is just from it's name (if possible), rather than needing to be familiar with key terms in order to decipher an abbreviation.
| self.session.commit() | ||
|
|
||
| def delete_credentials(self, ids: List[int], user: User): | ||
| for id in ids: |
There was a problem hiding this comment.
Should avoid using built-in names as variable names.
| salt = urlsafe_b64decode(user.salt) | ||
| kdf = PBKDF2HMAC(algorithm=hashes.SHA256(), length=32, salt=salt, iterations=10000) | ||
| key = urlsafe_b64encode(kdf.derive((password.encode("utf-8")))) | ||
| f = Fernet(key) |
There was a problem hiding this comment.
This should utilize the method defined above that does this thing.
| raise Exception("No groups to generate a password with!") | ||
|
|
||
| possible_chars = "".join(groups) | ||
| while True: |
There was a problem hiding this comment.
Unnecessary while True should be avoided. Could maybe be rewritten as
password = ""
# Check that there is at least one character from each selected group.
while not all(any(char in password for char in group) for group in groups):
password = "".join(secrets.choice(possible_chars) for _ in range(length))
return password
```| from grand_geckos.utils import check_password | ||
| from password_strength import tests | ||
|
|
||
| import pytest |
There was a problem hiding this comment.
unnecessary import and imported in the incorrect order.
|
|
||
| import pytest | ||
|
|
||
| class TestCheckPassword: |
There was a problem hiding this comment.
Fails flake8, missing one additional blank line before class definition.
|
|
||
| def test_good_passwords(self): | ||
| assert len(check_password("Gr@atP@ssw0rd")) == 0 | ||
| # TODO: Add more tests for good/bad passwords to help refine the checker |
There was a problem hiding this comment.
Purely preference, but in a team environment I would never recommend putting todo comments in code. If something is important enough to do, then it should have an issue created on Github and be properly documented. Todo comments like this have a tendency to be overlooked and ignored. It is unclear who is supposed to do this, why, and what exactly needs to be done.
| generate_password(length=0, | ||
| use_letters=True, | ||
| use_numbers=True, | ||
| use_symbols=False, | ||
| custom_letters="") |
There was a problem hiding this comment.
This is a bit ugly imo, would prefer to see it formatted like such:
generate_password(
length=2,
use_letters=True,
use_numbers=True,
use_symbols=False,
custom_letters=""
)
No description provided.