-
Notifications
You must be signed in to change notification settings - Fork 1.9k
test: Shifted admin check function and added integration test #6081
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
Conversation
0c1d08a to
68f2cd6
Compare
| logout_user() | ||
| self.assertEqual(auth_manager.is_accessible(), False) | ||
|
|
||
| def test_check_auth_admin(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you should also test with admin=False
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can create two users and one admin and other non-admin and this should give correct response for each of them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@poush Made the requested changes. Please have a look
|
@poush Will do that :)
…On Thu, 20 Jun 2019 at 12:41 PM, Piyush Agrawal ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In app/api/helpers/auth.py
<#6081 (comment)>
:
> @@ -31,3 +31,14 @@ def is_verified_user():
@staticmethod
def is_accessible():
return current_user.is_authenticated
+
+ @staticmethod
+ def check_auth_admin(username, password):
+ # This function is called to check for proper authentication & admin rights
+ if username and password:
+ user = User.query.filter_by(_email=username).first()
+ if user:
+ if user.is_correct_password(password):
+ if user.is_admin:
Hi @mrsaicharan1 <https://github.com/mrsaicharan1>, I think its better
that lines 40, 41, 41 be reduced to only 1
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6081?email_source=notifications&email_token=AGAHUW25H7RCWMJURRM77QLP3MUTVA5CNFSM4HZQCQGKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB4DRW6A#pullrequestreview-252124024>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGAHUW2FNYDV5CGY5XFGHUDP3MUTVANCNFSM4HZQCQGA>
.
|
|
Yeah two test cases. Agreed
…On Thu, 20 Jun 2019 at 12:51 PM, Shreyansh Dwivedi ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In app/api/helpers/auth.py
<#6081 (comment)>
:
> @@ -31,3 +31,14 @@ def is_verified_user():
@staticmethod
def is_accessible():
return current_user.is_authenticated
+
+ @staticmethod
+ def check_auth_admin(username, password):
+ # This function is called to check for proper authentication & admin rights
+ if username and password:
+ user = User.query.filter_by(_email=username).first()
+ if user:
+ if user.is_correct_password(password):
+ if user.is_admin:
@mrsaicharan1 <https://github.com/mrsaicharan1> yes please use and and
combine the logics
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6081?email_source=notifications&email_token=AGAHUW42G32G6L7VNJZSYATP3MVW7A5CNFSM4HZQCQGKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB4DSRCA#discussion_r295673838>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AGAHUW2MZTCP546JBGYKU4TP3MVW7ANCNFSM4HZQCQGA>
.
|
Codecov Report
@@ Coverage Diff @@
## development #6081 +/- ##
==============================================
Coverage ? 66.17%
==============================================
Files ? 285
Lines ? 14133
Branches ? 0
==============================================
Hits ? 9353
Misses ? 4780
Partials ? 0
Continue to review full report at Codecov.
|
68f2cd6 to
4f40630
Compare
dd7bc47 to
73b35af
Compare
73b35af to
40d7e5a
Compare
f95dba7 to
241ba79
Compare
Hound fixes Extend test case to non-admin & reduce LOC Update tests/all/integration/api/helpers/test_auth.py
241ba79 to
146bad8
Compare
|
@shreyanshdwivedi @uds5501 @poush @iamareebjamal Made the suggested changes. Please have a look now. |
Part of #5320
Short description of what this resolves:
This moves the admin rights function to the helpers and adds a test to check it
Changes proposed in this pull request:
Checklist
developmentbranch.