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

usersession/userd: add zoommtg url support #8304

Merged

Conversation

anonymouse64
Copy link
Contributor

Originally committed by @troyready in #8254

Original-Author: troyready <troy@troyready.com>
Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
Copy link
Contributor

@zyga zyga left a comment

Choose a reason for hiding this comment

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

+1

@zyga zyga added this to the 2.44 milestone Mar 20, 2020
@codecov-io
Copy link

codecov-io commented Mar 20, 2020

Codecov Report

Merging #8304 into master will increase coverage by 0.00%.
The diff coverage is 81.32%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #8304    +/-   ##
========================================
  Coverage   80.65%   80.65%            
========================================
  Files         717      724     +7     
  Lines       57254    57632   +378     
========================================
+ Hits        46177    46484   +307     
- Misses       7481     7513    +32     
- Partials     3596     3635    +39     
Impacted Files Coverage Δ
bootloader/bootloader.go 68.51% <ø> (ø)
cmd/snap-bootstrap/bootstrap/bootstrap.go 49.38% <0.00%> (-1.26%) ⬇️
cmd/snap-bootstrap/main.go 40.74% <0.00%> (-1.57%) ⬇️
cmd/snap-repair/cmd_run.go 55.81% <0.00%> (ø)
daemon/api_recovery.go 0.00% <0.00%> (ø)
image/helpers.go 67.01% <0.00%> (ø)
interfaces/builtin/greengrass_support.go 100.00% <ø> (ø)
interfaces/builtin/openvswitch_support.go 100.00% <ø> (ø)
interfaces/builtin/udisks2.go 100.00% <ø> (ø)
overlord/patch/patch5.go 6.45% <0.00%> (ø)
... and 86 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4112a7...7f678b9. Read the comment docs.

Signed-off-by: Ian Johnson <ian.johnson@canonical.com>
@mvo5
Copy link
Contributor

mvo5 commented Apr 1, 2020

What's the status here, do we need jamie or can this go in as is?

@anonymouse64
Copy link
Contributor Author

anonymouse64 commented Apr 1, 2020

@mvo5 waiting on @jdstrand since Jamie didn't approve the original one

Copy link

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

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

I asked in #8254 (comment) about the contents of the URL, but didn't get back anything definitive, so I looked and found: https://medium.com/zoom-developer-blog/zoom-url-schemes-748b95fd9205. The contents of the URL scheme are fine and this PR can be applied as is.

That said, should we also include "zoomus"? Based on the above blog, it seems 'no', but I'm not familiar with zoom URL schemes so I'm not sure. That shouldn't block this PR since if it comes up we can add it later.

@jdstrand
Copy link

jdstrand commented Apr 1, 2020

This seemed blocked on me so since I provided my review, I removed the blocked tag.

@anonymouse64
Copy link
Contributor Author

That said, should we also include "zoomus"?

@jdstrand let's say no until someone asks us for it. I haven't seen it in the wild either

@anonymouse64 anonymouse64 merged commit 06acdc4 into canonical:master Apr 1, 2020
@anonymouse64 anonymouse64 deleted the add-zoom-whitelisted-scheme branch April 1, 2020 17:12
anonymouse64 added a commit to anonymouse64/snapd that referenced this pull request Apr 1, 2020
…ted-scheme

usersession/userd: add zoommtg url support

Signed-off-by: Ian Johnson <person.uwsome@gmail.com>
@nermolov
Copy link

nermolov commented Jun 2, 2020

Why is this the only custom url scheme allowed through, while there are many many other custom url schemes that are still being blocked?

@troyready
Copy link
Contributor

@nermolov there's a broad fix in a testing release now that should finally address the core URL problem (instead of the previous strategy of whitelisting them).

(I hesitated to comment here on a closed PR & pollute it with comment spam, but since this is a particularly important issue I think it's relevant to cross-post)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants