#556 constrain URLs

Author: joepio

CHANGELOG.md

@@ -65,6 +65,8 @@ See [STATUS.md](server/STATUS.md) to learn more about which features will remain
 - Add systemd instructions to readme #271
 - Improve check_append error #558
 - Fix errors on successful export / import #565
+- Most Collection routes now live under `/collections`, e.g. `/collections/agents` instead of `/agents`. #556
+- Constrain new URLs. Commits for new Resources are now only valid if their parent is part of the current URL. So it's no longer possible to create `/some-path/new-resource` if `new-resource` is its parent is not in its URL. #556
 
 ## [v0.34.0] - 2022-10-31
 

lib/src/atomic_url.rs

@@ -36,9 +36,9 @@ impl AtomicUrl {
     pub fn set_route(&self, route: Routes) -> Self {
         let path = match route {
             Routes::AllVersions => "/all-versions".to_string(),
-            Routes::Agents => "/agents".to_string(),
+            Routes::Agents => "/collections/agents".to_string(),
             Routes::Collections => "/collections".to_string(),
-            Routes::Commits => "/commits".to_string(),
+            Routes::Commits => "/collections/commits".to_string(),
             Routes::CommitsUnsigned => "/commits-unsigned".to_string(),
             Routes::Endpoints => "/endpoints".to_string(),
             Routes::Import => "/import".to_string(),

lib/src/collections.rs

@@ -393,14 +393,24 @@ pub fn create_collection_resource_for_class(
 ) -> AtomicResult<Resource> {
     let class = store.get_class(class_subject)?;
 
+    // We use the `Collections` collection as the parent for all collections.
+    // This also influences their URLs.
+    let is_collections_collection = class.subject == urls::COLLECTION;
+
     // Pluralize the shortname
     let pluralized = match class.shortname.as_ref() {
         "class" => "classes".to_string(),
         "property" => "properties".to_string(),
         other => format!("{}s", other),
     };
 
-    let mut collection = CollectionBuilder::class_collection(&class.subject, &pluralized, store);
+    let path = if is_collections_collection {
+        "/collections".to_string()
+    } else {
+        format!("/collections/{}", pluralized)
+    };
+
+    let mut collection = CollectionBuilder::class_collection(&class.subject, &path, store);
 
     collection.sort_by = match class_subject {
         urls::COMMIT => Some(urls::CREATED_AT.to_string()),
@@ -421,7 +431,7 @@ pub fn create_collection_resource_for_class(
         .ok_or("No self_url present in store, can't populate collections")?;
 
     // Let the Collections collection be the top level item
-    let parent = if class.subject == urls::COLLECTION {
+    let parent = if is_collections_collection {
         drive.to_string()
     } else {
         drive

lib/src/commit.rs

@@ -46,6 +46,8 @@ pub struct CommitOpts {
     pub update_index: bool,
     /// For who the right checks will be perormed. If empty, the signer of the Commit will be used.
     pub validate_for_agent: Option<String>,
+    /// Checks if the URL of the parent is present in its Parent URL.
+    pub validate_subject_url_parent: bool,
 }
 
 /// A Commit is a set of changes to a Resource.
@@ -167,6 +169,20 @@ impl Commit {
             .apply_changes(resource_old.clone(), store, false)
             .map_err(|e| format!("Error applying changes to Resource {}. {}", self.subject, e))?;
 
+        // For new subjects, make sure that the parent of the resource is part of the URL of the new subject.
+        if is_new && opts.validate_subject_url_parent {
+            if let Ok(parent) = resource_new.get(urls::PARENT) {
+                let parent_str = parent.to_string();
+                if !self.subject.starts_with(&parent_str) {
+                    return Err(format!(
+                        "The parent '{}' is not part of the URL of the new subject '{}'.",
+                        parent_str, self.subject
+                    )
+                    .into());
+                }
+            }
+        }
+
         if opts.validate_rights {
             let validate_for = opts.validate_for_agent.as_ref().unwrap_or(&self.signer);
             if is_new {
@@ -381,6 +397,7 @@ impl Commit {
             validate_signature: false,
             validate_timestamp: false,
             validate_rights: false,
+            validate_subject_url_parent: false,
             validate_previous_commit: false,
             validate_for_agent: None,
             update_index: false,
@@ -699,6 +716,7 @@ mod test {
             validate_previous_commit: true,
             validate_rights: false,
             validate_for_agent: None,
+            validate_subject_url_parent: true,
             update_index: true,
         };
     }

lib/src/db/test.rs

@@ -192,7 +192,7 @@ fn destroy_resource_and_check_collection_and_commits() {
 fn get_extended_resource_pagination() {
     let store = Db::init_temp("get_extended_resource_pagination").unwrap();
     let subject = format!(
-        "{}commits?current_page=2&page_size=99999",
+        "{}collections/commits?current_page=2&page_size=99999",
         store.get_server_url()
     );
     let for_agent = &ForAgent::Public;
@@ -212,7 +212,7 @@ fn get_extended_resource_pagination() {
         .unwrap()
         .to_int()
         .unwrap();
-    assert_eq!(cur_page, 2);
+    assert_eq!(cur_page, num);
     assert_eq!(resource.get_subject(), &subject_with_page_size);
 }
 

lib/src/parse.rs

@@ -347,7 +347,8 @@ fn parse_json_ad_map_to_resource(
                     validate_timestamp: false,
                     validate_rights: parse_opts.for_agent != ForAgent::Sudo,
                     validate_previous_commit: false,
-                    validate_for_agent: Some(parse_opts.for_agent.to_string()),
+                    validate_for_agent: parse_opts.for_agent.clone(),
+                    validate_subject_url_parent: true,
                     update_index: true,
                 };
 
@@ -573,8 +574,8 @@ mod test {
             .import(include_str!("../test_files/local_id.json"), &parse_opts)
             .unwrap();
 
-        let reference_subject = generate_id_from_local_id(&importer, "reference");
-        let my_subject = generate_id_from_local_id(&importer, "my-local-id");
+        let reference_subject = generate_id_from_local_id(&importer, "parent");
+        let my_subject = generate_id_from_local_id(&importer, "parent/my-local-id");
         let found = store.get_resource(&my_subject).unwrap();
         let found_ref = store.get_resource(&reference_subject).unwrap();
 

lib/src/resources.rs

@@ -347,6 +347,7 @@ impl Resource {
             validate_for_agent: agent.subject.into(),
             // TODO: auto-merge should work before we enable this https://github.com/atomicdata-dev/atomic-server/issues/412
             validate_previous_commit: false,
+            validate_subject_url_parent: true,
             update_index: true,
         };
         let commit_response = commit.apply_opts(store, &opts)?;
@@ -375,6 +376,8 @@ impl Resource {
             validate_for_agent: agent.subject.into(),
             // https://github.com/atomicdata-dev/atomic-server/issues/412
             validate_previous_commit: false,
+            // This is contentious: https://github.com/atomicdata-dev/atomic-data-rust/issues/556
+            validate_subject_url_parent: false,
             update_index: true,
         };
         let commit_response = commit.apply_opts(store, &opts)?;
@@ -683,6 +686,7 @@ mod test {
                     validate_signature: true,
                     validate_timestamp: true,
                     validate_rights: false,
+                    validate_subject_url_parent: true,
                     validate_previous_commit: true,
                     validate_for_agent: None,
                     update_index: true,

lib/test_files/local_id.json

@@ -1,17 +1,17 @@
 [
   {
-    "https://atomicdata.dev/properties/localId": "reference",
+    "https://atomicdata.dev/properties/localId": "parent",
     "https://atomicdata.dev/properties/write": [
-      "my-local-id"
+      "parent/my-local-id"
     ],
     "https://atomicdata.dev/properties/name": "My referenced resource"
   },
   {
-    "https://atomicdata.dev/properties/localId": "my-local-id",
+    "https://atomicdata.dev/properties/localId": "parent/my-local-id",
     "https://atomicdata.dev/properties/name": "My resource that refers",
-    "https://atomicdata.dev/properties/parent": "reference",
+    "https://atomicdata.dev/properties/parent": "parent",
     "https://atomicdata.dev/properties/write": [
-      "reference"
+      "parent"
     ]
   }
 ]

server/src/handlers/commit.rs

@@ -13,7 +13,7 @@ pub async fn post_commit(
     let mut builder = HttpResponse::Ok();
     let incoming_commit_resource = parse_json_ad_commit_resource(&body, store)?;
     let incoming_commit = Commit::from_resource(incoming_commit_resource)?;
-if store.is_external_subject(&incoming_commit.subject)? {
+    if store.is_external_subject(&incoming_commit.subject)? {
         return Err("Subject of commit is external, and should be sent to its origin domain. This store can not own this resource. See https://github.com/atomicdata-dev/atomic-data-rust/issues/509".into());
     }
     let opts = CommitOpts {
@@ -24,6 +24,7 @@ if store.is_external_subject(&incoming_commit.subject)? {
         // https://github.com/atomicdata-dev/atomic-server/issues/412
         validate_previous_commit: false,
         validate_for_agent: Some(incoming_commit.signer.to_string()),
+        validate_subject_url_parent: true,
         update_index: true,
     };
     let commit_response = incoming_commit.apply_opts(store, &opts)?;

server/src/tests.rs

@@ -79,8 +79,8 @@ async fn server_tests() {
     assert!(body.as_str().contains("html"));
 
     // Should 200 (public)
-    let req =
-        test::TestRequest::with_uri("/properties").insert_header(("Accept", "application/ad+json"));
+    let req = test::TestRequest::with_uri("/collections/properties")
+        .insert_header(("Accept", "application/ad+json"));
     let resp = test::call_service(&app, req.to_request()).await;
     assert_eq!(
         resp.status().as_u16(),
@@ -109,8 +109,8 @@ async fn server_tests() {
     drive.save(store).unwrap();
 
     // Should 401 (Unauthorized)
-    let req =
-        test::TestRequest::with_uri("/properties").insert_header(("Accept", "application/ad+json"));
+    let req = test::TestRequest::with_uri("/collections/properties")
+        .insert_header(("Accept", "application/ad+json"));
     let resp = test::call_service(&app, req.to_request()).await;
     assert_eq!(
         resp.status().as_u16(),
@@ -119,7 +119,7 @@ async fn server_tests() {
     );
 
     // Get JSON-AD
-    let req = build_request_authenticated("/properties", &appstate);
+    let req = build_request_authenticated("/collections/properties", &appstate);
     let resp = test::call_service(&app, req.to_request()).await;
     let body = get_body(resp);
     println!("DEBUG: {:?}", body);
@@ -130,7 +130,7 @@ async fn server_tests() {
     );
 
     // Get JSON-LD
-    let req = build_request_authenticated("/properties", &appstate)
+    let req = build_request_authenticated("/collections/properties", &appstate)
         .insert_header(("Accept", "application/ld+json"));
     let resp = test::call_service(&app, req.to_request()).await;
     assert!(resp.status().is_success(), "setup not returning JSON-LD");
@@ -141,7 +141,7 @@ async fn server_tests() {
     );
 
     // Get turtle
-    let req = build_request_authenticated("/properties", &appstate)
+    let req = build_request_authenticated("/collections/properties", &appstate)
         .insert_header(("Accept", "text/turtle"));
     let resp = test::call_service(&app, req.to_request()).await;
     assert!(resp.status().is_success());