Skip to content

Commit b1ebaad

Browse files
rucoderrucoder
committed
fix: seed OAuth IS_*_ENABLED flags individually instead of all-or-nothing
The configure_instance management command checked whether *any* of the four IS_*_ENABLED keys (Google, GitHub, GitLab, Gitea) existed before deciding whether to create them. Because IS_GITEA_ENABLED was already seeded by instance_config_variables in the first loop, the exists() check always returned True and the remaining three keys were silently skipped. This meant IS_GITHUB_ENABLED, IS_GOOGLE_ENABLED, and IS_GITLAB_ENABLED were never created in the database. Toggling these providers on in God Mode sent a PATCH to update a non-existent row, which returned 200 OK but did nothing — the toggle appeared to succeed but reverted on page reload. Replace the all-or-nothing check with per-key get_or_create so each flag is seeded independently of the others. Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
1 parent 6627282 commit b1ebaad

File tree

3 files changed

+153
-109
lines changed

3 files changed

+153
-109
lines changed

‎apps/api/plane/license/management/commands/configure_instance.py‎

Lines changed: 53 additions & 109 deletions
Original file line numberDiff line numberDiff line change
@@ -40,113 +40,57 @@ def handle(self, *args, **options):
4040
else:
4141
self.stdout.write(self.style.WARNING(f"{obj.key} configuration already exists"))
4242

43-
keys = ["IS_GOOGLE_ENABLED", "IS_GITHUB_ENABLED", "IS_GITLAB_ENABLED", "IS_GITEA_ENABLED"]
44-
if not InstanceConfiguration.objects.filter(key__in=keys).exists():
45-
for key in keys:
46-
if key == "IS_GOOGLE_ENABLED":
47-
GOOGLE_CLIENT_ID, GOOGLE_CLIENT_SECRET = get_configuration_value(
48-
[
49-
{
50-
"key": "GOOGLE_CLIENT_ID",
51-
"default": os.environ.get("GOOGLE_CLIENT_ID", ""),
52-
},
53-
{
54-
"key": "GOOGLE_CLIENT_SECRET",
55-
"default": os.environ.get("GOOGLE_CLIENT_SECRET", "0"),
56-
},
57-
]
58-
)
59-
if bool(GOOGLE_CLIENT_ID) and bool(GOOGLE_CLIENT_SECRET):
60-
value = "1"
61-
else:
62-
value = "0"
63-
InstanceConfiguration.objects.create(
64-
key=key,
65-
value=value,
66-
category="AUTHENTICATION",
67-
is_encrypted=False,
68-
)
69-
self.stdout.write(self.style.SUCCESS(f"{key} loaded with value from environment variable."))
70-
if key == "IS_GITHUB_ENABLED":
71-
GITHUB_CLIENT_ID, GITHUB_CLIENT_SECRET = get_configuration_value(
72-
[
73-
{
74-
"key": "GITHUB_CLIENT_ID",
75-
"default": os.environ.get("GITHUB_CLIENT_ID", ""),
76-
},
77-
{
78-
"key": "GITHUB_CLIENT_SECRET",
79-
"default": os.environ.get("GITHUB_CLIENT_SECRET", "0"),
80-
},
81-
]
82-
)
83-
if bool(GITHUB_CLIENT_ID) and bool(GITHUB_CLIENT_SECRET):
84-
value = "1"
85-
else:
86-
value = "0"
87-
InstanceConfiguration.objects.create(
88-
key="IS_GITHUB_ENABLED",
89-
value=value,
90-
category="AUTHENTICATION",
91-
is_encrypted=False,
92-
)
93-
self.stdout.write(self.style.SUCCESS(f"{key} loaded with value from environment variable."))
94-
if key == "IS_GITLAB_ENABLED":
95-
GITLAB_HOST, GITLAB_CLIENT_ID, GITLAB_CLIENT_SECRET = get_configuration_value(
96-
[
97-
{
98-
"key": "GITLAB_HOST",
99-
"default": os.environ.get("GITLAB_HOST", "https://gitlab.com"),
100-
},
101-
{
102-
"key": "GITLAB_CLIENT_ID",
103-
"default": os.environ.get("GITLAB_CLIENT_ID", ""),
104-
},
105-
{
106-
"key": "GITLAB_CLIENT_SECRET",
107-
"default": os.environ.get("GITLAB_CLIENT_SECRET", ""),
108-
},
109-
]
110-
)
111-
if bool(GITLAB_HOST) and bool(GITLAB_CLIENT_ID) and bool(GITLAB_CLIENT_SECRET):
112-
value = "1"
113-
else:
114-
value = "0"
115-
InstanceConfiguration.objects.create(
116-
key="IS_GITLAB_ENABLED",
117-
value=value,
118-
category="AUTHENTICATION",
119-
is_encrypted=False,
120-
)
121-
self.stdout.write(self.style.SUCCESS(f"{key} loaded with value from environment variable."))
122-
if key == "IS_GITEA_ENABLED":
123-
GITEA_HOST, GITEA_CLIENT_ID, GITEA_CLIENT_SECRET = get_configuration_value(
124-
[
125-
{
126-
"key": "GITEA_HOST",
127-
"default": os.environ.get("GITEA_HOST", ""),
128-
},
129-
{
130-
"key": "GITEA_CLIENT_ID",
131-
"default": os.environ.get("GITEA_CLIENT_ID", ""),
132-
},
133-
{
134-
"key": "GITEA_CLIENT_SECRET",
135-
"default": os.environ.get("GITEA_CLIENT_SECRET", ""),
136-
},
137-
]
138-
)
139-
if bool(GITEA_HOST) and bool(GITEA_CLIENT_ID) and bool(GITEA_CLIENT_SECRET):
140-
value = "1"
141-
else:
142-
value = "0"
143-
InstanceConfiguration.objects.create(
144-
key="IS_GITEA_ENABLED",
145-
value=value,
146-
category="AUTHENTICATION",
147-
is_encrypted=False,
148-
)
149-
self.stdout.write(self.style.SUCCESS(f"{key} loaded with value from environment variable."))
150-
else:
151-
for key in keys:
43+
# Seed IS_*_ENABLED flags individually so that each missing key
44+
# is created independently. The previous all-or-nothing check
45+
# skipped every key when even one already existed (e.g.
46+
# IS_GITEA_ENABLED seeded via instance_config_variables).
47+
oauth_enabled_keys = {
48+
"IS_GOOGLE_ENABLED": lambda: all(
49+
get_configuration_value(
50+
[
51+
{"key": "GOOGLE_CLIENT_ID", "default": os.environ.get("GOOGLE_CLIENT_ID", "")},
52+
{"key": "GOOGLE_CLIENT_SECRET", "default": os.environ.get("GOOGLE_CLIENT_SECRET", "")},
53+
]
54+
)
55+
),
56+
"IS_GITHUB_ENABLED": lambda: all(
57+
get_configuration_value(
58+
[
59+
{"key": "GITHUB_CLIENT_ID", "default": os.environ.get("GITHUB_CLIENT_ID", "")},
60+
{"key": "GITHUB_CLIENT_SECRET", "default": os.environ.get("GITHUB_CLIENT_SECRET", "")},
61+
]
62+
)
63+
),
64+
"IS_GITLAB_ENABLED": lambda: all(
65+
get_configuration_value(
66+
[
67+
{"key": "GITLAB_HOST", "default": os.environ.get("GITLAB_HOST", "")},
68+
{"key": "GITLAB_CLIENT_ID", "default": os.environ.get("GITLAB_CLIENT_ID", "")},
69+
{"key": "GITLAB_CLIENT_SECRET", "default": os.environ.get("GITLAB_CLIENT_SECRET", "")},
70+
]
71+
)
72+
),
73+
"IS_GITEA_ENABLED": lambda: all(
74+
get_configuration_value(
75+
[
76+
{"key": "GITEA_HOST", "default": os.environ.get("GITEA_HOST", "")},
77+
{"key": "GITEA_CLIENT_ID", "default": os.environ.get("GITEA_CLIENT_ID", "")},
78+
{"key": "GITEA_CLIENT_SECRET", "default": os.environ.get("GITEA_CLIENT_SECRET", "")},
79+
]
80+
)
81+
),
82+
}
83+
84+
for key, check_configured in oauth_enabled_keys.items():
85+
obj, created = InstanceConfiguration.objects.get_or_create(
86+
key=key,
87+
defaults={
88+
"value": "1" if check_configured() else "0",
89+
"category": "AUTHENTICATION",
90+
"is_encrypted": False,
91+
},
92+
)
93+
if created:
94+
self.stdout.write(self.style.SUCCESS(f"{key} loaded with value from environment variable."))
95+
else:
15296
self.stdout.write(self.style.WARNING(f"{key} configuration already exists"))

‎apps/api/plane/tests/unit/management/__init__.py‎

Whitespace-only changes.

