Open panaaj opened 5 years ago
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.
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
?
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?
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.
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.
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.
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:
group
in SK spec at the momentEven 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.
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.
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"
}
}
},
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.
Seems we need a 'published' flag. Should that be status
, with ENUM draft,published
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.
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.
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.
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.
..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.
Unless I am reading it wrong, it appears that there are no further changes required.
Some clarifications, I hope:
meta._attr
is in the json schema, yes, but it is pretty underspecified. It is not documented in any way in the textual specification.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.
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:
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.
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.
properties
does not specify any structure, which makes it pretty useless for validation. Any document with anything inproperties
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.
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)
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": {
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.
I have completed the following:
gitbooks/resources_notes.md
as documentation for notes.test/data/full-valid/reources-note-sample.json
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?
@tkurki @panaaj provided some more changes, does it resolve your review?
This PR looks to address Notes schema limitations outlined in Issue #539 (@irandrews)
The current Notes schema facilitates the following:
/resources/regions
Although
position
,region
andgeohash
are not defined asoneOf
they are described as alternatives to each other. This implies that one can not define a Note at a specific location within aregion
.Therefore the scenario where the area of a region is:
is not specifically supported with the current definitions.
Additionally there is no way to:
To facilitate these scenarios the following changes are proposed:
Change the wording of the definitions of:
region
to be defined as additional toposition
orgeohash
position
andgeohash
to be defined as only being alternatives to each otherAdd a
group
attribute to allow the definition of a group or collection to which the Note belongsAdd an
author
attributeAdd a
readOnly
attribute to show whether the content of the Note is permitted to be changed.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 #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.