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

Add a security section to APIs that require user activation #20435

Merged
merged 3 commits into from
Sep 22, 2022

Conversation

Elchi3
Copy link
Member

@Elchi3 Elchi3 commented Sep 8, 2022

Description

Follow-up to #20358. It was proposed to add a note to all APIs that require user activation. I was considering to add a notecard banner to these pages but many pages already use too many banners and I don't want to add to that callout business on the pages [1]. So, I thought it might be wise to introduce a section called "Security" where we could mention all security related aspects of APIs. I did that in this PR. Let me know if that's okay.

[1] https://twitter.com/ddbeck/status/1509925868021420042

Motivation

I was motivated by this comment #20358 (comment).

Related issues and pull requests

This also contributes to the request made at openwebdocs/project#73.

@Elchi3 Elchi3 requested a review from a team as a code owner September 8, 2022 15:54
@Elchi3 Elchi3 requested review from wbamberg and removed request for a team September 8, 2022 15:54
@github-actions github-actions bot added the Content:WebAPI Web API docs label Sep 8, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2022

Preview URLs

Flaws

Note! 22 documents with no flaws that don't need to be listed. 🎉

URL: /en-US/docs/Web/API/PaymentRequest/show
Title: PaymentRequest.show()
Flaw count: 5

  • macros:
    • /en-US/docs/Web/API/PaymentDetailsModifier does not exist
    • /en-US/docs/Web/API/AddressErrors does not exist
    • /en-US/docs/Web/API/PaymentShippingOption does not exist
    • /en-US/docs/Web/API/PaymentRequest/retry does not exist
    • /en-US/docs/Web/API/PaymentRequest/complete does not exist

URL: /en-US/docs/Web/API/MediaDevices/getDisplayMedia
Title: MediaDevices.getDisplayMedia()
Flaw count: 2

  • broken_links:
    • Can't resolve /en-US/docs/Web/API/Media_Streams_API/Constraints
    • Can't resolve /en-US/docs/Web/API/Media_Streams_API

(this comment was updated 2022-09-21 12:23:05.461808)

@teoli2003
Copy link
Contributor

teoli2003 commented Sep 8, 2022

