Feature: Additional Note attributes

[panaaj]

This PR looks to address Notes schema limitations outlined in Issue #539 (@irandrews)

The current Notes schema facilitates the following:

• Create a Note at a geographic location
• Create a Note at a geohash
• Attach Note to a Region uuid in /resources/regions

Although position, region and geohash are not defined as oneOf they are described as alternatives to each other. This implies that one can not define a Note at a specific location within a region.

Therefore the scenario where the area of a region is:

1. Large enough to contain multiple hazards, at various locations within the region AND
2. One wants to place different Notes on specific hazards

is not specifically supported with the current definitions.

Additionally there is no way to:

• Group Notes together regardless of position or region.
• Define the author of a Note.
• Define whether the contents of the Note can be edited / changed.
• Define whether the note is available only on the host Signal K server or is shared with other servers / services.

To facilitate these scenarios the following changes are proposed:

1. Change the wording of the definitions of:

• region to be defined as additional to position or geohash
• position and geohash to be defined as only being alternatives to each other

2. Add a group attribute to allow the definition of a group or collection to which the Note belongs

3. Add an author attribute

4. Add a readOnly attribute to show whether the content of the Note is permitted to be changed.

5. Add a published attribute, to show whether a Note is available to other Signal K servers or services.

---

Adding these additional attributes would:

