-
Notifications
You must be signed in to change notification settings - Fork 11
feat: replace enterprise_support import with AccountSettingsReadOnlyF… #201
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
Changes from all commits
a7a5a6f
ce563a6
f9f072f
747934c
8ef0c30
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,7 +35,7 @@ def test_get_saml_idp_class_with_fake_identifier(self, log_mock): | |
|
|
||
| def test_get_user_details(self): | ||
| """ test get_attr and get_user_details of EdXSAMLIdentityProvider""" | ||
| edx_saml_identity_provider = EdXSAMLIdentityProvider('demo', **mock_conf) | ||
| edx_saml_identity_provider = EdXSAMLIdentityProvider(mock.Mock(), 'demo', **mock_conf) | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same social-auth-core 4.8.7 upgrade changed SAMLIdentityProvider.init from (self, entity_id, **kwargs) to (self, backend: BaseAuth, name: str, **kwargs). |
||
| assert edx_saml_identity_provider.get_user_details(mock_attributes) == expected_user_details | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -84,6 +84,7 @@ def get_env_setting(setting): | |||||||||||
| 'EVENT_BUS_PRODUCER_CONFIG', | ||||||||||||
| 'DEFAULT_FILE_STORAGE', | ||||||||||||
| 'STATICFILES_STORAGE', | ||||||||||||
| 'OPEN_EDX_FILTERS_CONFIG', | ||||||||||||
| ] | ||||||||||||
| }) | ||||||||||||
|
|
||||||||||||
|
|
@@ -281,6 +282,19 @@ def get_env_setting(setting): | |||||||||||
| EVENT_TRACKING_SEGMENTIO_EMIT_WHITELIST | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| # Merge OPEN_EDX_FILTERS_CONFIG from YAML into the default defined in common.py. | ||||||||||||
| # Pipeline steps from YAML are appended after steps defined in common.py. | ||||||||||||
| # The fail_silently value from YAML takes precedence over the one in common.py. | ||||||||||||
| for _filter_type, _filter_config in _YAML_TOKENS.get('OPEN_EDX_FILTERS_CONFIG', {}).items(): | ||||||||||||
|
||||||||||||
| for _filter_type, _filter_config in _YAML_TOKENS.get('OPEN_EDX_FILTERS_CONFIG', {}).items(): | |
| _open_edx_filters_config = _YAML_TOKENS.get('OPEN_EDX_FILTERS_CONFIG') or {} | |
| if not isinstance(_open_edx_filters_config, dict): | |
| raise ImproperlyConfigured('OPEN_EDX_FILTERS_CONFIG must be a dict') | |
| for _filter_type, _filter_config in _open_edx_filters_config.items(): |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |
| email_exists_or_retired, | ||
| username_exists_or_retired | ||
| ) | ||
| from common.djangoapps.third_party_auth.utils import get_saml_provider_for_user | ||
| from common.djangoapps.util.model_utils import emit_settings_changed_event | ||
| from common.djangoapps.util.password_policy_validators import validate_password | ||
| from lms.djangoapps.certificates.api import get_certificates_for_user | ||
|
|
@@ -38,9 +39,8 @@ | |
| from openedx.core.djangoapps.user_authn.utils import check_pwned_password | ||
| from openedx.core.djangoapps.user_authn.views.registration_form import validate_name, validate_username | ||
| from openedx.core.lib.api.view_utils import add_serializer_errors | ||
| from common.djangoapps.third_party_auth.utils import get_saml_provider_for_user | ||
| from openedx.features.enterprise_support.utils import get_enterprise_readonly_account_fields | ||
| from openedx.features.name_affirmation_api.utils import is_name_affirmation_installed | ||
| from openedx_filters.learning.filters import AccountSettingsReadOnlyFieldsRequested | ||
|
|
||
|
Comment on lines
39
to
44
|
||
| from .serializers import AccountLegacyProfileSerializer, AccountUserSerializer, UserReadOnlySerializer, _visible_fields | ||
|
|
||
|
|
@@ -194,11 +194,17 @@ def update_account_settings(requesting_user, update, username=None): | |
|
|
||
| def _validate_read_only_fields(user, data, field_errors): | ||
| # Check for fields that are not editable. Marking them read-only causes them to be ignored, but we wish to 400. | ||
| plugin_readonly_fields, __ = AccountSettingsReadOnlyFieldsRequested.run_filter( | ||
| readonly_fields=set(), | ||
| user=user, | ||
| ) | ||
|
Comment on lines
+197
to
+200
|
||
| plugin_readonly_fields = plugin_readonly_fields or set() | ||
|
|
||
| read_only_fields = set(data.keys()).intersection( | ||
| # Remove email since it is handled separately below when checking for changing_email. | ||
| (set(AccountUserSerializer.get_read_only_fields()) - {"email"}) | | ||
| set(AccountLegacyProfileSerializer.get_read_only_fields() or set()) | | ||
| get_enterprise_readonly_account_fields(user) | ||
| plugin_readonly_fields | ||
| ) | ||
|
Comment on lines
195
to
208
|
||
|
|
||
| for read_only_field in read_only_fields: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,6 @@ | |
| """ | ||
|
|
||
| import datetime | ||
| import itertools | ||
| import unicodedata | ||
| from unittest.mock import Mock, patch | ||
|
|
||
|
|
@@ -104,10 +103,12 @@ def setUp(self): | |
| self.staff_user = UserFactory(is_staff=True, password=self.password) | ||
| self.reset_tracker() | ||
|
|
||
| enterprise_patcher = patch('openedx.features.enterprise_support.api.enterprise_customer_for_request') | ||
| enterprise_learner_patcher = enterprise_patcher.start() | ||
| enterprise_learner_patcher.return_value = {} | ||
| self.addCleanup(enterprise_learner_patcher.stop) | ||
| filter_patcher = patch( | ||
| 'openedx.core.djangoapps.user_api.accounts.api.AccountSettingsReadOnlyFieldsRequested.run_filter', | ||
| return_value=(set(), None), | ||
| ) | ||
| filter_patcher.start() | ||
| self.addCleanup(filter_patcher.stop) | ||
|
|
||
| def test_get_username_provided(self): | ||
| """Test the difference in behavior when a username is supplied to get_account_settings.""" | ||
|
|
@@ -248,73 +249,19 @@ def test_update_success_for_enterprise(self): | |
| account_settings = get_account_settings(self.default_request)[0] | ||
| assert level_of_education == account_settings['level_of_education'] | ||
|
|
||
| @patch('openedx.features.enterprise_support.api.enterprise_customer_for_request') | ||
| @patch('openedx.features.enterprise_support.utils.third_party_auth.provider.Registry.get') | ||
| @ddt.data( | ||
| *itertools.product( | ||
| # field_name_value values | ||
| (("email", "[email protected]"), ("name", "new name"), ("country", "IN")), | ||
| # is_enterprise_user | ||
| (True, False), | ||
| # is_synch_learner_profile_data | ||
| (True, False), | ||
| # has `UserSocialAuth` record | ||
| (True, False), | ||
| ) | ||
| @patch( | ||
| 'openedx.core.djangoapps.user_api.accounts.api.AccountSettingsReadOnlyFieldsRequested.run_filter', | ||
| return_value=({'country'}, None), | ||
| ) | ||
| @ddt.unpack | ||
| def test_update_validation_error_for_enterprise( | ||
| self, | ||
| field_name_value, | ||
| is_enterprise_user, | ||
| is_synch_learner_profile_data, | ||
| has_user_social_auth_record, | ||
| mock_auth_provider, | ||
| mock_customer, | ||
| ): | ||
| idp_backend_name = 'tpa-saml' | ||
| mock_customer.return_value = {} | ||
| if is_enterprise_user: | ||
| mock_customer.return_value.update({ | ||
| 'uuid': 'real-ent-uuid', | ||
| 'name': 'Dummy Enterprise', | ||
| 'identity_provider': 'saml-ubc', | ||
| 'identity_providers': [ | ||
| { | ||
| "provider_id": "saml-ubc", | ||
| } | ||
| ], | ||
| }) | ||
| mock_auth_provider.return_value.sync_learner_profile_data = is_synch_learner_profile_data | ||
| mock_auth_provider.return_value.backend_name = idp_backend_name | ||
|
|
||
| update_data = {field_name_value[0]: field_name_value[1]} | ||
|
|
||
| user_fullname_editable = False | ||
| if has_user_social_auth_record: | ||
| UserSocialAuth.objects.create( | ||
| provider=idp_backend_name, | ||
| user=self.user | ||
| ) | ||
| else: | ||
| UserSocialAuth.objects.all().delete() | ||
| # user's fullname is editable if no `UserSocialAuth` record exists | ||
| user_fullname_editable = field_name_value[0] == 'name' | ||
|
|
||
| # prevent actual email change requests | ||
| with patch('openedx.core.djangoapps.user_api.accounts.api.student_views.do_email_change_request'): | ||
| # expect field un-editability only when all of the following conditions are met | ||
| if is_enterprise_user and is_synch_learner_profile_data and not user_fullname_editable: | ||
| with pytest.raises(AccountValidationError) as validation_error: | ||
| update_account_settings(self.user, update_data) | ||
| field_errors = validation_error.value.field_errors | ||
| assert 'This field is not editable via this API' == \ | ||
| field_errors[field_name_value[0]]['developer_message'] | ||
| else: | ||
| update_account_settings(self.user, update_data) | ||
| account_settings = get_account_settings(self.default_request)[0] | ||
| if field_name_value[0] != "email": | ||
| assert field_name_value[1] == account_settings[field_name_value[0]] | ||
| def test_readonly_field_from_filter_is_rejected(self, mock_run_filter): # pylint: disable=unused-argument | ||
| """ | ||
| When AccountSettingsReadOnlyFieldsRequested.run_filter returns a field as read-only, | ||
| update_account_settings should raise AccountValidationError for that field. | ||
| """ | ||
| with pytest.raises(AccountValidationError) as exc_info: | ||
| update_account_settings(self.user, {"country": "IN"}) | ||
| field_errors = exc_info.value.field_errors | ||
| assert 'This field is not editable via this API' == field_errors['country']['developer_message'] | ||
|
|
||
| def test_update_error_validating(self): | ||
| """Test that AccountValidationError is thrown if incorrect values are supplied.""" | ||
|
|
||
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.
The PR upgraded social-auth-core 4.7.0 → 4.8.7, which changed SAMLIdentityProvider.get_attr from (self, attributes, conf_key, default_attribute) (4 params) to (self, attributes, conf_key, default_attributes: tuple, *, validate_defaults=False) (5 params).