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.

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

feat: Added a password update route #15

Merged
merged 5 commits into from
Nov 7, 2020
Merged

feat: Added a password update route #15

merged 5 commits into from
Nov 7, 2020

Conversation

frgfm
Copy link
Member

@frgfm frgfm commented Nov 6, 2020

This PR adds the possibility of password update by:

  • renaming the existing /update-me route to /update-info for coherence
  • adding an admin and personal password update route. Both of them check if the entry exists, then hash the password and updates the entry in the table. For security reasons, instead of returning the input payload, I returned the user name to avoid exposing the password.
  • adding corresponding unittests
  • refactored schemas by adding a template class _CreatedAt and using multi-inheritance to avoid unneeded code duplication

I should bring your attention to route naming: I made the choice mentioned above but perhaps we should clarify the naming of all user routes since it's a complicated table.

Any feedback is welcome!

@frgfm frgfm added type: improvement New feature or request endpoint: users labels Nov 6, 2020
@frgfm frgfm added this to the 0.1.0 milestone Nov 6, 2020
@frgfm frgfm self-assigned this Nov 6, 2020
@frgfm frgfm linked an issue Nov 6, 2020 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Nov 6, 2020

Codecov Report

Merging #15 into master will increase coverage by 0.99%.
The diff coverage is 97.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #15      +/-   ##
==========================================
+ Coverage   89.27%   90.27%   +0.99%     
==========================================
  Files          16       16              
  Lines         457      442      -15     
==========================================
- Hits          408      399       -9     
+ Misses         49       43       -6     
Flag Coverage Δ
unittests 90.27% <97.14%> (+0.99%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/app/api/schemas.py 98.78% <94.73%> (+5.20%) ⬆️
src/app/api/routes/users.py 100.00% <100.00%> (ø)
src/app/api/routing.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c7e37c...7c84e7c. Read the comment docs.

@fe51
Copy link
Member

fe51 commented Nov 6, 2020

Hi there, thanks for the PR. (you are so fast I was willing to try this issue :D)

I review it soon and come back to you.

Copy link
Contributor

@florianriche florianriche left a comment

Choose a reason for hiding this comment

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

OK for me.

@@ -54,3 +54,14 @@ async def create_user(user_table: Table, payload: UserAuth):
pwd = await security.hash_password(payload.password)
payload = UserCreation(username=payload.username, hashed_password=pwd, scopes=payload.scopes)
return await create_entry(user_table, payload)


async def update_user_pwd(user_table: Table, payload: UserCred, entry_id: int = Path(..., gt=0)):
Copy link
Contributor

Choose a reason for hiding this comment

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

One should create another file to store those methods that do the interface between routing and crud. In my PR I added some lines in this file too. If this continue, the file will end up quite big.

@frgfm frgfm merged commit 1b3a031 into master Nov 7, 2020
@frgfm frgfm deleted the pwd-update branch November 7, 2020 01:10
@frgfm frgfm mentioned this pull request Mar 14, 2021
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a route for user password update
3 participants