Page MenuHomeSoftware Heritage
Differential D6748

debian: Fix confusion between the two versions
ClosedPublic
Actions

Authored by vlorentz on Dec 6 2021, 1:48 PM.

Details

Reviewers
olasd
anlambert
Group Reviewers
Reviewers
Commits
rDLDBASE26996ef3eef4: debian: Fix confusion between the two versions
Summary

'version' was documented as the intrinsic version (eg. '0.7.2-3') and
'full_version' as the one containing the suite name (eg. 'stretch/contrib/0.7.2-3').

In practice, it was the opposite, except in a few incorrect test.

This commit fixes said tests, and renamed 'full_version' to
'intrinsic_version'.

This is only a refactoring, the behavior is unchanged for now;
but a future commit will remove the 'version' (which is extrinsic) from
the release name (which should contain only data intrinsic to the DSC).

Diff Detail

Event Timeline

vlorentz created this revision.Dec 6 2021, 1:48 PM
vlorentz added 1 blocking reviewer(s): olasd.Dec 6 2021, 1:49 PM
swh-public-ci added a comment.Dec 6 2021, 1:50 PM

Build is green

Patch application report for D6748 (id=24507)

Rebasing onto 5d22455c94...

Current branch diff-target is up to date.
Changes applied before test
commit 379ff90b7703f7c8e8dc2faa97713641d7e21757
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Mon Dec 6 13:48:04 2021 +0100

    debian: Fix confusion between the two versions
    
    'version' was documented as the intrinsic version (eg. '0.7.2-3') and
    'full_version' as the one containing the suite name (eg. 'stretch/contrib/0.7.2-3').
    
    In practice, it was the opposite, except in a few incorrect test.
    
    This commit fixes said tests, and renamed 'full_version' to
    'intrinsic_version'.
    
    This is only a refactoring, the behavior is unchanged for now;
    but a future commit will remove the 'version' (which is extrinsic) from
    the release name (which should contain only data intrinsic to the DSC).

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

Harbormaster completed remote builds in B25381: Diff 24507.Dec 6 2021, 1:50 PM
vlorentz requested review of this revision.Dec 6 2021, 1:50 PM
vlorentz updated this revision to Diff 24508.Dec 6 2021, 1:54 PM

update the doc accordingly

vlorentz updated this revision to Diff 24509.Dec 6 2021, 1:55 PM

oops, I confused the names again in the doc

swh-public-ci added a comment.Dec 6 2021, 1:56 PM

Build is green

Patch application report for D6748 (id=24508)

Rebasing onto 5d22455c94...

Current branch diff-target is up to date.
Changes applied before test
commit c469149b77c57c8df75bf1e51a6919cd3294747e
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Mon Dec 6 13:48:04 2021 +0100

    debian: Fix confusion between the two versions
    
    'version' was documented as the intrinsic version (eg. '0.7.2-3') and
    'full_version' as the one containing the suite name (eg. 'stretch/contrib/0.7.2-3').
    
    In practice, it was the opposite, except in a few incorrect test.
    
    This commit fixes said tests, and renamed 'full_version' to
    'intrinsic_version'.
    
    This is only a refactoring, the behavior is unchanged for now;
    but a future commit will remove the 'version' (which is extrinsic) from
    the release name (which should contain only data intrinsic to the DSC).

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

Harbormaster completed remote builds in B25382: Diff 24508.Dec 6 2021, 1:56 PM
swh-public-ci added a comment.Dec 6 2021, 1:58 PM

Build is green

Patch application report for D6748 (id=24509)

Rebasing onto 5d22455c94...

Current branch diff-target is up to date.
Changes applied before test
commit 958674dc24171ced169397054ffb1502b60f7f43
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Mon Dec 6 13:48:04 2021 +0100

    debian: Fix confusion between the two versions
    
    'version' was documented as the intrinsic version (eg. '0.7.2-3') and
    'full_version' as the one containing the suite name (eg. 'stretch/contrib/0.7.2-3').
    
    In practice, it was the opposite, except in a few incorrect test.
    
    This commit fixes said tests, and renamed 'full_version' to
    'intrinsic_version'.
    
    This is only a refactoring, the behavior is unchanged for now;
    but a future commit will remove the 'version' (which is extrinsic) from
    the release name (which should contain only data intrinsic to the DSC).

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

Harbormaster completed remote builds in B25383: Diff 24509.Dec 6 2021, 1:58 PM
vlorentz added a child revision: D6749: debian: Remove the extrinsic version from release names.Dec 6 2021, 2:06 PM
anlambert accepted this revision.Dec 6 2021, 2:41 PM
anlambert added a subscriber: anlambert.

Looks good to me.

olasd accepted this revision.Dec 6 2021, 2:44 PM
This revision is now accepted and ready to land.Dec 6 2021, 2:44 PM
olasd added a comment.Dec 6 2021, 2:44 PM

Thank you!

ardumont mentioned this in T3774: Deploy swh.loader.core v2.0.0.Dec 7 2021, 9:29 AM
vlorentz updated this revision to Diff 24537.Dec 7 2021, 11:06 AM

rebase

swh-public-ci added a comment.Dec 7 2021, 11:08 AM

Build is green

Patch application report for D6748 (id=24537)

Rebasing onto 2d9e93a2f2...

Current branch diff-target is up to date.
Changes applied before test
commit 26996ef3eef494eb187e01b999255cfa101ad758
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Mon Dec 6 13:48:04 2021 +0100

    debian: Fix confusion between the two versions
    
    'version' was documented as the intrinsic version (eg. '0.7.2-3') and
    'full_version' as the one containing the suite name (eg. 'stretch/contrib/0.7.2-3').
    
    In practice, it was the opposite, except in a few incorrect test.
    
    This commit fixes said tests, and renamed 'full_version' to
    'intrinsic_version'.
    
    This is only a refactoring, the behavior is unchanged for now;
    but a future commit will remove the 'version' (which is extrinsic) from
    the release name (which should contain only data intrinsic to the DSC).

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

Harbormaster completed remote builds in B25411: Diff 24537.Dec 7 2021, 11:08 AM
Closed by commit rDLDBASE26996ef3eef4: debian: Fix confusion between the two versions (authored by vlorentz). · Explain WhyDec 7 2021, 1:26 PM
This revision was automatically updated to reflect the committed changes.
vlorentz added a commit: rDLDBASE26996ef3eef4: debian: Fix confusion between the two versions.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
swh/
loader/
package/
debian/
17 lines93%
Loading...
tests/
28 lines100%
Loading...

Diff 24507

View Options

swh/loader/package/debian/loader.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/loader/package/debian/tests/test_debian.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