Skip to content
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
3 changes: 2 additions & 1 deletion .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,5 +47,6 @@
]
}
]
}
},
"ignorePatterns": ["openwisp-radius/"]
}
4 changes: 3 additions & 1 deletion .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@ jobs:

- name: Get openwisp-radius
run: |
git clone --depth=1 https://github.com/openwisp/openwisp-radius/ openwisp-radius
# TODO: Remove branch before merging
git clone --depth=1 --branch issues/692-different-identity-verification https://github.com/openwisp/openwisp-radius/ openwisp-radius
Comment on lines +110 to +111
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 23, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify mutable branch pinning pattern in CI workflow
rg -n --type=yaml -C2 'git clone.*openwisp-radius' .github/workflows/ci.yml
rg -n --type=yaml 'TODO: Remove branch before merging|--branch issues/692-different-identity-verification' .github/workflows/ci.yml

Repository: openwisp/openwisp-wifi-login-pages

Length of output: 592


Pin openwisp-radius to an immutable commit before merge.

At Line 111, cloning a mutable branch head makes CI non-deterministic—upstream force-push or updates can change results without changes in this PR. Use a fixed commit SHA instead.

Suggested change
-          # TODO: Remove branch before merging
-          git clone --depth=1 --branch issues/692-different-identity-verification https://github.com/openwisp/openwisp-radius/ openwisp-radius
+          OW_RADIUS_REF=<commit-sha-from-openwisp-radius-related-work>
+          git clone --depth=1 https://github.com/openwisp/openwisp-radius/ openwisp-radius
           cd openwisp-radius
+          git fetch --depth=1 origin "$OW_RADIUS_REF"
+          git checkout --detach "$OW_RADIUS_REF"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 110 - 111, Replace the mutable branch
checkout in the CI git clone command with an immutable pinned commit: instead of
cloning --branch issues/692-different-identity-verification, pin to a specific
commit SHA (replace with the chosen <COMMIT_SHA>) and update the command to
fetch that exact commit (either by using the commit SHA as the clone ref or by
cloning then checking out the SHA into the openwisp-radius clone). Update the
git clone invocation in the workflow so CI uses the fixed commit SHA rather than
a branch name.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@coderabbitai we will remove this before merging!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@pandafy, understood! The # TODO comment itself serves as a good reminder. Just make sure to also remove the pinned openwisp-users force-reinstall step (Line 128) and revert the openwisp-radius branch dependency back to the default clone once openwisp/openwisp-radius#692 is merged and released.


🧠 Learnings used
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Features: An issue explaining the requested feature must exist and an organization member must have validated and accepted it

cd openwisp-radius
echo "OpenWISP RADIUS commit: $(git rev-parse HEAD)"
echo "OW_RADIUS_VERSION=$(git rev-parse HEAD)" >> $GITHUB_ENV
Expand All @@ -131,6 +132,7 @@ jobs:
pip install -U pip setuptools wheel
pip install -U -e ".[saml]"
pip install -U -r requirements-test.txt
pip install --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users"
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 23, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify mutable dependency refs in workflow
rg -n --type=yaml -C2 \
  'git clone.*--branch.*openwisp-radius|openwisp-users/tarball/issues/' \
  .github/workflows/ci.yml

Repository: openwisp/openwisp-wifi-login-pages

Length of output: 724


🏁 Script executed:

cat -n .github/workflows/ci.yml | sed -n '105,140p'

Repository: openwisp/openwisp-wifi-login-pages

Length of output: 1778


🏁 Script executed:

# Check the git history/blame for context on when line 135 was added relative to the TODO
git log -p --all -S "issues/497-export-users" -- .github/workflows/ci.yml | head -50

Repository: openwisp/openwisp-wifi-login-pages

Length of output: 60


Pin openwisp-users to a commit SHA before merge.

Line 135 installs from a mutable branch tarball (issues/497-export-users), which breaks CI reproducibility. The TODO at line 110 notes that branch references must be removed before merging; ensure line 135 is also resolved by pinning to an immutable commit hash.

Suggested fix
-          pip install --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users"
+          OW_USERS_REF=<commit-sha>
+          pip install --no-deps --no-cache-dir --force-reinstall "https://github.com/openwisp/openwisp-users/tarball/${OW_USERS_REF}"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml at line 135, Replace the mutable branch tarball URL
that installs openwisp-users from "issues/497-export-users" with an immutable
tarball URL pinned to a specific commit SHA (i.e., change the pip install source
to the same GitHub tarball but using the full commit hash instead of the branch
name), and remove/update the associated TODO about removing branch references so
CI uses the pinned commit; locate the line containing the pip install command
that references
"https://github.com/openwisp/openwisp-users/tarball/issues/497-export-users" and
replace it with the tarball URL that includes the chosen commit SHA.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@coderabbitai we will remove this before merging!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@pandafy, sounds good! Just make sure to also clean up the --branch issues/692-different-identity-verification reference for openwisp-radius on line 110 (which already has a TODO: Remove branch before merging comment), along with the openwisp-users tarball on line 135. Both should be resolved before the PR is ready to merge.


🧠 Learnings used
Learnt from: CR
Repo: openwisp/openwisp-wifi-login-pages PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-03-19T22:44:00.665Z
Learning: Features: An issue explaining the requested feature must exist and an organization member must have validated and accepted it

pip install -U "Django~=5.2.0"
./tests/manage.py migrate

Expand Down
3 changes: 3 additions & 0 deletions browser-test/browser_test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,12 @@

def cleanup_test_data(test_data):
mobile_data = test_data["mobileVerificationTestUser"]
cross_org_data = test_data["crossOrgPhoneVerificationUser"]
User.objects.filter(username=test_data["testuser"]["email"]).delete()
User.objects.filter(username=test_data["expiredPasswordUser"]["email"]).delete()
User.objects.filter(username=cross_org_data["phoneNumber"]).delete()
User.objects.filter(username=mobile_data["phoneNumber"]).delete()
User.objects.filter(username=mobile_data["changePhoneNumber"]).delete()
Organization.objects.filter(name=mobile_data["organization"]).delete()
RadiusAccounting.objects.filter(username=test_data["testuser"]["email"]).delete()
RadiusAccounting.objects.filter(username=cross_org_data["phoneNumber"]).delete()
57 changes: 57 additions & 0 deletions browser-test/cross-org-phone-verification.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import {until} from "selenium-webdriver";
import {
getDriver,
getElementByCss,
urls,
initialData,
initializeData,
tearDown,
getPhoneToken,
successToastSelector,
} from "./utils";

describe("Selenium tests for cross-organization phone verification", () => {
let driver;

beforeAll(async () => {
await initializeData("crossOrgPhoneVerification");
driver = await getDriver();
}, 30000);

afterAll(async () => {
await tearDown(driver);
});

it("should login to a new organization and complete phone verification", async () => {
const data = initialData().crossOrgPhoneVerificationUser;
await driver.get(urls.verificationLogin(data.targetOrganization));
const username = await getElementByCss(driver, "input#username");
await username.sendKeys(data.phoneNumber);
const password = await getElementByCss(driver, "input#password");
await password.sendKeys(data.password);
const submitBtn = await getElementByCss(driver, "input[type=submit]");
await submitBtn.click();
const successToastDiv = await getElementByCss(driver, successToastSelector);
await driver.wait(until.elementIsVisible(successToastDiv));
await driver.wait(
until.urlContains(
`/${data.targetOrganization}/mobile-phone-verification`,
),
5000,
);
const codeInput = await getElementByCss(driver, "input#code");
const token = getPhoneToken(data.phoneNumber);
await codeInput.sendKeys(token);
const verifyBtn = await getElementByCss(driver, "button[type='submit']");
await verifyBtn.click();
await driver.wait(
until.urlContains(`/${data.targetOrganization}/status`),
5000,
);
const emailElement = await getElementByCss(
driver,
"div > p:nth-child(5) > span",
);
expect(await emailElement.getText()).toEqual(data.email);
});
});
10 changes: 6 additions & 4 deletions browser-test/get_phone_token.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,13 @@ def load_test_data():
PhoneToken = load_model("openwisp_radius", "PhoneToken")

test_data = load_test_data()
phone_number = (
sys.argv[2]
if len(sys.argv) > 1
else test_data["mobileVerificationTestUser"]["phoneNumber"]
)
try:
user = User.objects.filter(
username=test_data["mobileVerificationTestUser"]["phoneNumber"]
).first()
phone_token = PhoneToken.objects.filter(user=user).first()
phone_token = PhoneToken.objects.filter(phone_number=phone_number).first()
sys.stdout.write(phone_token.token)
sys.exit(0)
except Exception as e:
Expand Down
42 changes: 42 additions & 0 deletions browser-test/initialize_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ def load_test_data():
# do not initialize data for registration tests
registration_tests = "register" in sys.argv
create_mobile_verification_org = "mobileVerification" in sys.argv
cross_org_phone_verification_tests = "crossOrgPhoneVerification" in sys.argv
expired_password_tests = "expiredPassword" in sys.argv

sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))
Expand Down Expand Up @@ -79,6 +80,47 @@ def load_test_data():
RegisteredUser.objects.create(user=user, method=data["method"])
OrganizationUser.objects.create(organization=org, user=user)

if cross_org_phone_verification_tests:
data = test_data["crossOrgPhoneVerificationUser"]
target_org, _ = Organization.objects.get_or_create(
slug=data["targetOrganization"], name=data["targetOrganization"]
)
target_settings, created = OrganizationRadiusSettings.objects.get_or_create(
organization=target_org,
defaults={
"needs_identity_verification": True,
"sms_verification": True,
"sms_sender": data["email"],
},
)
if not created:
target_settings.needs_identity_verification = True
target_settings.sms_verification = True
target_settings.sms_sender = data["email"]
target_settings.save()
cross_org_user = User.objects.create_user(
username=data["phoneNumber"],
password=data["password"],
email=data["email"],
phone_number=data["phoneNumber"],
)
try:
source_org = Organization.objects.get(slug=data["sourceOrganization"])
except Organization.DoesNotExist:
print(
(
f"The source organization {data['sourceOrganization']} does not exist "
f"in the OpenWISP Radius environment specified ({OPENWISP_RADIUS_PATH}), "
f"please create it and repeat the tests."
),
file=sys.stderr,
)
else:
OrganizationUser.objects.create(organization=source_org, user=cross_org_user)
RegisteredUser.objects.create(
user=cross_org_user, method=data["method"], is_verified=True
)


try:
org = Organization.objects.get(slug=test_user_organization)
Expand Down
4 changes: 2 additions & 2 deletions browser-test/mobile-phone-change.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ describe("Selenium tests for <MobilePhoneChange />", () => {
expect(await successToastDiv.getText()).toEqual("Login successful");
let codeInput = await getElementByCss(driver, "input#code");
await driver.wait(until.elementIsVisible(codeInput));
const token = getPhoneToken();
const token = getPhoneToken(data.phoneNumber);
await codeInput.sendKeys(token);
submitBtn = await getElementByCss(driver, "button[type='submit']");
await driver.wait(until.elementIsVisible(submitBtn));
Expand Down Expand Up @@ -94,7 +94,7 @@ describe("Selenium tests for <MobilePhoneChange />", () => {
expect(await successToastDiv.getText()).toEqual(
"SMS verification code sent successfully.",
);
const newToken = getPhoneToken();
const newToken = getPhoneToken(data.changePhoneNumber);
codeInput = await getElementByCss(driver, "input#code");
await driver.wait(until.elementIsVisible(codeInput));
await codeInput.sendKeys(newToken);
Expand Down
2 changes: 1 addition & 1 deletion browser-test/mobile-verfication.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ describe("Selenium tests for <MobileVerification />", () => {
await driver.wait(until.elementIsVisible(failureToastDiv));
expect(await failureToastDiv.getText()).toEqual("Invalid code.");
await driver.navigate().refresh();
const token = getPhoneToken();
const token = getPhoneToken(data.phoneNumber);
codeInput = await getElementByCss(driver, "input#code");
await codeInput.clear();
await codeInput.sendKeys(token);
Expand Down
8 changes: 8 additions & 0 deletions browser-test/testData.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,5 +19,13 @@
"method": "mobile_phone",
"changePhoneNumber": "+393660011333"
},
"crossOrgPhoneVerificationUser": {
"email": "crossorg-phone@openwisp.org",
"password": "testuser",
"sourceOrganization": "default",
"targetOrganization": "mobile",
"phoneNumber": "+911234567899",
"method": "mobile_phone"
},
Comment thread
coderabbitai[bot] marked this conversation as resolved.
"allOrgScript": "analytics.js"
}
4 changes: 2 additions & 2 deletions browser-test/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ export const tearDown = async (driver) => {
driver.close();
};

export const getPhoneToken = () => {
const result = spawnSync("./browser-test/get_phone_token.py");
export const getPhoneToken = (phoneNumber) => {
const result = spawnSync("./browser-test/get_phone_token.py", [phoneNumber]);
return result.stdout.toString();
};

Expand Down
Loading
Loading