This looks good! I have two comments.

  • Should we update the template? I think so, and mentioning the two types of user activation (there will likely be others, like the ones used for the Web Audio API)
  • Also, is there a way to know about this constraint from the WebIDL or w3c/webref? (I'm trying to think how we could not forget one case in the future)

@wbamberg
Copy link
Collaborator

wbamberg commented Sep 8, 2022

What do you think about using a macro/two macros for this? Yes I know, macros, but:

  • it's boilerplate text. Why not define it in one place?
  • macros are easier to find and replace if we ever manage to move to a more sensible system

Presumably the future here is that "user activation required" and "permissions" are bit of metadata associated with the API, that the page builder reads and inserts whatever note stuff we think is best to convey that.

I was considering to add a notecard banner to these pages but many pages already use too many banners and I don't want to add to that callout business on the pages [1].

I mean I'm with you on this, totally, but that's just a choice about how to style it and where to put it, not about how we represent it internaly.

Edited to add: or, why not use metadata now, and do it right from the start?

@Elchi3
Copy link
Member Author

Elchi3 commented Sep 8, 2022

Presumably the future here is that "user activation required" and "permissions" are bit of metadata associated with the API, that the page builder reads and inserts whatever note stuff we think is best to convey that.

Yes, I thought the same. I think the amount of APIs that use this metadata is growing but is it still low overall.
I think security related metadata is:

  1. secure context (https) required?
  2. permission required?
  3. user activation required?

I think we use a macro for 1. and nothing for 2. and 3. because macros seem old school and deprecated. I'm happy to use a macro if it is okay to create a new one. (is it?)

Edited to add: or, why not use metadata now, and do it right from the start?

Does "use metadata now" mean to add a front-matter key, or something else? What is our metadata strategy? (it feels this PR is stumbling into some larger questions, which are worth asking, but it also feels like I just entered a rabbit hole.)

@teoli2003
Copy link
Contributor

@wbamberg Where do you think we should store this metadata? YAML, mdn/data, mdn/browser-compat-data ?

@wbamberg
Copy link
Collaborator

wbamberg commented Sep 8, 2022

@wbamberg Where do you think we should store this metadata? YAML, mdn/data, mdn/browser-compat-data ?

I think we should store it in YAML front matter (if we can't fetch it out of webref). But I think we would need to have a think about what all the front matter we would expect to need is, and how to represent it (e.g. how do you represent things like "needs-user-activation" which is like a flag).

@wbamberg
Copy link
Collaborator

wbamberg commented Sep 8, 2022

Does "use metadata now" mean to add a front-matter key, or something else? What is our metadata strategy? (it feels this PR is stumbling into some larger questions, which are worth asking, but it also feels like I just entered a rabbit hole.)

Sorry.

I think it is easier to get from: "macro" to "data" than to get from "prose" to "data". So using a macro now seems like a way to be able to implement this feature today without having to ask these difficult questions today, and without having to deal with so much soup if we ever do want to tackle these questions. But since I don't have good answers about metadata, I'm also happy for this to get merged as it is.

I think security related metadata is:

  1. secure context (https) required?

  2. permission required?

  3. user activation required?

I think we use a macro for 1. and nothing for 2. and 3.

I wonder if we could extend https://github.com/mdn/yari/blob/main/kumascript/macros/secureContext_header.ejs with some arguments, and use it for the other things. Then at least there wouldn't be a new banner/macro.

@hamishwillee
Copy link
Collaborator

hamishwillee commented Sep 8, 2022

I wonder if we could extend https://github.com/mdn/yari/blob/main/kumascript/macros/secureContext_header.ejs with some arguments, and use it for the other things. Then at least there wouldn't be a new banner/macro.

FWIW I'd prefer to go straight to yaml. Make them any requirement default as false if not declared.

security_requirements:
  secure_context: true
  user_permission: true
  user_activation: true

I would render them in the securecontext macro, potentially later do based on page type.

Why? My guess is that this is just as easy as working out how to get the macro to take various optional arguments, and should be fairly easy to extend. You could even render it more or less based on the list: "Following security requirements apply:".

P.S. This does not necessarily preclude the macro being required to be at a particular spot such as a "Security" section, in the same way that we have "Compatibility" etc. But that's a separate discussion.

@wbamberg
Copy link
Collaborator

wbamberg commented Sep 9, 2022

I wonder if we could extend https://github.com/mdn/yari/blob/main/kumascript/macros/secureContext_header.ejs with some arguments, and use it for the other things. Then at least there wouldn't be a new banner/macro.

FWIW I'd prefer to go straight to yaml. Make them any requirement default as false if not declared.

security_requirements:
  secure_context: true
  user_permission: true
  user_activation: true

Small points, but we use dashes, not underscores, and if they are just flags it would be cleaner just to say: if they are present the requirement applies:

security-requirements:
  - secure-context
  - user-activation
  - user-permission

But also I think permissions are names, not just booleans, so maybe, er:

security-requirements:
  flags:
    - secure-context
    - user-activation
  user-permission: clipboard-read

I would render them in the securecontext macro, potentially later do based on page type.

Yeah, this ties into page types inasmuch as it is only valid for certain page types (not to CSS properties, for instance).

Why? My guess is that this is just as easy as working out how to get the macro to take various optional arguments, and should be fairly easy to extend. You could even render it more or less based on the list: "Following security requirements apply:".

P.S. This does not necessarily preclude the macro being required to be at a particular spot such as a "Security" section, in the same way that we have "Compatibility" etc. But that's a separate discussion.

Yes, agreed. Just like with BCD, unless you want a banner at the top of the page you need a macro to indicate the spot (until we can build pages from templates).

One thing: in BCD we have the "## Browser compatibility" heading in content, and the macro/yari only supply the table. That works OK because just about all pages have BCD, but not so many have something to say about these requirements.

So in this case do we also have "## Security requirements" in content, and have the macro just say "None." in all the cases where there is nothing to say? Seems ugly. Or does the macro render the heading as well if it has something to say, and not otherwise?

And do we have the macro in all pages, which seems weird when it almost always does nothing, or do we only have it when there is something to say, which means authors have to update the page in 2 places (the data and the macro call).

@hamishwillee
Copy link
Collaborator

Yeah, why not:

security-requirements:
  flags:
    - secure-context
    - user-activation
  user-permission: 
    - clipboard-read
    - whatever-thingy

So in this case do we also have "## Security requirements" in content, and have the macro just say "None." in all the cases where there is nothing to say? Seems ugly. Or does the macro render the heading as well if it has something to say, and not otherwise?

I would say we only have the section if it is needed. We could choose to just leave it up the top of the page or we could say "must live above the compatibility, if it is defined".
But this is a distraction. If we can get the metadata and macro agreed, the rest will be relatively easy.

@Elchi3
Copy link
Member Author

Elchi3 commented Sep 9, 2022

(I think good data modeling takes some time and thoughts. I'm unsure if it is a good idea to add-on to this PR to do it, but here we are.)

So, user activation can't be a flag either, I think. Most features are gated by "transient" user activation, but there is also "sticky" user activation for a few. Also, I'm not sure I'd introduce a grouping called "flags". It seems like unnecessary nesting to me.

I'd propose it like so:

security-requirements:
 - secure-context: true
 - user-activation: transient
 - permissions: 
   - clipboard-read

So, how do we proceed here?

  1. Keep the prose changes to add a "Security" section to the pages
  2. Add YAML (if we agree on how it looks like exactly).
  3. Merge this PR
  4. Do a follow up project to do something sensible with the new YAML.

Yes?

@teoli2003
Copy link
Contributor

I like the direction this is taking!

So, user activation can't be a flag either, I think. Most features are gated by "transient" user activation, but there is also "sticky" user activation for a few.

Also we can discover more cases in the future (peeking at Web Audio API where contexts are not usable unless there has been some user interaction, and I don't know if it fits the two cases).

Maybe we can even do the following:

security-requirements:
 - secure-context
 - user-activation: transient
 - permissions: 
   - clipboard-read

(Avoiding the true)

And I'm 👍 for a security section above spec or compat sections, when needed.

@wbamberg
Copy link
Collaborator

wbamberg commented Sep 9, 2022

(I think good data modeling takes some time and thoughts. I'm unsure if it is a good idea to add-on to this PR to do it, but here we are.)

Yes, it does, and I'm also unsure about this, which is why I suggested using a macro as a stepping stone.

So, how do we proceed here?

1. Keep the prose changes to add a "Security" section to the pages

2. Add YAML (if we agree on how it looks like exactly).

3. Merge this PR

4. Do a follow up project to do something sensible with the new YAML.

Yes?

I guess the thing is, once we've merged this PR, it's not so easy to find the places that had this prose added so we can update them. Because it's just prose. If it's a macro, that's a lot easier.

@teoli2003
Copy link
Contributor

teoli2003 commented Sep 10, 2022

If we add YAML for security data, we should use it and, therefore, add a macro (like {{Compat}} and {{Specifications}}). Once we are advanced enough with templates and page types, we probably won't need it anymore, but I agree that a new macro here would:

  • Allow us to find & replace easily once the templating system is in place.
  • Allow easy maintenance as scattered prose is a nightmare to maintain.
  • Don't add more complexity than what we have with {{Compat}} and {{Specifications}}.
  • Is a good stepping-stone towards our utopia.

@Elchi3
Copy link
Member Author

Elchi3 commented Sep 22, 2022

I guess the thing is, once we've merged this PR, it's not so easy to find the places that had this prose added so we can update them.

User activation gated features are now listed here: https://developer.mozilla.org/en-US/docs/Web/Security/User_activation

I looked into developing this PR further, like adding a macro and adding more front-matter, but it feels really half baked. It seems that macros+front-matter should then also be added to all pages that talk about the HTTPS requirement or about needed permissions. I'm not ready to go down this route in this PR. I think it would be fine to accept the prose changes this PR does, but if it isn't, I'm leaning towards closing here and make a plan somewhere to implement the Ideas that came up here properly.

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 all right, I'll not stop this useful change from landing :).

@wbamberg wbamberg merged commit a243190 into mdn:main Sep 22, 2022
@Elchi3 Elchi3 deleted the user-activation-apis branch September 22, 2022 16:44
himanshugarg pushed a commit to himanshugarg/content that referenced this pull request Sep 27, 2022
* Add a security section to APIs that require user activation

* Fix markdown lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants