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

feat: Allow user verification through API request #7280

Merged
merged 4 commits into from
Sep 23, 2020
Merged

feat: Allow user verification through API request #7280

merged 4 commits into from
Sep 23, 2020

Conversation

mansiag
Copy link
Contributor

@mansiag mansiag commented Sep 21, 2020

Fixes #7278

Short description of what this resolves:

Allows admins to verify users through API request

Changes proposed in this pull request:

  • Remove is_verified field from being read-only
  • Add conditions to raise errors if anyone tries to post this field and if anyone other than admin tries to update this field

Checklist

  • I have read the Contribution & Best practices Guide and my PR follows them.
  • My branch is up-to-date with the Upstream development branch.
  • The unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • All the functions created/modified in this PR contain relevant docstrings.

houndci-bot
houndci-bot reviewed Sep 21, 2020
View reviewed changes
app/api/users.py Outdated
@@ -284,6 +289,12 @@ def before_update_object(self, user, data, view_kwargs):
{'source': ''}, "You are not authorized to update this information."
)

if not has_access('is_admin') and data.get('is_verified') is not None:
raise ForbiddenError(
{'pointer': '/data/attributes/is_verified'},
Copy link

Choose a reason for hiding this comment

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

trailing whitespace

app/api/users.py Outdated
@@ -67,6 +67,11 @@ def before_create_object(self, data, view_kwargs):
{'pointer': '/data/attributes/email'}, "Email already exists"
)

if data.get('is_verified') is not None:
raise UnprocessableEntityError(
{'pointer': '/data/attributes/is_verified'}, "You are not allowed to submit this field"
Copy link

Choose a reason for hiding this comment

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

line too long (103 > 90 characters)

@codecov
Copy link

codecov bot commented Sep 21, 2020 •
edited
Loading

Codecov Report

Merging #7280 into development will decrease coverage by 0.01%.
The diff coverage is 20.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #7280      +/-   ##
===============================================
- Coverage        63.56%   63.54%   -0.02%     
===============================================
  Files              259      259              
  Lines            13038    13042       +4     
===============================================
  Hits              8288     8288              
- Misses            4750     4754       +4
Impacted Files Coverage Δ
app/api/users.py 29.14% <0.00%> (-0.69%) ⬇️
app/api/schema/users.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 98a7563...0f128e6. Read the comment docs.

iamareebjamal
iamareebjamal reviewed Sep 21, 2020
View reviewed changes
app/api/users.py Outdated
@@ -67,6 +67,11 @@ def before_create_object(self, data, view_kwargs):
{'pointer': '/data/attributes/email'}, "Email already exists"
)

if data.get('is_verified') is not None:
Copy link
Member

Choose a reason for hiding this comment

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

This might be false and should still work

iamareebjamal
iamareebjamal reviewed Sep 21, 2020
View reviewed changes
app/api/users.py Outdated
@@ -284,6 +289,12 @@ def before_update_object(self, user, data, view_kwargs):
{'source': ''}, "You are not authorized to update this information."
)

if not has_access('is_admin') and data.get('is_verified') is not None:
Copy link
Member

Choose a reason for hiding this comment

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

If this is same as previous state, it should still work

iamareebjamal
iamareebjamal reviewed Sep 21, 2020
View reviewed changes
app/api/users.py Outdated
@@ -289,9 +290,9 @@ def before_update_object(self, user, data, view_kwargs):
{'source': ''}, "You are not authorized to update this information."
)

if not has_access('is_admin') and data.get('is_verified') is not None:
if not has_access('is_admin') and data.get('is_verified') != user.is_verified:
Copy link
Member

Choose a reason for hiding this comment

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

If a user did not submit is_verified, this will throw. This should only throw when user submits is_verified and it is different from current one

iamareebjamal
iamareebjamal reviewed Sep 22, 2020
View reviewed changes
app/api/users.py Outdated
not has_access('is_admin')
and data.get('is_verified') is not None
and data.get('is_verified') != user.is_verified
):
raise ForbiddenError(
{'pointer': '/data/attributes/is_verified'},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{'pointer': '/data/attributes/is_verified'},
{'pointer': '/data/attributes/is-verified'},

iamareebjamal
iamareebjamal reviewed Sep 22, 2020
View reviewed changes
app/api/users.py Outdated
@@ -67,6 +67,12 @@ def before_create_object(self, data, view_kwargs):
{'pointer': '/data/attributes/email'}, "Email already exists"
)

if data.get('is_verified'):
raise UnprocessableEntityError(
{'pointer': '/data/attributes/is_verified'},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{'pointer': '/data/attributes/is_verified'},
{'pointer': '/data/attributes/is-verified'},

@iamareebjamal iamareebjamal changed the title Feat: Allow user verification through API request feat: Allow user verification through API request Sep 22, 2020
@auto-label auto-label bot added the feature label Sep 22, 2020
iamareebjamal
iamareebjamal reviewed Sep 22, 2020
View reviewed changes
app/api/users.py
@@ -284,6 +290,16 @@ def before_update_object(self, user, data, view_kwargs):
{'source': ''}, "You are not authorized to update this information."
)

if (
not has_access('is_admin')
and data.get('is_verified') is not None
Copy link
Member

Choose a reason for hiding this comment

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

Now, there is one catch left. If the user is not admin, and they sent None, what will happen. Please check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

f the user sends None, then there will be a Validation error (Not a Valid Boolean), doesn't matter if the user is admin or not.
image

@iamareebjamal iamareebjamal merged commit 6c601a8 into fossasia:development Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@iamareebjamal iamareebjamal iamareebjamal approved these changes

Assignees
No one assigned
Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feat: Allow User Verification through API request
3 participants
@mansiag @iamareebjamal @houndci-bot