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

[bug] unexpected freeradius authorize failed #150

Merged
merged 1 commit into from Aug 25, 2020

Conversation

atb00ker
Copy link
Member

No description provided.

@@ -191,7 +191,7 @@ def get_user(self, request):
# ensure user is member of the authenticated org
# or RadiusToken for the user exists.
if (
RadiusToken.objects.filter(user=user, can_auth=True).exists()
RadiusToken.objects.filter(user=user).exists()
Copy link
Member Author

@atb00ker atb00ker Aug 13, 2020

Choose a reason for hiding this comment

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

Why can_auth is removed here:

Send auth with "username=admin&password=admin"

Success

Send auth with "username=admin&password=radtoken"

Success

Send auth with "username=admin&password=admin"

Failed Unexpectedly

Why? Because can_auth=True is false now!

Fix: remove can_auth from get_user() and only keep it in check_user_token()

Copy link
Member

Choose a reason for hiding this comment

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

can you please add a test for this bug? Should be quick.

@atb00ker atb00ker added this to In progress in [GSoC20] Merge modules via automation Aug 13, 2020
@atb00ker atb00ker added the bug Something isn't working label Aug 13, 2020
@atb00ker atb00ker self-assigned this Aug 13, 2020
@atb00ker atb00ker moved this from In progress to Ready for review in [GSoC20] Merge modules Aug 13, 2020
@coveralls
Copy link

coveralls commented Aug 13, 2020

Pull Request Test Coverage Report for Build 576

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.316%

Totals Coverage Status
Change from base Build 574: 0.0%
Covered Lines: 2179
Relevant Lines: 2194

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 562

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.316%

Totals Coverage Status
Change from base Build 555: 0.0%
Covered Lines: 2179
Relevant Lines: 2194

💛 - Coveralls

@@ -191,7 +191,7 @@ def get_user(self, request):
# ensure user is member of the authenticated org
# or RadiusToken for the user exists.
if (
RadiusToken.objects.filter(user=user, can_auth=True).exists()
RadiusToken.objects.filter(user=user).exists()
Copy link
Member

Choose a reason for hiding this comment

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

can you please add a test for this bug? Should be quick.

[GSoC20] Merge modules automation moved this from Ready for review to Open Pull-Requests Aug 13, 2020
@atb00ker atb00ker force-pushed the minor-fix-can-auth branch 3 times, most recently from c0ae381 to 2dbba63 Compare August 14, 2020 05:51
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Maybe this PR needs to be rebased to fix the failing build.

@atb00ker atb00ker moved this from Open Pull-Requests to Ready for review in [GSoC20] Merge modules Aug 20, 2020
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Thanks!

@nemesifier nemesifier merged commit dde6517 into openwisp:master Aug 25, 2020
[GSoC20] Merge modules automation moved this from Ready for review to Done Aug 25, 2020
@atb00ker atb00ker deleted the minor-fix-can-auth branch September 14, 2020 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants