Page MenuHomeSoftware Heritage
Differential D5335

tests: Rename keycloak_mock fixture to keycloak_oidc
ClosedPublic
Actions

Authored by anlambert on Mar 25 2021, 1:58 PM.

Details

Reviewers
ardumont
Group Reviewers
Reviewers
Commits
rDAUTHdb5388d38a6a: tests: Rename keycloak_mock fixture to keycloak_oidc
Summary

Let's start to apply recommended guidelines when writing tests.

See "Don't give mock/stubs/fakes special names" section.

Diff Detail

Event Timeline

anlambert created this revision.Mar 25 2021, 1:58 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptMar 25 2021, 1:58 PM
swh-public-ci added a comment.Mar 25 2021, 1:59 PM

Build is green

Patch application report for D5335 (id=19128)

Rebasing onto b1e0415698...

Current branch diff-target is up to date.
Changes applied before test
commit dc33050a245e1d0d78383752fb0827d352f21625
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Mar 25 13:52:00 2021 +0100

    tests: Rename keycloak_mock fixture to keycloak_oidc
    
    Let's start to apply recommended guidelines when writing tests.

See https://jenkins.softwareheritage.org/job/DAUTH/job/tests-on-diff/53/ for more details.

Harbormaster completed remote builds in B20213: Diff 19128.Mar 25 2021, 1:59 PM
anlambert requested review of this revision.Mar 25 2021, 1:59 PM
ardumont added a subscriber: ardumont.Mar 25 2021, 2:18 PM

ok

That will break deposit tests though (i gather you will deal with the webapp ones soon).

Can we keep aliases on old names to avoid the temporary breakage?

anlambert added a comment.Mar 25 2021, 2:19 PM
In D5335#135635, @ardumont wrote:

ok

That will break deposit tests though (i gather you will deal with the webapp ones soon).

Can we keep aliases on old names to avoid the temporary breakage?

Sure, will update the diff.

anlambert updated this revision to Diff 19131.Mar 25 2021, 2:24 PM

Ensure backaward compatibility with old function name

swh-public-ci added a comment.Mar 25 2021, 2:25 PM

Build is green

Patch application report for D5335 (id=19131)

Rebasing onto b1e0415698...

Current branch diff-target is up to date.
Changes applied before test
commit db5388d38a6a591d0d9d8bc35a440e13f62cac53
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Thu Mar 25 13:52:00 2021 +0100

    tests: Rename keycloak_mock fixture to keycloak_oidc
    
    Let's start to apply recommended guidelines when writing tests.

See https://jenkins.softwareheritage.org/job/DAUTH/job/tests-on-diff/54/ for more details.

Harbormaster completed remote builds in B20216: Diff 19131.Mar 25 2021, 2:25 PM
ardumont accepted this revision.Mar 25 2021, 2:27 PM

Thanks.

This revision is now accepted and ready to land.Mar 25 2021, 2:27 PM
Closed by commit rDAUTHdb5388d38a6a: tests: Rename keycloak_mock fixture to keycloak_oidc (authored by anlambert). · Explain WhyMar 25 2021, 5:11 PM
This revision was automatically updated to reflect the committed changes.
anlambert added a commit: rDAUTHdb5388d38a6a: tests: Rename keycloak_mock fixture to keycloak_oidc.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
auth/
6 lines95%
Loading...
tests/
4 lines100%
Loading...
django/
4 lines100%
Loading...
42 lines100%
Loading...

Diff 19128

View Options

swh/auth/pytest_plugin.py

  • This file was added.
# Copyright (C) 2020-2021 The Software Heritage developers
anlambertUnsubmitted
Not Done
Inline Actions

I think it will be better to create a swh.auth.django.models module and put that model in it.

In a similar manner, authentication backends should be put in a swh.auth.django.backends (in an other diff of course).

anlambert: I think it will be better to create a `swh.auth.django.models` module and put that model in it.
ardumontAuthorUnsubmitted
Done
Inline Actions

I think it will be better to create a swh.auth.django.models module and put that model in it.

ok

In a similar manner, authentication backends should be put in a swh.auth.django.backends (in an other diff of course).

Yes, although note i was not planning on adding the backends.

ardumont: > I think it will be better to create a swh.auth.django.models module and put that model in it.
# See the AUTHORS file at the top-level directory of this distribution
# License: GNU Affero General Public License version 3, or any later version
# See top-level LICENSE file for more information
from datetime import datetime
from typing import Optional, Set
from django.contrib.auth.models import User
class OIDCUser(User):
"""
Custom User proxy model for remote users storing OpenID Connect
related data: profile containing authentication tokens.
The model is also not saved to database as all users are already stored
in the Keycloak one.
"""
# OIDC subject identifier
sub: str = ""
# OIDC tokens and session related data, only relevant when a user
# authenticates from a web browser
access_token: Optional[str] = None
expires_at: Optional[datetime] = None
id_token: Optional[str] = None
refresh_token: Optional[str] = None
refresh_expires_at: Optional[datetime] = None
scope: Optional[str] = None
session_state: Optional[str] = None
# User permissions
permissions: Set[str]
class Meta:
# TODO: To redefine in subclass of this class
# Forced to empty otherwise, django complains about it
# "Model class swh.auth.django.OIDCUser doesn't declare an explicit app_label
# and isn't in an application in INSTALLED_APPS"
app_label = ""
proxy = True
def save(self, **kwargs):
"""
Override django.db.models.Model.save to avoid saving the remote
users to web application database.
"""
pass
def get_group_permissions(self, obj=None) -> Set[str]:
"""
Override django.contrib.auth.models.PermissionsMixin.get_group_permissions
to get permissions from OIDC
"""
return self.get_all_permissions(obj)
def get_all_permissions(self, obj=None) -> Set[str]:
"""
Override django.contrib.auth.models.PermissionsMixin.get_all_permissions
to get permissions from OIDC
"""
return self.permissions
def has_perm(self, perm, obj=None) -> bool:
"""
Override django.contrib.auth.models.PermissionsMixin.has_perm
to check permission from OIDC
"""
if self.is_active and self.is_superuser:
return True
return perm in self.permissions
def has_module_perms(self, app_label) -> bool:
"""
Override django.contrib.auth.models.PermissionsMixin.has_module_perms
to check permissions from OIDC.
"""
if self.is_active and self.is_superuser:
return True
return any(perm.startswith(app_label) for perm in self.permissions)
View Options

swh/auth/tests/conftest.py

  • This file was added.
# Copyright (C) 2020-2021 The Software Heritage developers
anlambertUnsubmitted
Not Done
Inline Actions

I think it will be better to create a swh.auth.django.models module and put that model in it.

In a similar manner, authentication backends should be put in a swh.auth.django.backends (in an other diff of course).

anlambert: I think it will be better to create a `swh.auth.django.models` module and put that model in it.
ardumontAuthorUnsubmitted
Done
Inline Actions

I think it will be better to create a swh.auth.django.models module and put that model in it.

ok

In a similar manner, authentication backends should be put in a swh.auth.django.backends (in an other diff of course).

Yes, although note i was not planning on adding the backends.

ardumont: > I think it will be better to create a swh.auth.django.models module and put that model in it.
# See the AUTHORS file at the top-level directory of this distribution
# License: GNU Affero General Public License version 3, or any later version
# See top-level LICENSE file for more information
from datetime import datetime
from typing import Optional, Set
from django.contrib.auth.models import User
class OIDCUser(User):
"""
Custom User proxy model for remote users storing OpenID Connect
related data: profile containing authentication tokens.
The model is also not saved to database as all users are already stored
in the Keycloak one.
"""
# OIDC subject identifier
sub: str = ""
# OIDC tokens and session related data, only relevant when a user
# authenticates from a web browser
access_token: Optional[str] = None
expires_at: Optional[datetime] = None
id_token: Optional[str] = None
refresh_token: Optional[str] = None
refresh_expires_at: Optional[datetime] = None
scope: Optional[str] = None
session_state: Optional[str] = None
# User permissions
permissions: Set[str]
class Meta:
# TODO: To redefine in subclass of this class
# Forced to empty otherwise, django complains about it
# "Model class swh.auth.django.OIDCUser doesn't declare an explicit app_label
# and isn't in an application in INSTALLED_APPS"
app_label = ""
proxy = True
def save(self, **kwargs):
"""
Override django.db.models.Model.save to avoid saving the remote
users to web application database.
"""
pass
def get_group_permissions(self, obj=None) -> Set[str]:
"""
Override django.contrib.auth.models.PermissionsMixin.get_group_permissions
to get permissions from OIDC
"""
return self.get_all_permissions(obj)
def get_all_permissions(self, obj=None) -> Set[str]:
"""
Override django.contrib.auth.models.PermissionsMixin.get_all_permissions
to get permissions from OIDC
"""
return self.permissions
def has_perm(self, perm, obj=None) -> bool:
"""
Override django.contrib.auth.models.PermissionsMixin.has_perm
to check permission from OIDC
"""
if self.is_active and self.is_superuser:
return True
return perm in self.permissions
def has_module_perms(self, app_label) -> bool:
"""
Override django.contrib.auth.models.PermissionsMixin.has_module_perms
to check permissions from OIDC.
"""
if self.is_active and self.is_superuser:
return True
return any(perm.startswith(app_label) for perm in self.permissions)
View Options

swh/auth/tests/django/test_utils.py

  • This file was added.
# Copyright (C) 2020-2021 The Software Heritage developers
anlambertUnsubmitted
Not Done
Inline Actions

I think it will be better to create a swh.auth.django.models module and put that model in it.

In a similar manner, authentication backends should be put in a swh.auth.django.backends (in an other diff of course).

anlambert: I think it will be better to create a `swh.auth.django.models` module and put that model in it.
ardumontAuthorUnsubmitted
Done
Inline Actions

I think it will be better to create a swh.auth.django.models module and put that model in it.

ok

In a similar manner, authentication backends should be put in a swh.auth.django.backends (in an other diff of course).

Yes, although note i was not planning on adding the backends.

ardumont: > I think it will be better to create a swh.auth.django.models module and put that model in it.
# See the AUTHORS file at the top-level directory of this distribution
# License: GNU Affero General Public License version 3, or any later version
# See top-level LICENSE file for more information
from datetime import datetime
from typing import Optional, Set
from django.contrib.auth.models import User
class OIDCUser(User):
"""
Custom User proxy model for remote users storing OpenID Connect
related data: profile containing authentication tokens.
The model is also not saved to database as all users are already stored
in the Keycloak one.
"""
# OIDC subject identifier
sub: str = ""
# OIDC tokens and session related data, only relevant when a user
# authenticates from a web browser
access_token: Optional[str] = None
expires_at: Optional[datetime] = None
id_token: Optional[str] = None
refresh_token: Optional[str] = None
refresh_expires_at: Optional[datetime] = None
scope: Optional[str] = None
session_state: Optional[str] = None
# User permissions
permissions: Set[str]
class Meta:
# TODO: To redefine in subclass of this class
# Forced to empty otherwise, django complains about it
# "Model class swh.auth.django.OIDCUser doesn't declare an explicit app_label
# and isn't in an application in INSTALLED_APPS"
app_label = ""
proxy = True
def save(self, **kwargs):
"""
Override django.db.models.Model.save to avoid saving the remote
users to web application database.
"""
pass
def get_group_permissions(self, obj=None) -> Set[str]:
"""
Override django.contrib.auth.models.PermissionsMixin.get_group_permissions
to get permissions from OIDC
"""
return self.get_all_permissions(obj)
def get_all_permissions(self, obj=None) -> Set[str]:
"""
Override django.contrib.auth.models.PermissionsMixin.get_all_permissions
to get permissions from OIDC
"""
return self.permissions
def has_perm(self, perm, obj=None) -> bool:
"""
Override django.contrib.auth.models.PermissionsMixin.has_perm
to check permission from OIDC
"""
if self.is_active and self.is_superuser:
return True
return perm in self.permissions
def has_module_perms(self, app_label) -> bool:
"""
Override django.contrib.auth.models.PermissionsMixin.has_module_perms
to check permissions from OIDC.
"""
if self.is_active and self.is_superuser:
return True
return any(perm.startswith(app_label) for perm in self.permissions)
View Options

swh/auth/tests/test_keycloak.py

  • This file was added.
# Copyright (C) 2020-2021 The Software Heritage developers
anlambertUnsubmitted
Not Done
Inline Actions

I think it will be better to create a swh.auth.django.models module and put that model in it.

In a similar manner, authentication backends should be put in a swh.auth.django.backends (in an other diff of course).

anlambert: I think it will be better to create a `swh.auth.django.models` module and put that model in it.
ardumontAuthorUnsubmitted
Done
Inline Actions

I think it will be better to create a swh.auth.django.models module and put that model in it.

ok

In a similar manner, authentication backends should be put in a swh.auth.django.backends (in an other diff of course).

Yes, although note i was not planning on adding the backends.

ardumont: > I think it will be better to create a swh.auth.django.models module and put that model in it.
# See the AUTHORS file at the top-level directory of this distribution
# License: GNU Affero General Public License version 3, or any later version
# See top-level LICENSE file for more information
from datetime import datetime
from typing import Optional, Set
from django.contrib.auth.models import User
class OIDCUser(User):
"""
Custom User proxy model for remote users storing OpenID Connect
related data: profile containing authentication tokens.
The model is also not saved to database as all users are already stored
in the Keycloak one.
"""
# OIDC subject identifier
sub: str = ""
# OIDC tokens and session related data, only relevant when a user
# authenticates from a web browser
access_token: Optional[str] = None
expires_at: Optional[datetime] = None
id_token: Optional[str] = None
refresh_token: Optional[str] = None
refresh_expires_at: Optional[datetime] = None
scope: Optional[str] = None
session_state: Optional[str] = None
# User permissions
permissions: Set[str]
class Meta:
# TODO: To redefine in subclass of this class
# Forced to empty otherwise, django complains about it
# "Model class swh.auth.django.OIDCUser doesn't declare an explicit app_label
# and isn't in an application in INSTALLED_APPS"
app_label = ""
proxy = True
def save(self, **kwargs):
"""
Override django.db.models.Model.save to avoid saving the remote
users to web application database.
"""
pass
def get_group_permissions(self, obj=None) -> Set[str]:
"""
Override django.contrib.auth.models.PermissionsMixin.get_group_permissions
to get permissions from OIDC
"""
return self.get_all_permissions(obj)
def get_all_permissions(self, obj=None) -> Set[str]:
"""
Override django.contrib.auth.models.PermissionsMixin.get_all_permissions
to get permissions from OIDC
"""
return self.permissions
def has_perm(self, perm, obj=None) -> bool:
"""
Override django.contrib.auth.models.PermissionsMixin.has_perm
to check permission from OIDC
"""
if self.is_active and self.is_superuser:
return True
return perm in self.permissions
def has_module_perms(self, app_label) -> bool:
"""
Override django.contrib.auth.models.PermissionsMixin.has_module_perms
to check permissions from OIDC.
"""
if self.is_active and self.is_superuser:
return True
return any(perm.startswith(app_label) for perm in self.permissions)
About · Developer information · Legal