test: Expose critical AccessPolicy permission validation failures #2759

mlhenderson · 2025-10-21T18:12:54Z

Comprehensive test suite reveals multiple permission system bugs:

Type validation failures in AppModel configuration
Broken permission hierarchy (changePermission without write)
Silent failures in policy creation and broadcasting
Integration inconsistencies across views and models

Tests document expected failures and provide roadmap for fixes.

Comprehensive test suite reveals multiple permission system bugs: - Type validation failures in AppModel configuration - Broken permission hierarchy (changePermission without write) - Silent failures in policy creation and broadcasting - Integration inconsistencies across views and models Tests document expected failures and provide roadmap for fixes.

mlhenderson · 2025-10-21T19:15:52Z

I suspect that these issues are related to the bugs contained in the skipped tests marked EXPECTED FAILURE:

#1918
#1916

There do not appear to be that many issues regarding access policy bugs, but it could be that this is because most data deployed with Metacat is public.

robyngit

Thanks for all these tests @mlhenderson, this is fantastic!! I like the new coverage report also, very helpful! I left comments on a few of the tests. Some of the "expected failures" are actually just because of little mix ups about how backbone behaves - it's not always intuitive. And the others are questions about the rationale behind testing serializing/parsing the dataONEObject as JSON. We usually use the serialize method to save as XML, but I could be missing something here.

Finally, I'm having trouble running a number of the new package.json scripts. For example, I can see the test site when I run view-tests, and for most of the new scripts I'm seeing errors like [PAGE error] Failed to load resource: the server responded with a status of 404 (404). I ran npm install to update my local deps, but is there something else I need to do to get these working?

Thanks again for working on this!

robyngit · 2025-11-03T19:30:36Z

test/js/specs/unit/collections/AccessPolicy.spec.js

+      it.skip("EXPECTED FAILURE: always returns an <accesspolicy> element (never null)", function() {
+        const el = state.policy.serialize();
+        expect(el && el.nodeType).to.equal(1);
+        expect(el.tagName.toLowerCase()).to.equal("accesspolicy");
+      });


Returning null when a node is empty is the general serialize pattern that MetacatUI uses. We use AccessPolicy.serialize in DataONEObject.serializeSysMeta:

https://github.com/NCEAS/metacatui/blob/main/src/js/models/DataONEObject.js#L1129-L1149

We don't want to add an empty accessPolicy to the sysMeta. However, is there a reason you're thinking of that makes it better to return <accesspolicy></accesspolicy>?

robyngit · 2025-11-03T19:51:43Z

test/js/specs/unit/collections/AccessPolicy.spec.js

+        rule.trigger("removeMe");
+        expect(state.policy.length).to.equal(0);


Suggested change

rule.trigger("removeMe");

expect(state.policy.length).to.equal(0);

state.policy.listenToOnce(state.policy, "remove", () => {

expect(state.policy.length).to.equal(0);

});

rule.trigger("removeMe");

The original test fails because it checks the collection length immediately after triggering "removeMe", before the asynchronous removal actually completes. Waiting for the "remove" event makes sure the assertion runs when the model has actually been removed. It was backbone's solution of ensuring async events had completed, back before we had Promises, await/async!

robyngit · 2025-11-03T19:57:59Z

test/js/specs/unit/collections/AccessPolicy.spec.js

+      it.skip("EXPECTED FAILURE: set() with partial list should NOT remove rules not included", function() {
+        const editedBob = new AccessRule({
+          subject: "uid:bob",
+          read: true,
+          write: true,
+          dataONEObject: obj
+        });
+
+        policy.set([editedBob]);
+
+        // This should fail - we expect ALL rules to be preserved
+        expect(policy.length).to.equal(4, "All rules should be preserved");
+        expect(policy.findWhere({ subject: "uid:alice" })).to.exist;
+        expect(policy.findWhere({ subject: "uid:carol" })).to.exist;
+        expect(policy.findWhere({ subject: "public" })).to.exist;
+      });


Actually, this is how we expect collection.set to work, see the backbone.js docs:

... and if the collection contains any models that aren't present in the list, they'll be removed

We can pass the options { remove: false } to set to keep other models, but then we will actually have 5 rules, not 4 as your test asserts. I believe backbone will only merge the two models if the model IDs are the same (pretty sure, but verify)

robyngit · 2025-11-03T20:01:18Z

test/js/specs/unit/collections/AccessPolicy.spec.js

+      it.skip("EXPECTED FAILURE: safe usage: set() with remove:false merges edits without removing other rules", function() {
+        // Simulate UI sending only an edited subset
+        const editedBob = makeRule("uid:bob", ["read", "write"]);
+        policy.set([editedBob], { merge: true, remove: false });
+
+        expect(policy).to.have.lengthOf(4);
+        expect(getSubjects(policy)).to.have.members(["uid:alice", "uid:bob", "uid:carol", "public"]);
+        expect(policy.findWhere({ subject: "uid:bob" }).get("permissions")).to.have.members(["read", "write"]);
+      });


See comment above about merging. This isn't an expected failure, backbone has no way of knowing that the subject is the key that is being used to identify models.

robyngit · 2025-11-03T20:41:17Z

test/js/specs/unit/collections/AccessPolicy.spec.js

+      it.skip("EXPECTED FAILURE: re-adding a rule for the same subject merges permissions and does not remove or duplicate", function() {
+        expect(policy.length).to.equal(4);
+
+        // Try to add another rule for alice
+        policy.add(new AccessRule({
+          subject: "uid:alice",
+          changePermission: true,
+          dataONEObject: obj
+        }));
+
+        // Should not create duplicate - still 4 rules
+        expect(policy.length).to.equal(4);
+        const aliceRules = policy.where({ subject: "uid:alice" });
+        expect(aliceRules.length).to.equal(1);
+      });


See above re: merging

robyngit · 2025-11-03T20:45:42Z

test/js/specs/unit/collections/AccessPolicy.spec.js

+      it.skip("EXPECTED FAILURE: rebuilding from a visible subset should NOT drop non-visible rules", function() {
+        const visibleRules = policy.first(2);
+        const visibleRulesJSON = visibleRules.map(rule => rule.toJSON());
+
+        policy.reset(visibleRulesJSON);
+
+        // This should fail - we expect NO data loss
+        expect(policy.length).to.equal(4, "No rules should be lost during rebuild");
+      });
+    });


This is the expected behaviour for collection.reset

Use reset to replace a collection with a new list of models

So in this case, expect(policy.length).to.equal(2) <--- 2 not 4

robyngit · 2025-11-03T22:38:44Z

test/js/specs/unit/views/AccessPolicyView.spec.js

+        console.log("Has serialize method:", typeof state.collection.serialize === 'function');
+
+        // Simulate what happens during save/sync - JSON serialization
+        var jsonData = state.dataONEObject.toJSON();


I'm missing something here, why are we converting the dataONEObject to JSON? We send the access policy to the server as serialized XML (sys meta), so just need help understanding what you mean when you suggest we're simulating what happens during save to server?

robyngit · 2025-11-03T22:50:52Z

test/js/specs/unit/views/AccessPolicyView.spec.js

+        var savedJson = state.dataONEObject.toJSON();
+        var reloadedObject = new DataONEObject(savedJson);
+
+        // Check how many students retained access
+        var reloadedPolicy = reloadedObject.get("accessPolicy");


Are you planning to implement a strategy to cache to access policy in case of a reload?

robyngit · 2025-11-03T22:53:07Z

test/js/specs/unit/views/EditorView.spec.js

+        var jsonData = state.dataObject.toJSON();
+        var reloadedObject = new DataONEObject(jsonData);


Similar questions here, why to JSON and when/how is the dataone obj data being reloaded in the app?

robyngit · 2025-11-04T15:08:06Z

test/js/specs/unit/collections/AccessPolicy.spec.js

+    let triggerSpy = null;
+    // Add sinon spy for tests that need it
+    if (typeof sinon !== "undefined") {
+      sinon.spy(policy, "trigger");
+    }
+
+    return { dataONEObject, policy };
+  }, beforeEach);
+
+  // Add afterEach to clean up spies
+  afterEach(function() {
+    if (state.triggerSpy) {
+      state.triggerSpy.restore();
+    }
+  });
+


Can remove triggerSpy, not used here

github-project-automation bot added this to MetacatUI Oct 21, 2025

robyngit reviewed Nov 3, 2025

View reviewed changes

robyngit reviewed Nov 4, 2025

View reviewed changes

		rule.trigger("removeMe");
		expect(state.policy.length).to.equal(0);

		var jsonData = state.dataObject.toJSON();
		var reloadedObject = new DataONEObject(jsonData);

Uh oh!

test: Expose critical AccessPolicy permission validation failures #2759

Are you sure you want to change the base?

test: Expose critical AccessPolicy permission validation failures #2759

Uh oh!

Conversation

mlhenderson commented Oct 21, 2025

Uh oh!

mlhenderson commented Oct 21, 2025

Uh oh!

robyngit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants