Repository navigation
make sure pre saml login is enabled after saml is disabled - #13953
Open
DaanHoogland wants to merge 3 commits into
Open
DaanHoogland wants to merge 3 commits into
DaanHoogland wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #13953 +/- ##
=============================================
- Coverage 19.72% 3.70% -16.03%
=============================================
Files 6371 493 -5878
Lines 575738 42558 -533180
Branches 70471 8063 -62408
=============================================
- Hits 113582 1578 -112004
+ Misses 449810 40751 -409059
+ Partials 12346 229 -12117
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🔴 Test Coverage Grade:
|
| Metric | Value |
|---|---|
| Line coverage | 25.50% |
| Branch coverage | 19.80% |
Grade Scale
| Grade | Line Coverage | Meaning |
|---|---|---|
| 🟢 A | ≥ 80% | Excellent - this code sleeps well at night 😴 |
| 🟡 B | 60-79% | Good - almost there, don't stop now 😉 |
| 🟠 C | 40-59% | Acceptable - your code is wearing a seatbelt, but no airbags 😬 |
| 🔴 D | 20-39% | Marginal - boldly shipping where no test has gone before 🖖 |
| ⛔ F | < 20% | Failing - tests? what tests? 🔥 |
Branch coverage is shown as a secondary signal. Grade is determined by line coverage.
View full Actions run
|
Member
|
@blueorangutan package |
|
@kiranchavala a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19161 |
kiranchavala
left a comment
kiranchavala
left a comment
Member
There was a problem hiding this comment.
Issue is present
-
Configure LDAP server . add a ldap account
-
Log in as the ldap user
(localcloud) 🐱 > list users username=kiran listall=true filter=username,id,usersource
⣷ 😸 trying to log in...
{
"count": 1,
"user": [
{
"id": "ad5e867e-5ce2-43e0-ac4c-3c5450f8f8ca",
"username": "kiran",
"usersource": "ldap"
}
]
}
-
Ldap login succesfull
-
Configure saml on the ldap user
(localcloud) 🐱 > list users username=kiran listall=true filter=username,id,usersource
{
"count": 1,
"user": [
{
"id": "ad5e867e-5ce2-43e0-ac4c-3c5450f8f8ca",
"username": "kiran",
"usersource": "saml2"
}
]
}
-
Saml login successfull
-
Disable Saml authentication on the user
(localcloud) 🐱 > list users username=kiran listall=true filter=username,id,usersource
{
"count": 1,
"user": [
{
"id": "ad5e867e-5ce2-43e0-ac4c-3c5450f8f8ca",
"username": "kiran",
"usersource": "saml2disabled"
}
]
}
- Unable to login with ldap
mysql> select * from user_details;
+----+---------+---------------+-------+---------+
| id | user_id | name | value | display |
+----+---------+---------------+-------+---------+
| 2 | 5 | PreSamlSource | LDAP | 1 |
+----+---------+---------------+-------+---------+
1 row in set (0.00 sec)
- logs
2026-09-15 10:48:28,138 DEBUG [c.c.a.ApiServlet] (qtp1151704483-16:[ctx-c49d6da0]) (logid:a8550e5c) ===START=== 10.0.3.251 -- POST login
2026-09-15 10:48:28,138 DEBUG [c.c.a.ApiSessionListener] (qtp1151704483-16:[ctx-c49d6da0]) (logid:a8550e5c) Session created by Id : node0tjykvf1cdsxl1kne3qnrxnejw9 , session: Session@add86b1{id=node0tjykvf1cdsxl1kne3qnrxnejw9,x=node0tjykvf1cdsxl1kne3qnrxnejw9.node0,req=1,res=true} , source: Session@add86b1{id=node0tjykvf1cdsxl1kne3qnrxnejw9,x=node0tjykvf1cdsxl1kne3qnrxnejw9.node0,req=1,res=true} , event: javax.servlet.http.HttpSessionEvent[source=Session@add86b1{id=node0tjykvf1cdsxl1kne3qnrxnejw9,x=node0tjykvf1cdsxl1kne3qnrxnejw9.node0,req=1,res=true}]
2026-09-15 10:48:28,142 DEBUG [c.c.u.AccountManagerImpl] (qtp1151704483-16:[ctx-c49d6da0]) (logid:a8550e5c) Attempting to log in user: kiran in domain 1
2026-09-15 10:48:28,143 DEBUG [c.c.u.AccountManagerImpl] (qtp1151704483-16:[ctx-c49d6da0]) (logid:a8550e5c) Unable to authenticate user with username kiran in domain 1
2026-09-15 10:48:28,144 DEBUG [c.c.u.AccountManagerImpl] (qtp1151704483-16:[ctx-c49d6da0]) (logid:a8550e5c) User: kiran in domain 1 has failed to log in
2026-09-15 10:48:28,555 DEBUG [c.c.a.ApiSessionListener] (qtp1151704483-16:[ctx-c49d6da0]) (logid:a8550e5c) Session destroyed by Id : node0tjykvf1cdsxl1kne3qnrxnejw9 , session: Session@add86b1{id=node0tjykvf1cdsxl1kne3qnrxnejw9,x=node0tjykvf1cdsxl1kne3qnrxnejw9.node0,req=1,res=true} , source: Session@add86b1{id=node0tjykvf1cdsxl1kne3qnrxnejw9,x=node0tjykvf1cdsxl1kne3qnrxnejw9.node0,req=1,res=true} , event: javax.servlet.http.HttpSessionEvent[source=Session@add86b1{id=node0tjykvf1cdsxl1kne3qnrxnejw9,x=node0tjykvf1cdsxl1kne3qnrxnejw9.node0,req=1,res=true}]
2026-09-15 10:48:28,556 DEBUG [c.c.a.ApiServlet] (qtp1151704483-16:[ctx-c49d6da0]) (logid:a8550e5c) Authentication failure: {"loginresponse":{"uuidList":[],"errorcode":531,"errortext":"Failed to authenticate user kiran in domain 1; please provide valid credentials"}}
2026-09-15 10:48:28,556 DEBUG [c.c.a.ApiServlet] (qtp1151704483-16:[ctx-c49d6da0]) (logid:a8550e5c) ===END=== 10.0.3.251 -- POST login
Contributor
Author
@kiranchavala , did you set enable.login.with.disabled.saml=true during this test? |
…ogin after disabling saml
Contributor
Author
|
@kiranchavala , I added a test to actually confirm the behaviour is maintained as you describe: |
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
5 open issues
user.getSource().equals(...)will throw aNullPointerExceptionif the user's source is null… · Newuser.getSource().toString()but later parsed with… · NewConfigKeyinternals and “reset” by forcing_value=nulland… · New_userDao,_domainMgr).… · NewauthorizeUser(..., enable=true)and asserts that no detail is added; it’s not… · New
What changed in this PR
Updates SAML enable/disable behavior to preserve and restore a user’s pre-SAML authentication source (e.g., LDAP) when SAML is disabled (optionally via config), instead of always falling back to UNKNOWN.
Changes:
- Persist the user’s pre-SAML
User.Sourceintouser_detailswhen enabling SAML (skipping SAML-related and null sources). - Restore the persisted source when disabling SAML if
enable.login.with.disabled.saml=true, otherwise setSAML2DISABLED. - Add unit tests covering persistence, restoration, and fallback behaviors.
| File | Description |
|---|---|
| plugins/user-authenticators/saml2/src/main/java/org/apache/cloudstack/saml/SAML2AuthManagerImpl.java | Store a pre-SAML source detail on enable; restore it on disable when the config flag is enabled. |
| plugins/user-authenticators/saml2/src/test/java/org/apache/cloudstack/SAML2AuthManagerImplTest.java | Add tests for storing/restoring pre-SAML source and for config-driven disable behavior. |
Review effort: Lite
Comment on lines
+478
to
+481
| private boolean disableSAMLForUser(UserVO user) { | ||
| if (!user.getSource().equals(User.Source.SAML2)) { | ||
| return false; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Suggested change
| private boolean disableSAMLForUser(UserVO user) { | |
| if (!user.getSource().equals(User.Source.SAML2)) { | |
| return false; | |
| } | |
| private boolean disableSAMLForUser(UserVO user) { | |
| if (!User.Source.SAML2.equals(user.getSource())) { | |
| return false; | |
| } |
Comment on lines
+471
to
+473
| if (user.getSource() != null && !User.Source.SAML2.equals(user.getSource()) && !User.Source.SAML2DISABLED.equals(user.getSource())) { | ||
| userDetailsDao.addDetail(user.getId(), PRE_SAML_SOURCE_DETAIL_KEY, user.getSource().toString(), false); | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Suggested change
| if (user.getSource() != null && !User.Source.SAML2.equals(user.getSource()) && !User.Source.SAML2DISABLED.equals(user.getSource())) { | |
| userDetailsDao.addDetail(user.getId(), PRE_SAML_SOURCE_DETAIL_KEY, user.getSource().toString(), false); | |
| } | |
| if (user.getSource() != null && !User.Source.SAML2.equals(user.getSource()) && !User.Source.SAML2DISABLED.equals(user.getSource())) { | |
| userDetailsDao.addDetail(user.getId(), PRE_SAML_SOURCE_DETAIL_KEY, user.getSource().name(), false); | |
| } |
Comment on lines
+105
to
+131
| private static <T> void setConfigKeyValue(ConfigKey<T> configKey, T value) { | ||
| try { | ||
| Field valueField = ConfigKey.class.getDeclaredField("_value"); | ||
| valueField.setAccessible(true); | ||
| valueField.set(configKey, value); | ||
|
|
||
| Field dynamicField = ConfigKey.class.getDeclaredField("_isDynamic"); | ||
| dynamicField.setAccessible(true); | ||
| dynamicField.setBoolean(configKey, false); | ||
| } catch (IllegalAccessException | NoSuchFieldException e) { | ||
| throw new RuntimeException("Failed to set ConfigKey value", e); | ||
| } | ||
| } | ||
|
|
||
| private static void resetConfigKeyValue(ConfigKey<?> configKey) { | ||
| try { | ||
| Field valueField = ConfigKey.class.getDeclaredField("_value"); | ||
| valueField.setAccessible(true); | ||
| valueField.set(configKey, null); | ||
|
|
||
| Field dynamicField = ConfigKey.class.getDeclaredField("_isDynamic"); | ||
| dynamicField.setAccessible(true); | ||
| dynamicField.setBoolean(configKey, true); | ||
| } catch (IllegalAccessException | NoSuchFieldException e) { | ||
| throw new RuntimeException("Failed to reset ConfigKey value", e); | ||
| } | ||
| } |
Comment on lines
+183
to
+184
| @Test | ||
| public void testAuthorizeUserDoesNotRestorePreSamlSourceWhenAlreadyAuthorized() { |
|
Copilot stopped reviewing on behalf of
DaanHoogland due to an error
October 20, 2026 09:25
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR...
Fixes: #12595
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?