Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Combination security scheme #944

Merged
merged 16 commits into from Sep 30, 2020
Merged

Combination security scheme #944

merged 16 commits into from Sep 30, 2020

Conversation

mmccool
Copy link
Contributor

@mmccool mmccool commented Aug 11, 2020

Addresses: #901

A security scheme that allows other security scheme definitions to be combined using either "any" or "all" combinations. The "anyOf" and "allOf" elements are meant to be similar to the corresponding data schema definitions (which is why I used these terms instead of the "and" and "or" terms discussed in the linked issue).

Things to discuss:

  1. Maybe we should use "oneOf" instead of "anyOf"? (edit: decided "oneOf" is better)
  2. We could alternatively define separate schemes for "anyOf" and "allOf", but then some other keyword is needed to list the arguments. (edit: can have one scheme, but if made default, can get a simple syntax in the end)
  3. Ontology files and JSON Schema not yet updated, we should discuss first; once we have reached consensus on the design, these details can be filled in. (edit: now done)
  4. The restriction that only one of "anyOf" or "allOf" must be used is not easy to describe in the table. Basically exactly one of these is mandatory. (edit: restriction added to JSON schema, but not sure how to describe in ontology or shapes; marked with FIXME, needs to be discussed)

Preview | Diff

@mmccool mmccool linked an issue Aug 11, 2020 that may be closed by this pull request
@mmccool mmccool marked this pull request as draft August 11, 2020 20:08
@mmccool mmccool changed the title Combination security scheme WIP: Combination security scheme Aug 11, 2020
@farshidtz
Copy link
Member

I vote for "oneOf" (exactly one) instead of "anyOf" (one or more). A consumer program which can work with more than one, may choose to do so and override inputs (e.g. header) and possibly cause conflicts.

@farshidtz
Copy link
Member

As mentioned here, having the combination scheme, I suggest deprecating the Array type for security or at least making it NOT RECOMMENDED.

Having different ways to define AND combination (or anything for that matter) reduces usability and makes it harder for applications to adopt Thing Descriptions.

@mmccool
Copy link
Contributor Author

mmccool commented Aug 17, 2020

The text really needs an example. (edit: there are two now) Farshid (@farshidtz) has one here, which I extract below:

        "combination": {
            "scheme": "combination",
            "oneOf": [
                "oauth2_code",
                "oauth2_client",
                "oauth2_device"
            ]
        }

Note this uses the suggested "oneOf" syntax (which I agree with and intend to change).

As an additional idea, if we allow the "inline security definitions" to also be used for "oneOf" and "allOf", then we could dispense with the names and securityDefinition and do things like:

       "security": {
            "scheme": "combination",
            "oneOf": [
                 {
                    "scheme": "oauth2",
                    "flow": "code",
                    ...
                 },
                 {
                     "scheme": "oauth2",
                     "flow": "client",
                     ...
                 },
                 {
                     "scheme": "oauth2",
                     "flow": "device",
                     ...
                 }
            ]
        }

@mmccool
Copy link
Contributor Author

mmccool commented Aug 17, 2020

So I had another thought about how to make this more compact, related to a point raised in the Security TF meeting today about "why don't we just have oneOf/allOf by themselves". I realized this IS possible if we put them at the same level as the "scheme" element and get rid of the special pseudo-scheme "combination". Basically, in a SecurityScheme object you would use one of "scheme", "oneOf", or "allOf". This essentially inlines and combines the data schema for the "combination" pseudo-scheme into the SecurityScheme object itself (although it further complicates the variant record issue...).

Anyhow, here are the examples above rewritten to use that approach:

        {
            "oneOf": [
                "oauth2_code",
                "oauth2_client",
                "oauth2_device"
            ]
        }

And if we allow the "inline security definitions":

       "security": {
            "oneOf": [
                 {
                    "scheme": "oauth2",
                    "flow": "code",
                    ...
                 },
                 {
                     "scheme": "oauth2",
                     "flow": "client",
                     ...
                 },
                 {
                     "scheme": "oauth2",
                     "flow": "device",
                     ...
                 }
            ]
        }

In short, nice and compact, but I think we'll have to check whether we can write a JSON Schema that can validate it. (edit: confirmed that JSON schema is possible, but need two variants of the combination schema, one for "oneOf" and another for "allOf")

@mmccool
Copy link
Contributor Author

mmccool commented Aug 17, 2020

