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

(PC-32153)[API] chore: remove feature flag WIP_ENABLE_NEW_HASHING_ALGORITHM #16293

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion api/.env.development
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ TYPEFORM_IMPORT_CHUNK_SIZE=100
TZ=UTC
UBBLE_MOCK_API_URL=https://mock-ubble.ehp.passculture.team/api
UBBLE_MOCK_CONFIG_URL=https://mock-ubble.ehp.passculture.team
USE_FAST_AND_INSECURE_PASSWORD_HASHING_ALGORITHM=1
USE_FAST_AND_INSECURE_PASSWORD_HASHING_ALGORITHM=0
VIRUSTOTAL_BACKEND=pcapi.connectors.virustotal.LoggerBackend
WEBAPP_V2_REDIRECT_URL=http://localhost:5173
WEBAPP_V2_URL=http://localhost:5173
Expand Down
16 changes: 4 additions & 12 deletions api/src/pcapi/core/offerers/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -814,10 +814,7 @@ def generate_and_save_api_key(offerer_id: int) -> str:
def generate_offerer_api_key(offerer_id: int) -> tuple[models.ApiKey, str]:
clear_secret = secrets.token_hex(32)
prefix = _generate_api_key_prefix()
if feature.FeatureToggle.WIP_ENABLE_NEW_HASHING_ALGORITHM.is_active():
key = models.ApiKey(offererId=offerer_id, prefix=prefix, secret=crypto.hash_public_api_key(clear_secret))
else:
key = models.ApiKey(offererId=offerer_id, prefix=prefix, secret=crypto.hash_password(clear_secret))
key = models.ApiKey(offererId=offerer_id, prefix=prefix, secret=crypto.hash_public_api_key(clear_secret))

return key, f"{prefix}{API_KEY_SEPARATOR}{clear_secret}"

