-
Notifications
You must be signed in to change notification settings - Fork 633
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
Bootstrap remote access data into page #7015
Conversation
Set default conditional on app plugin being enabled.
Codecov Report
|
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 have just left a small optimization that could be applied to the code. Appart from t hat, everything is fine from my point of view.
My only concern is that we don't have tests for this behaviour and it's complex enough to deserve them (big problems to the users can appear if it does not work properly). If you don't add them here, please, create an issue so we don't forget them.
Thanks.
kolibri/core/auth/api.py
Outdated
@@ -494,6 +495,14 @@ def create(self, request): | |||
password = request.data.get("password", "") | |||
facility_id = request.data.get("facility", None) | |||
|
|||
if not allow_other_browsers_to_connect() and not valid_app_key_on_request( |
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.
Just a simple optimization for the cases where the app plugin is not enabled:
+from kolibri.core.device.models import app_is_enabled
class KolibriAuthPermissionsFilter(filters.BaseFilterBackend):
@@ -495,7 +496,7 @@ class SessionViewSet(viewsets.ViewSet):
password = request.data.get("password", "")
facility_id = request.data.get("facility", None)
- if not allow_other_browsers_to_connect() and not valid_app_key_on_request(
+ if app_is_enabled() and not allow_other_browsers_to_connect() and not valid_app_key_on_request(
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.
Yep, I had also just noticed this - important for making sure this setting is a noop for non-app Kolibris!
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.
Updated!
Follow up issue here: #7019 |
Summary
Reviewer guidance
Does a non-app context properly get blocked by the remote access setting?
Should we change the wording of remote access to make clear this is for non-app access? If not, we should probably change the restrictions to be based on IP address (127.0.0.1 rather than app-context).
References
Fixes #6998, resolves #7012
Contributor Checklist
PR process:
Testing:
Reviewer Checklist
yarn
andpip
)