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

Changes for "can_request_access" feature #10877

Open
wants to merge 6 commits into
base: feature/institutional_access
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/institutions/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class InstitutionSerializer(JSONAPISerializer):
ser.CharField(read_only=True),
permission='view_institutional_metrics',
)
can_request_access = ser.CharField(read_only=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Make it clear this refers to a specific feature. Refactor to something like institutional_request_access_enabled

links = LinksField({
'self': 'get_api_url',
'html': 'get_absolute_html_url',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def url(self, project):

@pytest.fixture()
def institution(self):
return InstitutionFactory()
return InstitutionFactory(can_request_access=True)

@pytest.fixture()
def institution2(self):
Expand Down
41 changes: 41 additions & 0 deletions api_tests/users/views/test_user_message_institutional_access.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
InstitutionFactory
)
from website.mails import USER_MESSAGE_INSTITUTIONAL_ACCESS_REQUEST
from webtest import AppError


@pytest.mark.django_db
Expand All @@ -17,6 +18,10 @@ class TestUserMessageInstitutionalAccess:

@pytest.fixture()
def institution(self):
return InstitutionFactory(can_request_access=True)

@pytest.fixture()
def institution_without_access(self):
return InstitutionFactory()

@pytest.fixture()
Expand All @@ -33,16 +38,32 @@ def user_with_affiliation(self, institution):
user.add_or_update_affiliated_institution(institution)
return user

@pytest.fixture()
def user_with_affiliation_on_institution_without_access(self, institution_without_access):
user = AuthUserFactory()
user.add_or_update_affiliated_institution(institution_without_access)
return user

@pytest.fixture()
def institutional_admin(self, institution):
admin_user = AuthUserFactory()
institution.get_group('institutional_admins').user_set.add(admin_user)
return admin_user

@pytest.fixture()
def institutional_admin_on_institution_without_access(self, institution_without_access):
admin_user = AuthUserFactory()
institution_without_access.get_group('institutional_admins').user_set.add(admin_user)
return admin_user

@pytest.fixture()
def url_with_affiliation(self, user_with_affiliation):
return f'/{API_BASE}users/{user_with_affiliation._id}/messages/'

@pytest.fixture()
def url_with_affiliation_on_institution_without_access(self, user_with_affiliation_on_institution_without_access):
return f'/{API_BASE}users/{user_with_affiliation_on_institution_without_access._id}/messages/'

@pytest.fixture()
def url_without_affiliation(self, user):
return f'/{API_BASE}users/{user._id}/messages/'
Expand Down Expand Up @@ -89,6 +110,26 @@ def test_institutional_admin_can_create_message(self, mock_send_mail, app, insti
assert 'Requesting user access for collaboration' in mock_send_mail.call_args[1]['message_text']
assert user_message._id == data['id']

@mock.patch('osf.models.user_message.send_mail')
def test_institutional_admin_can_not_create_message(self, mock_send_mail, app, institutional_admin_on_institution_without_access,
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a test for the request node endpoint too.

institution_without_access, url_with_affiliation_on_institution_without_access,
payload):
"""
Ensure an institutional admin cannot create a `UserMessage` with a `message` and `institution` witch has 'can_request_access' as False
"""
mock_send_mail.return_value = mock.MagicMock()

# Use pytest.raises to explicitly expect the 403 error
with pytest.raises(AppError) as exc_info:
app.post_json_api(
url_with_affiliation_on_institution_without_access,
payload,
auth=institutional_admin_on_institution_without_access.auth
)

# Assert that the raised error contains the 403 Forbidden status
assert '403 Forbidden' in str(exc_info.value)

def test_unauthenticated_user_cannot_create_message(self, app, user, url_with_affiliation, payload):
"""
Ensure that unauthenticated users cannot create a `UserMessage`.
Expand Down
6 changes: 3 additions & 3 deletions framework/email/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
Category,
Attachment,
FileContent,
Email,
To,
Personalization,
Cc,
Expand Down Expand Up @@ -163,7 +162,8 @@ def _send_with_sendgrid(

client = client or SendGridAPIClient(settings.SENDGRID_API_KEY)
mail = Mail(
from_email=Email(from_addr),
from_email=from_addr,
to_emails=to_addr,
bodintsov marked this conversation as resolved.
Show resolved Hide resolved
html_content=message,
subject=subject,
)
Expand Down Expand Up @@ -191,7 +191,7 @@ def _send_with_sendgrid(
mail.add_personalization(personalization)

if categories:
mail.add_category([Category(x) for x in categories])
mail.category = [Category(x) for x in categories]
bodintsov marked this conversation as resolved.
Show resolved Hide resolved

if attachment_name and attachment_content:
attachment = Attachment(
Expand Down
18 changes: 18 additions & 0 deletions osf/migrations/0026_institution_can_request_access.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Generated by Django 4.2.15 on 2024-12-27 14:08
Copy link
Contributor

Choose a reason for hiding this comment

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

Bunch this in with the other migrations on this branch, just to be tidy. Delete both this and 0025 and re-run makemigrations and use that file.


from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('osf', '0025_noderequest_requested_permissions_and_more'),
]

operations = [
migrations.AddField(
model_name='institution',
name='can_request_access',
field=models.BooleanField(db_index=True, default=False),
),
]
1 change: 1 addition & 0 deletions osf/models/institution.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ class Institution(DirtyFieldsMixin, Loggable, ObjectIDMixin, BaseModel, Guardian
related_name='institutions'
)

can_request_access = models.BooleanField(default=False)
is_deleted = models.BooleanField(default=False, db_index=True)
deleted = NonNaiveDateTimeField(null=True, blank=True)
deactivated = NonNaiveDateTimeField(null=True, blank=True)
Expand Down
2 changes: 1 addition & 1 deletion osf/models/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,7 @@ def osf_groups(self):

def is_institutional_admin(self, institution):
group_name = institution.format_group('institutional_admins')
return self.groups.filter(name=group_name).exists()
return self.groups.filter(name=group_name).exists() and institution.can_request_access
Copy link
Contributor

Choose a reason for hiding this comment

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

It's difficult question where this AND check should be in the codebase. The institutional_admins permission may have other uses in the future that don't involve this feature, even though this is a quick fix and probably works for now you should move this check to the permissions classes that also check for this feature's request_type. UserMessagePermissions and InstitutionalAdminRequestTypePermission are better places to insert a conditional for this feature. is_institutional_admin can't "lie" about whether the user has that permission just because the feature is on/off.


def group_role(self, group):
"""
Expand Down
4 changes: 4 additions & 0 deletions osf_tests/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -257,10 +257,14 @@ class InstitutionFactory(DjangoModelFactory):
email_domains = FakeList('domain_name', n=1)
orcid_record_verified_source = ''
delegation_protocol = ''
can_request_access = False
Copy link
Contributor

Choose a reason for hiding this comment

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

I would default this to true, just because it's going to true going forward, but I'm not sure if it will break a bunch of tests consider using True here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would leave this as False, please. We don't want Product to have to turn this off for every institution to start with. We can always change it to True after launch once it goes fully into use.


class Meta:
model = models.Institution

@classmethod
def _create(cls, *args, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know what the point of doing this is, write a comment/docstring explaining or remove it.

return super()._create(*args, **kwargs)

class NodeLicenseRecordFactory(DjangoModelFactory):
year = factory.Faker('year')
Expand Down
Loading