Expand All @@ -829,14 +826,9 @@ def generate_provider_api_key(provider: providers_models.Provider) -> tuple[mode

clear_secret = secrets.token_hex(32)
prefix = _generate_api_key_prefix()
if feature.FeatureToggle.WIP_ENABLE_NEW_HASHING_ALGORITHM.is_active():
key = models.ApiKey(
offerer=offerer, provider=provider, prefix=prefix, secret=crypto.hash_public_api_key(clear_secret)
)
else:
key = models.ApiKey(
offerer=offerer, provider=provider, prefix=prefix, secret=crypto.hash_password(clear_secret)
)
key = models.ApiKey(
offerer=offerer, provider=provider, prefix=prefix, secret=crypto.hash_public_api_key(clear_secret)
)

return key, f"{prefix}{API_KEY_SEPARATOR}{clear_secret}"

Expand Down
7 changes: 1 addition & 6 deletions api/src/pcapi/core/offerers/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
from pcapi.core.factories import BaseFactory
import pcapi.core.geography.factories as geography_factories
import pcapi.core.users.factories as users_factories
from pcapi.models import feature
from pcapi.models.validation_status_mixin import ValidationStatus
from pcapi.utils import crypto
from pcapi.utils import siren as siren_utils
Expand Down Expand Up @@ -370,11 +369,7 @@ def _create(
*args: typing.Any,
**kwargs: typing.Any,
) -> models.ApiKey:
if feature.FeatureToggle.WIP_ENABLE_NEW_HASHING_ALGORITHM.is_active():
kwargs["secret"] = crypto.hash_public_api_key(kwargs.get("secret", DEFAULT_SECRET))
else:
kwargs["secret"] = crypto.hash_password(kwargs.get("secret", DEFAULT_SECRET))

kwargs["secret"] = crypto.hash_public_api_key(kwargs.get("secret", DEFAULT_SECRET))
return super()._create(model_class, *args, **kwargs)


Expand Down
7 changes: 3 additions & 4 deletions api/src/pcapi/core/offerers/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -1084,10 +1084,9 @@ def check_secret(self, clear_text: str) -> bool:
if self.secret.decode("utf-8").startswith("$sha3_512$"):
return crypto.check_public_api_key(clear_text, self.secret)
if crypto.check_password(clear_text, self.secret):
if FeatureToggle.WIP_ENABLE_NEW_HASHING_ALGORITHM.is_active():
self.secret = crypto.hash_public_api_key(clear_text)
db.session.commit()
logger.info("Switched hash of API key from bcrypt to SHA3-512", extra={"key_id": self.id})
self.secret = crypto.hash_public_api_key(clear_text)
db.session.commit()
logger.info("Switched hash of API key from bcrypt to SHA3-512", extra={"key_id": self.id})
return True
return False

Expand Down
2 changes: 0 additions & 2 deletions api/src/pcapi/models/feature.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ class FeatureToggle(enum.Enum):
WIP_ENABLE_NATIONAL_PROGRAM_NEW_RULES_PUBLIC_API = (
"Activer les nouvelles règles de création et d'édition d'offres collecrives pour l'API publique (collective)"
)
WIP_ENABLE_NEW_HASHING_ALGORITHM = "Activer le nouveau système de hachage des clés publiques d'API"
WIP_ENABLE_OFFER_ADDRESS = "Activer l'association des offres à des adresses."
WIP_ENABLE_OFFER_ADDRESS_COLLECTIVE = "Activer l'association des offres collectives à des adresses."
WIP_SUGGESTED_SUBCATEGORIES = "Activer les sous-catégories suggérées par IA lors de la création d'offre"
Expand Down Expand Up @@ -210,7 +209,6 @@ def nameKey(self) -> str:
FeatureToggle.WIP_ENABLE_OFFER_ADDRESS_COLLECTIVE,
FeatureToggle.WIP_ENABLE_NEW_COLLECTIVE_OFFERS_AND_BOOKINGS_STRUCTURE,
FeatureToggle.WIP_ENABLE_NEW_FINANCE_WORKFLOW,
FeatureToggle.WIP_ENABLE_NEW_HASHING_ALGORITHM,
FeatureToggle.WIP_ENABLE_PRO_DIDACTIC_ONBOARDING,
FeatureToggle.WIP_ENABLE_PRO_DIDACTIC_ONBOARDING_AB_TEST,
FeatureToggle.WIP_ENABLE_REMINDER_MARKETING_MAIL_METADATA_DISPLAY,
Expand Down
8 changes: 5 additions & 3 deletions api/tests/core/offerers/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -529,16 +529,18 @@ def test_generate_and_save_api_key(self):
assert found_api_key.offerer == offerer

def test_get_provider_from_api_key(self):
value = "a very secret legacy key"
value = "development_prefix_a very secret key"
offerer = offerers_factories.OffererFactory()
provider = providers_factories.ProviderFactory(localClass=None, name="RiotRecords")
providers_factories.OffererProviderFactory(offerer=offerer, provider=provider)
offerers_factories.ApiKeyFactory(
offerer=offerer, provider=provider, prefix="development_a very s", secret="ecret legacy key"
offerer=offerer, provider=provider, prefix="development_prefix", secret="a very secret key"
)

with assert_num_queries(1):
found_api_key = offerers_api.find_api_key(value)
assert found_api_key.provider == provider

assert found_api_key.provider == provider

def test_legacy_api_key(self):
value = "a very secret legacy key"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ class GetEventsTest(PublicAPIVenueEndpointHelper):
endpoint_method = "get"

num_queries_with_error = 1 # select api_key, offerer and provider
num_queries_with_error += 1 # select features
num_queries_with_error += 1 # check provider EXISTS
num_queries = num_queries_with_error + 1 # select offers

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ class GetProductsTest(PublicAPIVenueEndpointHelper):
endpoint_method = "get"

num_queries_400 = 1 # select api_key, offerer and provider
num_queries_400 += 1 # select features
num_queries_404 = num_queries_400 + 1 # check venue_provider exists
num_queries_success = num_queries_404 + 1 # select offer

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -385,15 +385,14 @@ def test_update_name_and_description(self, client):
new_name = product_offer.name + " updated"
new_desc = product_offer.description + " updated"

# 1. get api key
# 2. check FF
# 3. get offer and related data
# 4. select oa
# 5. update offer
# 6. reload provider
# 7. reload offer and related data (before serialization)
# 8. check venue offerer address
with assert_num_queries(8):
expected_num_queries = 1 # get api key
expected_num_queries += 1 # get offer and related data
expected_num_queries += 1 # select oa
expected_num_queries += 1 # update offer
expected_num_queries += 1 # reload provider
expected_num_queries += 1 # reload offer and related data (before serialization)
expected_num_queries += 1 # check venue offerer address
with assert_num_queries(expected_num_queries):
response = client.with_explicit_token(offerers_factories.DEFAULT_CLEAR_API_KEY).patch(
"/public/offers/v1/products",
json={"offerId": offer_id, "name": new_name, "description": new_desc},
Expand Down
1 change: 0 additions & 1 deletion api/tests/utils/crypto_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ def test_no_password_leak(self):
assert "user@AZERTY123" not in str(exception), str(exception)


@pytest.mark.settings(USE_FAST_AND_INSECURE_PASSWORD_HASHING_ALGORITHM=False)
class ProdEnvironmentPasswordHasherTest:
def test_hash_password_uses_bcrypt(self):
hashed = crypto.hash_password("secret")
Expand Down
57 changes: 0 additions & 57 deletions api/tests/validation/routes/users_authentifications_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -162,40 +162,9 @@ def test_require_active_provider(self, provider_is_active, expected_response_sta

assert response.status_code == expected_response_status

@pytest.mark.parametrize("use_fast_and_insecure_password_hashing_algorithm", [True, False])
@pytest.mark.features(WIP_ENABLE_NEW_HASHING_ALGORITHM=False)
@patch("pcapi.utils.crypto.check_public_api_key")
@patch("pcapi.utils.crypto.hash_public_api_key")
def test_ff_for_prod_and_testing_env(
self,
hash_public_api_key_function,
check_public_api_key_function,
use_fast_and_insecure_password_hashing_algorithm,
settings,
):
settings.USE_FAST_AND_INSECURE_PASSWORD_HASHING_ALGORITHM = use_fast_and_insecure_password_hashing_algorithm

api_key = offerers_factories.ApiKeyFactory()
old_secret = api_key.secret
if use_fast_and_insecure_password_hashing_algorithm:
# md5 hashing
assert not old_secret.decode("utf-8").startswith("$2b$")
assert not old_secret.decode("utf-8").startswith("$sha3_512$")
else:
assert old_secret.decode("utf-8").startswith("$2b$") # testing that the secret is hashed with bcrypt
app = self._make_app()
client = TestClient(app.test_client())
headers = {"Authorization": "Bearer development_prefix_clearSecret"}
response = client.get("/test", headers=headers)
assert response.status_code == 200
assert api_key.secret.decode("utf-8") == old_secret.decode("utf-8")
check_public_api_key_function.assert_not_called()
hash_public_api_key_function.assert_not_called()

@pytest.mark.settings(
USE_FAST_AND_INSECURE_PASSWORD_HASHING_ALGORITHM=False,
)
@pytest.mark.features(WIP_ENABLE_NEW_HASHING_ALGORITHM=True)
def test_old_api_key_update(self, db_session):
"""Test that old api key is updated to the new hashing algorithm"""
api_key = offerers_factories.ApiKeyFactory()
Expand All @@ -215,7 +184,6 @@ def test_old_api_key_update(self, db_session):
@pytest.mark.settings(
USE_FAST_AND_INSECURE_PASSWORD_HASHING_ALGORITHM=False,
)
@pytest.mark.features(WIP_ENABLE_NEW_HASHING_ALGORITHM=True)
@pytest.mark.parametrize("hashcode", ["$2a$12$hashed_password", "$2x$12$hashed_password", "$2y$12$hashed_password"])
@patch("pcapi.utils.crypto.check_password")
@patch("pcapi.utils.crypto.check_public_api_key")
Expand All @@ -234,7 +202,6 @@ def test_deprecated_hash(self, mock_check_public_api_key, mock_check_password, h
@pytest.mark.settings(
USE_FAST_AND_INSECURE_PASSWORD_HASHING_ALGORITHM=True,
)
@pytest.mark.features(WIP_ENABLE_NEW_HASHING_ALGORITHM=True)
def test_unsecure_old_api_key_update(self, db_session):
"""Test that old md5 api key isn't updated to the new hashing algorithm"""
api_key = offerers_factories.ApiKeyFactory()
Expand All @@ -248,27 +215,3 @@ def test_unsecure_old_api_key_update(self, db_session):
assert not api_key.secret.decode("utf-8").startswith("$sha3_512$")
assert check_public_api_key("clearSecret", api_key.secret)
db_session.flush()

@pytest.mark.settings(
USE_FAST_AND_INSECURE_PASSWORD_HASHING_ALGORITHM=False,
)
def test_switching_ff(self, features, db_session):
"""Test that the hashing algorithm is updated when the feature flag is switched"""
app = self._make_app()
client = TestClient(app.test_client())
features.WIP_ENABLE_NEW_HASHING_ALGORITHM = False
api_key = offerers_factories.ApiKeyFactory()
assert api_key.secret.decode("utf-8").startswith("$2b$")

features.WIP_ENABLE_NEW_HASHING_ALGORITHM = True
headers = {"Authorization": "Bearer development_prefix_clearSecret"}
response = client.get("/test", headers=headers)
assert response.status_code == 200
db_session.refresh(api_key)
assert api_key.secret.decode("utf-8").startswith("$sha3_512$")
assert check_public_api_key("clearSecret", api_key.secret)

features.WIP_ENABLE_NEW_HASHING_ALGORITHM = False
response = client.get("/test", headers=headers)
assert response.status_code == 200
assert check_public_api_key("clearSecret", api_key.secret)
Loading