Farshid (@farshidtz) also proposed deprecating the current syntax where an array in "security" means an "allOf" combination. There are two reasons to do this:

  1. it is redundant with the new "allOf" syntax
  2. it conflicts with the usage in OpenAPI, which uses "OR" (oneOf) semantics.

To my mind 2 is the bigger problem, since it is likely to confuse people with an OpenAPI background. Note that "deprecation" means in the 1.1 spec we would say "NOT RECOMMENDED" and would only remove it in TD 2.0 (since it would break backward compatibility).

However, with the compact proposed syntax, there is a simple equivalence:

     "security": ["a", "b", "c"]

would be exactly equivalent to

     "security": { "allOf": ["a", "b", "c"] }

The second form is slightly more verbose but is clearer.

@mmccool
Copy link
Contributor Author

mmccool commented Aug 17, 2020

Upon reflection, I realized the proposed "compact" syntax w/o "scheme" would cause a compatibility problem in the 1.x version of the spec since "scheme" is mandatory. Omitting it would break TD 1.x processors that expect it. So what I propose doing is introducing this in several steps:

  1. Adopt a "combination" security scheme as in this PR.
  2. In TD 1.1, make this the recommended way to do combinations and mark the array syntax as deprecated (but still legal) (edit: since this is for TD 1.1, it can be added to this PR as well)
  3. In TD 2.0, make "scheme" optional, or more precisely, give it the default value of "combination". Then the above syntax works: if "scheme" is omitted, then you need one of "allOf" or "oneOf" instead.
  4. In TD 2.0, also remove the array syntax for "security"; however, the compact syntax for "allOf" is available instead.

@mmccool
Copy link
Contributor Author

mmccool commented Aug 21, 2020

I made a bunch of improvements following the discussion above:

  • Changed "anyOf" to "oneOf"
  • Marked use of array value for "security" as NOT RECOMMENDED
  • Added an editor's note indicating this should be deprecated and why
  • Updated all examples to remove array notation for "security" (ed note above also recommends that array notation not be used for single value)
  • Added an example for "allOf" and another one for "oneOf." First example for "allOf" shows an alternative to the use of the (deprecated) array value for security; second example shows using "oneOf" as an alternative to using repeated forms.

The PR has also been updated to add appropriate definitions to the ontology and validation files, including to the JSON schema, and to make the comments in the ontology definitions consistent with the edits in index.template.html. Still an issue with defining the "only one of oneOf or allOf" in the shape definitions, however; marked with FIXME.

Also fixed some typos and respec issues.

@mmccool
Copy link
Contributor Author

mmccool commented Aug 21, 2020

Please feel free to re-review! I'll leave it in draft status for now though until we do a group review in the next Security TF call.

@mmccool mmccool changed the title WIP: Combination security scheme Combination security scheme Aug 24, 2020
@mmccool mmccool marked this pull request as ready for review August 24, 2020 12:58
@takuki
Copy link
Contributor

takuki commented Sep 2, 2020

In Sept 2 TD call, it was suggested to add certain constraints such as prohibiting nested CombinationSecurityScheme, for example.

It was also pointed out that we need to ask for some implementation experience first, especially on Consumer side.

In the coming PlugFest, we should ask implementers to experiment with Combination security scheme.

There is a mashup of microservices use case on producer (or proxy) side for Combination security scheme.

Consumer can use a security scheme from "oneOf" from the Combination security scheme.
This expectation should be described in a note in this PR.
(Note currently, there is a limitation in NodeWoT implementation.)

@mmccool mmccool marked this pull request as draft September 23, 2020 15:07
@mmccool
Copy link
Contributor Author

mmccool commented Sep 23, 2020

Marked as draft because I have to add one more assertion to prevent recursive use (to simplify testing).
Also this can't be merged at the moment due to render script updates - rebasing and possibly manual conflict resolution are still needed.

@sebastiankb
Copy link
Contributor

from today's TD call:

@sebastiankb
Copy link
Contributor

from today's TD call:

  • decided to merge this PR
  • however, this new feature should be marked "at risk"
  • there are conflicts, have to be fixed.
  • add an editorial note about a specific use case
  • @mmccool will address those points and will merge this PR

@mmccool mmccool marked this pull request as ready for review September 30, 2020 23:24
@mmccool mmccool merged commit 58f543f into w3c:master Sep 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarifying use of multiple security schemes in the security term
5 participants