‎apps/api/plane/tests/unit/management/test_configure_instance.py‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
# Copyright (c) 2023-present Plane Software, Inc. and contributors
2+
# SPDX-License-Identifier: AGPL-3.0-only
3+
# See the LICENSE file for details.
4+
5+
import pytest
6+
from io import StringIO
7+
from unittest.mock import patch
8+
9+
from django.core.management import call_command
10+
11+
from plane.license.models import InstanceConfiguration
12+
13+
14+
@pytest.mark.unit
15+
class TestConfigureInstanceOAuthSeeding:
16+
"""Test that configure_instance seeds IS_*_ENABLED flags individually."""
17+
18+
@pytest.mark.django_db
19+
def test_creates_all_enabled_flags_when_none_exist(self):
20+
"""All four IS_*_ENABLED flags should be created on a fresh database."""
21+
out = StringIO()
22+
with patch.dict("os.environ", {"SECRET_KEY": "test-secret"}):
23+
call_command("configure_instance", stdout=out)
24+
25+
keys = [
26+
"IS_GOOGLE_ENABLED",
27+
"IS_GITHUB_ENABLED",
28+
"IS_GITLAB_ENABLED",
29+
"IS_GITEA_ENABLED",
30+
]
31+
for key in keys:
32+
assert InstanceConfiguration.objects.filter(key=key).exists(), (
33+
f"{key} should have been created"
34+
)
35+
36+
@pytest.mark.django_db
37+
def test_creates_missing_flags_when_gitea_already_exists(self):
38+
"""Missing IS_*_ENABLED flags should be created even if IS_GITEA_ENABLED already exists.
39+
40+
This is the regression scenario: IS_GITEA_ENABLED is seeded by
41+
instance_config_variables in the first loop, which previously
42+
caused the all-or-nothing exists() check to skip the other three.
43+
"""
44+
# Simulate the state after first loop seeds IS_GITEA_ENABLED
45+
InstanceConfiguration.objects.create(
46+
key="IS_GITEA_ENABLED",
47+
value="0",
48+
category="GITEA",
49+
is_encrypted=False,
50+
)
51+
52+
out = StringIO()
53+
with patch.dict("os.environ", {"SECRET_KEY": "test-secret"}):
54+
call_command("configure_instance", stdout=out)
55+
56+
for key in ["IS_GOOGLE_ENABLED", "IS_GITHUB_ENABLED", "IS_GITLAB_ENABLED"]:
57+
assert InstanceConfiguration.objects.filter(key=key).exists(), (
58+
f"{key} should have been created even though IS_GITEA_ENABLED already existed"
59+
)
60+
61+
@pytest.mark.django_db
62+
def test_does_not_overwrite_existing_enabled_flags(self):
63+
"""Existing IS_*_ENABLED values should not be overwritten on re-run."""
64+
InstanceConfiguration.objects.create(
65+
key="IS_GITHUB_ENABLED",
66+
value="1",
67+
category="AUTHENTICATION",
68+
is_encrypted=False,
69+
)
70+
71+
out = StringIO()
72+
with patch.dict("os.environ", {"SECRET_KEY": "test-secret"}):
73+
call_command("configure_instance", stdout=out)
74+
75+
config = InstanceConfiguration.objects.get(key="IS_GITHUB_ENABLED")
76+
assert config.value == "1", "Existing IS_GITHUB_ENABLED=1 should not be overwritten"
77+
78+
@pytest.mark.django_db
79+
def test_enabled_flags_default_to_zero_without_credentials(self):
80+
"""Without OAuth env vars, IS_*_ENABLED flags should default to '0'."""
81+
out = StringIO()
82+
env = {
83+
"SECRET_KEY": "test-secret",
84+
"GITHUB_CLIENT_ID": "",
85+
"GITHUB_CLIENT_SECRET": "",
86+
"GOOGLE_CLIENT_ID": "",
87+
"GOOGLE_CLIENT_SECRET": "",
88+
"GITLAB_HOST": "",
89+
"GITLAB_CLIENT_ID": "",
90+
"GITLAB_CLIENT_SECRET": "",
91+
"GITEA_HOST": "",
92+
"GITEA_CLIENT_ID": "",
93+
"GITEA_CLIENT_SECRET": "",
94+
}
95+
with patch.dict("os.environ", env):
96+
call_command("configure_instance", stdout=out)
97+
98+
for key in ["IS_GOOGLE_ENABLED", "IS_GITHUB_ENABLED", "IS_GITLAB_ENABLED", "IS_GITEA_ENABLED"]:
99+
config = InstanceConfiguration.objects.get(key=key)
100+
assert config.value == "0", f"{key} should be '0' without credentials"

0 commit comments

Comments
 (0)