MLE-28498 11.3.5 Test Fixes for Security Update#1074
Conversation
This contains permission fixes for testing as a result of the 11.3.5 changes.
Updated transforms tests to expect rest-transform-user as a result of 11.3.5 changes from ML-28684.
Copilot fix for flakey dmsdk tests
jonmille
requested review from
BillFarber,
anu3990,
rjrudin and
stevebio
as code owners
|
Latest run with a configured branch in Jenkins skipping pull request tests to run regressions: https://ml-clt-jenkins.progress.com/blue/organizations/jenkins/devexp%2FNode-Client%2FNode-client-api/detail/MLE-28498-11-3-5-regression-analysis/3/pipeline. fromDocs issues are still being addressed |
There was a problem hiding this comment.
Pull request overview
Updates the test suite and test-app security configuration to align with MarkLogic Server 11.3.5 security changes (privileges/roles/permissions), particularly around REST transform execution and data movement tests.
Changes:
- Update transform-related tests to expect transforms to run as
rest-transform-userand adjust async error handling. - Fix
readAlltest completion timing to avoid callingdone()beforestreamToArrayfinishes. - Update test-app security users/roles to include new/required roles and privileges (e.g.,
rest-invoke-user, added privileges onrest-evaluator).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test-complete/nodejs-transform-javascript.js | Updates transform user expectation and switches to promise .catch(done) handling in two tests. |
| test-complete/nodejs-dmsdk-readall-1.js | Fixes premature done() calls and adds early returns after done(err). |
| test-basic/documents-transform.js | Updates JavaScript transform tests to expect rest-transform-user. |
| test-app/src/main/ml-config/security/users/rest-writer.json | Adds rest-extension-user role to rest-writer. |
| test-app/src/main/ml-config/security/users/rest-transform-user.json | Introduces a dedicated transform execution user for tests. |
| test-app/src/main/ml-config/security/users/rest-temporal-writer.json | Adds rest-extension-user role and normalizes role array formatting. |
| test-app/src/main/ml-config/security/users/rest-reader.json | Adds rest-extension-user and rest-invoke-user roles. |
| test-app/src/main/ml-config/security/users/rest-admin.json | Adds additional roles (rest-evaluator, rest-extension-user, sparql-update-user). |
| test-app/src/main/ml-config/security/roles/rest-invoke-user.json | Adds a new role granting xdmp-login to support transform invocation behavior changes. |
| test-app/src/main/ml-config/security/roles/rest-evaluator.json | Expands inherited roles and grants additional execute privileges required post-security update. |
| @@ -0,0 +1,11 @@ | |||
| { | |||
| "role-name": "rest-invoke-user", | |||
There was a problem hiding this comment.
This role name is surprising - I would assume that it grants invoke privileges. I would rename it to "login-role" or something self-documenting like that. You could then have multiple roles that require the login privilege inherit this role.
There was a problem hiding this comment.
Renamed to rest-login.json and role-name to rest-login
| @@ -1,8 +1,6 @@ | |||
| { | |||
| "user-name": "rest-reader", | |||
There was a problem hiding this comment.
Would be useful to rename this - probably in a separate PR - given that it now refers to 3 things - a user, and then OOTB role and privilege. It's already confusing enough that there are OOTB roles and privileges that are both named rest-reader and rest-writer.
| documents[0].content.should.have.property('timestamp'); | ||
| documents[0].content.should.have.property('userName'); | ||
| documents[0].content.userName.should.eql('rest-writer'); | ||
| documents[0].content.userName.should.eql('rest-transform-user'); // MLE-28684: transforms now run as rest-transform-user |
There was a problem hiding this comment.
I generally frown on this sort of comment - an assertion message would be far more useful. I think the main thing someone would be wondering is - hmm, why is this user expected? What is special about this user? A good assertion message would state "As of such and such markLogic version, a user now needs (fill in the blank) privilege in order to do such and such". The Jira ticket ID can be in version history too, it doesn't need to be in a comment or assertion message.
There was a problem hiding this comment.
Added an assertion message to all the areas where this comment applied with the reason and marklogic versions where this change was introduced
Renamed rest-invoke-user.json -> rest-login.json Added message to asserts with transform tests checking userName equal to rest-transform-user
After confirming with the MarkLogic Server team, op:from-docs is only supported in MLS 12.1 since it is a new feature. I have added a skip for anything lower than MLS 12.1
This PR primarily contains privilege, role, and permission test fixes as a result of the MLS 11.3.5 security fixes. Thes fixes were also applied to 12.0.2 and 12.1.0.
Added a condition for optic-fromDocs.js tests to only run on MLS 12.1 as it is a new feature only in MLS 12.1
Jira Story: https://progresssoftware.atlassian.net/browse/MLE-28498