• Facilitate the retrieval of notes that are part of a specific group by supplying the relevant parameter to /resources/notes?group=<group_name> (as proposed in PR feature: Add Resources section #541).

• Identify the author of a Note.

• Identify a Note as non-editable.

• Identify a Note as available outside of the host Signal K server.

[irandrews]

I am very keen for this PR to be adopted, as its absence is currently holding up my project. I can live with all the changes in the current PR. There are two open suggestions above which I’ve commented on.

[rob42]

So two remaining questions:

1. validity, using far future dates, does this suffice for draft/published/deleted ?
2. Any objections to replacing accessType with use of meta._attr?

[panaaj]

With regards to meta._attr I have looked through the spec and I can't seem to find a reference to it.
the data_model_metadata.md document talks about the metadata attributes and setting them up globally but there is no mention of permissions meta data.
In node-server ./<some path>/meta._attr is not recognised ( not sure about Artemis) but it begs the question how do we deal with an immediate use case in the short term?

[tkurki]

validity, using far future dates, does this suffice for draft/published/deleted

Not to me. draft status related to publishing is semantically different from validity. Why would we want to overload validity with this special meaning? For example if I want to show notes in draft status differently in the UI or provide a list of notes in draft status we would need to define a magic value to represent drafts.

[tkurki]

Something to consider: are some of the use cases and additions here specific to this @irandrews 's use case or are they generic? Not everything has to be in the specification from day zero, we can write software that extends Signal K, figure out the details with field use, maybe redo some of the stuff and then expand the specification. Naturally there is no specification for everybody to refer to. But often additions created from a purely theoretical standpoint don't stand the test of real use, or need to be extended anyway to be usable.

[panaaj]

For "use case" specific attributes is it maybe worth considering a mechanism to provide extended attributes in a similar way to gpx (they define ext to hold application / use case specific attributes) or GeoJSON (they have the properties object)?

Since GeoJSON is used in a number of resources it's kind of already in place.

[tkurki]

Having a generic, non note specific method to represent access is something to consider, but we don't have one at this stage.

I disagree with adopting the Unix access model wholesale. Some of the features are not relevant without further specification:

• what does execute mean?
• there is no concept of group in SK spec at the moment
• what's a directory in SK data model? can you retrieve the access information for a key that you don't have any access to (no read/write)? does write access mean that you can remove or just empty a path?
• do we want to implement special cases like having write but no read access?

Even if we'd adopt the model as a whole we would still have to define the format to present this data in the API. I don't think encoding this in numbers or strings the way Unix-like utilities intended for human use makes sense, as programmatic access would require an extra step to interpret what the value means.

Again even if we'd adopt the model in the future as a whole there is nothing stopping us from providing an application developer friendly, backwards compatible way to this information: what if we add meta.accessType enum with values READ and WRITE? This would allow an application to be informed about the access type the current user has for this resource. Even if we expand the access model in the future we could keep this API with very little overhead.

[tkurki]

I was about to add something about generic key-value pairs for use case specific values, but Adrian beat me to it. I think it would be a good idea. Already discussed in #539. Call them properties to be in line with GeoJSON.

[rob42]

Definition for _attr is in https://github.com/SignalK/specification/blob/master/schemas/definitions.json

"_attr": {
      "type": "object",
      "title": "_attr schema.",
      "description": "Filesystem specific data, e.g. security, possibly more later.",
      "properties": {
        "_mode": {
          "type": "integer",
          "title": "_mode schema.",
          "description": "Unix style permissions, often written in `owner:group:other` form, `-rw-r--r--`",
          "default": 644
        },
        "_owner": {
          "type": "string",
          "title": "_owner schema.",
          "description": "The owner of this resource.",
          "default": "self"
        },
        "_group": {
          "type": "string",
          "title": "_group schema.",
          "description": "The group owning this resource.",
          "default": "self"
        }
      }
    },

[rob42]

As Ive said before unix style permissions arent perfect but they have stood the test of time and are already in the spec. As always it can be implemented by a server any way that suits, this is just a rendition of the permissions for sharing.

AFAIK key level security is not implemented in node server, but it is in the artemis server. I learned a lot about the issues and problems involved, and parsing them at high speed. Simply put, it works for our needs. In application it works exactly the same as a Linux filesystem in what you can see and do. The use of groups is also an implementation issue. It just provides a way to identify a collection of users which is more than owner, and less than all.

BTW its your.key.meta._attr, so would be ...your/key/meta/_attr in a GET.

[rob42]

Seems we need a 'published' flag. Should that be status, with ENUM draft,published

[panaaj]

I have made the following changes.....
Replaced draft and readOnly attributes with a properties object to hold additional key/value pairs required by specific use cases.

So now draft and readOnly (if the use case requires them) can be placed in the properties object.

[irandrews]

I think the permissions issue is so important it deserves more thought and its own PR. owner/group/world may have a role to play in the context of, say, SignalK being used to crowdshare weather or navigation data, (with other boats or chart/met suppliers) but I don't think a discussion about notes is the right place to make the decision.
For this PR, I think the properties block is a good solution.

[rob42]

published seems to have value, lets add it.

Security was argued over endlessly in the past, the _attr is already in the spec. It doesnt need revisiting. Security and access to paths is critical to a viable server (at scale) but you cannot control the data once it leaves your servers. Look at the whole DRM mess, it just doesnt work. Remember the implementation of security is internal to the server, you can do it anyway you want.

So the only need for _attr is to provide a generic method to signal permissions beyond your servers, or between them, or for users to set key permissions in a generic way (from an app). The UNIX perms model is well understood, and works, we dont need to re-invent a custom version.

readOnly has no practical use. Its equivalent to a 10yr old putting a "No burglars" sign on his bedroom door.

Before you flame me yet again consider how the readOnly flag will work. Most of you use the node server so AFAIK it only has web based global login. No key level security, aka no real security beyond a simple web app model at all.

I login to your server, load the note, see the readOnly flag and laugh, edit it anyway and PUT it back. What happens? Do you quickly add a bit of code for notes that checks readOnly and refuses to write? Excellent, the author can no longer write either. Another quick fix...cool a custom one off security for one aspect of notes. Now, about charts, autopilot, position...or if I inject a PUT message over ws or tcp..

Try this using the artemis server and the author (owner) sets -rw-r--r-- your PUT will fail because you dont have write permission. It wont work on charts or autopilot either. The same generic security works across all transports for all keys. Its there now - try implementing it in node and you will understand. The servers implementation is not important, just that it applies the permissions requested by the author.

If you really cant see past readOnly use it in your own implementation, but dont add it to the spec and expect others to implement it.

[irandrews]

I don't mind how its done, but the purpose of the read/write flag on a note is to tell the client whether to put an 'edit' button on the screen next to the note.
I thought we'd accepted that a 'properties' block will be the way to do this, so its out of the spec.

[rob42]

..one use for the meta._attr on a note is to tell the client whether to put an 'edit' button on the screen next to the note.

[panaaj]

Unless I am reading it wrong, it appears that there are no further changes required.

[tkurki]

Some clarifications, I hope:

• Node server has per key security / access control lists that operate on Signal K paths, see https://github.com/SignalK/signalk-server-node/blob/master/test/security.js#L65-L93
• meta._attr is in the json schema, yes, but it is pretty underspecified. It is not documented in any way in the textual specification.
• Node server does not implement meta._attr

I think the best course of action here is that for the use cases at hand we adopt meta._attr as outlined by Rob above.

Personally I am not too crazy about using unix command line numeric representation in an API that is supposed to be self documenting and obvious to the most casual observer, but it is one commonly understood way to do this. As Rob says it is just way to communicate the permissions - we have no Signal K API to modify the permissions.

Further down the line we should clear the ambiguity related to meta._attr with additional documentation and maybe add more user friendly APIs. Unix style permissions, often written in owner:group:other form, -rw-r--r-- is too vague for my taste. Like, if you don't know this already where can you learn more? do we support setuid and setgid as well? how do the access levels translate to Signal K actions? is execute level relevant somehow? etc.

That said this PR is mergeable to me.

[tkurki]

Sorry, I was a bit hasty saying that this is in mergeable status. I was only reading the discussion, not the actual schema files.

Three problems:

• No test data - there is nothing that checks if the updated schema works the way we think and no full document to serve as an example on how notes are supposed to be used. A trap we have fallen into several times that I would not like to repeat. I suggest crafting one or more samples documents in test/data/full-valid/, they will be automatically validated in the tests.
• properties does not specify any structure, which makes it pretty useless for validation. Any document with anything in properties will fail validation. Unknown property names are doable in JSON schema. I guess all types should be allowed as values.
• groups use is not documented, as it stands it is too vague to be usable. Earlier in this discussion I suggested that it should be handled like regions, with notes using id pointers, then the real group could have a name and a description

@panaaj sorry about change request this late in the game - mea culpa.

[tkurki]

Now that I wrote groups I realised another problem: what if a note belongs to several groups? It should be an array of group ids I think. Again a nice sample document would go a long way to clearing things up here.

[panaaj]

• properties does not specify any structure, which makes it pretty useless for validation. Any document with anything in properties will fail validation. Unknown property names are doable in JSON schema. I guess all types should be allowed as values.

I found this regarding the JSON schema and object properties:

The additionalProperties keyword is used to control the handling of extra stuff, that is, properties whose names are
not listed in the properties keyword. By default any additional properties are allowed.
The additionalProperties keyword may be either a boolean or an object. If additionalProperties is a boolean
and set to false, no additional properties will be allowed.

So it seems to be valid but I could add "additionalProperties": true for clarity.

[panaaj]

Now that I wrote groups I realised another problem: what if a note belongs to several groups? It should be an array of group ids I think. Again a nice sample document would go a long way to clearing things up here.

The way I view group is it represents the name of a collection of notes, and (while a note could technically be associated with more than one group) they generally attached to an item / object.
A bit like a sticky note...... you write on it, you tear it off and then you stick it to something.

I think it notes with multiple groups might be over-complicating it a little but happy to be wrong!

I'm happy to draft something to show how notes can be used which will possibly clarify things.. (btw that is the motivation behind #541)

[panaaj]

Also thinking if a note needs to be related to a position, area or region should the schema include a anyOf [ {required: [..]} ] for these properties?

                "^urn:mrn:signalk:uuid:[0-9A-Fa-f]{8}-[0-9A-Fa-f]{4}-4[0-9A-Fa-f]{3}-[89ABab][0-9A-Fa-f]{3}-[0-9A-Fa-f]{12}$": {
                    "type": "object",
                    "description": "A note about a region, named with a UUID. Notes might include navigation or cruising info, images, or anything",
                    "anyOf": [
                        {
                            "required": ["region"]
                        },
                        {
                            "required": ["position"]
                        },
                        {
                            "required": ["geohash"]
                        }                                                
                    ],
                    "properties": {

[tkurki]

This excludes notes that are not connected to geography. Why would we want to have everything tied to geography?

[panaaj]

So then it is OK for a note not to be connected to anything (position, geography, group, another resource, signalk path), basically just a title and / or description.

I have no problem with this.... it's just that notes generally relate to something.

[panaaj]

I have completed the following:

1. created gitbooks/resources_notes.md as documentation for notes.
2. created test/data/full-valid/reources-note-sample.json
3. updated schemas/groups/resources.json

Is there anything else required to finalize this PR to be merged?
Does a link to the gitbooks/resources_notes.md file need to be created or will this be done by the publishing process?

[fabdrol]

@tkurki @panaaj provided some more changes, does it resolve your review?