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

Meetings iframe visibility #8307

Merged
merged 28 commits into from
Nov 26, 2021
Merged

Conversation

entantoencuanto
Copy link
Contributor

@entantoencuanto entantoencuanto commented Sep 6, 2021

🎩 What? Why?

This PR:

  • Adds a new enum attribute to meetings to control level access to iframe
  • Adds management in meetings admin panel to select iframe access level when iframe is enabled for meeting
  • Changes online meeting cell visibility considering:
    • Iframe access level of meeting
    • Privacy and transparency of meeting

📌 Related Issues

Testing

  1. Sign in as admin
  2. In the dashboard, go to edit a meeting form in a process
  3. Select "Online" as meeting type
  4. See that you have the options "Iframe embed type" and "Iframe access level". They should work as described at Online meetings iframe visibility to visitors/participants/registered #7719

📋 Checklist

🚨 Please review the guidelines for contributing to this repository.

  • CONSIDER adding a unit test if your PR resolves an issue.
  • ✔️ DO check open PR's to avoid duplicates.
  • ✔️ DO keep pull requests small so they can be easily reviewed.
  • ✔️ DO build locally before pushing.
  • ✔️ DO make sure tests pass.
  • ✔️ DO make sure any new changes are documented in docs/.
  • ✔️ DO add and modify seeds if necessary.
  • ✔️ DO add CHANGELOG upgrade notes if required.
  • ✔️ DO add to GraphQL API if there are new public fields.
  • ✔️ DO add link to MetaDecidim if it's a new feature.
  • AVOID breaking the continuous integration build.
  • AVOID making significant changes to the overall architecture.

📷 Screenshots

image

Embed type: embed in meeting page

image

Embed type: live event

image

Embed type: external

image

♥️ Thank you!

@entantoencuanto entantoencuanto changed the title Feat/meetings iframe visibility Meetings iframe visibility Sep 6, 2021
@entantoencuanto entantoencuanto force-pushed the feat/meetings_iframe_visibility branch 2 times, most recently from 1d52e16 to baf5af8 Compare September 7, 2021 16:24
@entantoencuanto entantoencuanto marked this pull request as ready for review September 7, 2021 17:23
@andreslucena
Copy link
Member

I think that we actually have some missing pieces in this puzzle, mainly we're missing the /live_event interface for an embedded type of meetings. One of the main use cases that we have in Decidim Barcelona is using Online Meetings with Jitsi and/or 8x8 or similar platforms (with WebRTC), and with this final PR, this isn't supported. This is closely related to the "Live Event" interface developed in #8065.

At the moment if I configure a Meeting with

  • Type: Online
  • Online Meeting URL: https://meet.jit.si/decidim-testing
    Then when I click in "Join Meeting" I go to the external link directly, but I need some way of configuring it to the "Live Event" UI.

So, to have this we'd need to have a couple of changes:

  1. Remove the "Show embedded iframe for this URL" checkbox
  2. Create a new dropdown selector called "Iframe embed type" with options:
    2.1 None: it doesn't do anything (no button, no live_event access). It's the default option. When this is selected "Iframe access level" is hidden.
    2.2 Embed in meeting page : current behavior as the checkbox checked, so it'd be migrated from the meetings that have already checked "Show embedded iframe for this URL"
    2.3 Open in live event page (with optional polls): the button would link to /live_event URL. ACLs for seeing the button and accessing /live_event URL would be honored by the "Iframe access level" setting in both cases.
    2.4 Open in a new tab: the button would link to the external link (current behavior of checkbox "Show embedded iframe for this URL" unchecked)
  3. Regarding "Embed in meeting page" (old "Show embedded iframe for this URL") we'd add the "meet.jit.si" URL to the whitelist. It doesn't need any transformation. In the future, we should be adding other services that people use to this whitelist, although I don't know if they need any kind of transformation (ie 8x8.com or Whereby.com), so for the moment I'd only add Jitsi.

As a summary:

Options Button Link Explanation
None No No Do nothing
Embed in meeting page No No Shows the iframe in the same page
Open in live event page (with optional polls) Yes Yes, link to /live_event Shows the iframe in the live event page
Open in a new tab Yes Yes, link to Online Meeting URL Opens the URL in a new tab

This is how it'd finally look like:

imatge
imatge

@entantoencuanto entantoencuanto force-pushed the feat/meetings_iframe_visibility branch from baf5af8 to f9ffe5c Compare September 20, 2021 18:08
@entantoencuanto
Copy link
Contributor Author

I have started working on the changes proposed

cc/ @decidim/product

@entantoencuanto entantoencuanto force-pushed the feat/meetings_iframe_visibility branch from 4822e0a to 892461a Compare September 22, 2021 20:40
@entantoencuanto
Copy link
Contributor Author

I've implemented the changes and also:

  • Added jitsi to the allowlist of embeddable in iframe URLs
  • Included a validation to reject meetings creation/updates when the URL doesn't fit in the embeddable allowlist and the iframe embed type is set as "Open in live event page (with optional polls)" (this validation also works if "Embed in meeting page" is set)

Ready to check in the test application @decidim/product

@andreslucena
Copy link
Member

andreslucena commented Sep 29, 2021

A couple of bugs I've found:

  • Given that I'm a visitor, and there's a meeting configured with "Open in live event page (with optional polls)" and "Only signed-in participants" then I see the "Link available soon" in the card (I shouldn't see it, it should be consistent with the meeting page):

imatge

  • Given that I'm a visitor, and there's a meeting configured with "Open in a new tab" and "Only signed-in participants" then I see the "Link available soon" in the card (I shouldn't see it, it should be consistent with the meeting page):

imatge

  • Given that there's a meeting configured with "Embed in meeting page" and the start date is a couple of months from now (for instance, 20220125), then I see the embed (I shouldn't see it, it should honour timing from Online meetings iframe visibility with time #7722)

imatge

  • In the admin form we should change the help text to better reflect these changes:

Only a few services allow embedding in meeting or live event (YouTube, Twitch and Jitsi)

imatge

@leio10 leio10 added in-review module: meetings type: feature PRs or issues that implement a new feature and removed in-review labels Oct 1, 2021
@entantoencuanto entantoencuanto force-pushed the feat/meetings_iframe_visibility branch from 0865a49 to c26e4f0 Compare October 4, 2021 18:58
@entantoencuanto
Copy link
Contributor Author

Hi, @decidim/product , I've added the requested changes and It's ready to test in the staging application

@andreslucena
Copy link
Member

andreslucena commented Oct 5, 2021

Hi, @decidim/product , I've added the requested changes and It's ready to test in the staging application

Awesome, I've reviewed it, we're getting closer!!

I've updated the description and Acceptance Criteria of #7719 so it's possible to follow with the last changes. A couple more details:

  1. I've left some review comments changing the help text so it's clearer

Regarding the updated Acceptance Ctiteria:

  1. There's one about not showing ACL if it doesn't make sense:
  • Given that I'm an administrator configuring an online meeting when I select "Iframe embed type" of "None" then I should not see the "Iframe access level" dropdown
  1. The other ones are related to the /live_event URL and a possible attack. If a user knows this URL then she can access it directly without honoring the defined ACL. She can know it because this is open source, she has already accessed this URL for another meeting and can infer it, she has received it from another participant that has access, etc... We'd need to follow the same ACL as the "show button" logic (that works as expected and defined!!). In this case, the ACLs that aren't working are:
  • Given that there's a Meeting with videoconference when it's configured with "Iframe embed type" of "Open in live event page (with optional polls)" and with "Iframe access level" of "Only signed-in participants" when I'm not logged in then I should not see the button and I should not be able to visit /live_event URL
  • Given that there's a Meeting with videoconference when it's configured with "Iframe embed type" of "Open in live event page (with optional polls)" and with "Iframe access level" of "Registered participants to this meeting" when I'm not logged in then I should not see the button and I should not be able to visit /live_event URL
  • Given that there's a Meeting with videoconference when it's configured with "Iframe embed type" of "Open in live event page (with optional polls)" and with "Iframe access level" of "Registered participants to this meeting when I'm logged in and not registered to this meeting then I should not see the button and I should not be able to visit /live_event URL

Could you check this @entantoencuanto 🙏🏽 ?

We're really close!! 🚀

@entantoencuanto entantoencuanto force-pushed the feat/meetings_iframe_visibility branch from c36034a to 401dc9b Compare October 13, 2021 18:08
@entantoencuanto
Copy link
Contributor Author

Hi, @andreslucena, I have added the changes requested. About the hiding of ACL select depending on iframe embed type selection the feature was in the code but the review app was not correctly deployed and these changes were ignored. Now it can be checked.

cc/ @decidim/product

@andreslucena
Copy link
Member

Hi, @andreslucena, I have added the changes requested. About the hiding of ACL select depending on iframe embed type selection the feature was in the code but the review app was not correctly deployed and these changes were ignored. Now it can be checked.

cc/ @decidim/product

Great! Now it works, although what I see is that when I select the "Open URL in a new tab" option in "Iframe embed type" I don't have the "Iframe access level". I should have it. Can you fix it?

Also:

  1. I've left some review comments changing the help text so it's clearer

This wasn't accepted, can you do that?

  1. The other ones are related to the /live_event URL and a possible attack. If a user knows this URL then she can access it directly without honoring the defined ACL. She can know it because this is open source, she has already accessed this URL for another meeting and can infer it, she has received it from another participant that has access, etc... We'd need to follow the same ACL as the "show button" logic (that works as expected and defined!!). In this case, the ACLs that aren't working are:

This isn't working as expected:

As a visitor (non-signed in participant), in a "Open in live event page (with optional polls)" and with "Iframe access level" of "Only signed-in participants, in the meeting page I don't see the button, but I can access the live event page. Maybe it wasn't deployed well to staging?

@entantoencuanto
Copy link
Contributor Author

Hi, @andreslucena, sorry for the delay. The iframe access level is now available also when "Open URL in a new tab" is selected. I've also deployed again and I think that it's working as expected now

cc/ @decidim/product

@andreslucena
Copy link
Member

andreslucena commented Nov 4, 2021

The iframe access level is now available also when "Open URL in a new tab" is selected. I've also deployed again and I think that it's working as expected now

Great, now it works as expected with this dropdown!! With that we can say that #7719 is fixed. I've also reviewed the acceptance criterias defined in #7720 and is fixed too. 🥳 🎉

But I see a problem with timing and that the "live event":

  • Given that there's a meeting configured with "Open in live event page (with optional polls)" and the start date is a couple of months from now (for instance, 20220125), then I can visit the /live_event URL (I shouldn't see it, it should honour timing from Online meetings iframe visibility with time #7722)

And also it's missing this one:

  • In the admin form we should change the help text to better reflect these changes:

Only a few services allow embedding in meeting or live event (YouTube, Twitch and Jitsi)

Regarding this last one, I've made the change myself. Please accept this

@entantoencuanto
Copy link
Contributor Author

Regarding this last one, I've made the change myself. Please accept this

Sorry, I thought it was already accepted, I must have done something wrong. I think now it's merged correctly

@entantoencuanto entantoencuanto force-pushed the feat/meetings_iframe_visibility branch from 224529a to a356011 Compare November 4, 2021 14:27
@entantoencuanto
Copy link
Contributor Author

Ready to check @andreslucena

cc/ @decidim/product

@entantoencuanto entantoencuanto force-pushed the feat/meetings_iframe_visibility branch from a356011 to 85bf1c6 Compare November 26, 2021 10:42
@entantoencuanto
Copy link
Contributor Author

Conflicts solved by rebasing

login_as user, scope: :user
end

it "shows the meeting link embedded" do
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the "meeting link embedded" as a concept, but then I realized it makes perfect sense with the "iframe_embed_type" of "open_in_new_tab" 🙈 🤷🏽

@andreslucena andreslucena merged commit 0abc0ea into develop Nov 26, 2021
@andreslucena andreslucena deleted the feat/meetings_iframe_visibility branch November 26, 2021 11:16
@andreslucena
Copy link
Member

Great job in this huge PR @entantoencuanto - the internal joke that I always make in @decidim/product when talking about these features is that this is going to be awesome for the next pandemia 🤣

Also, I found out while reviewing this that we didn't take into account what happens with meetings created by participants:

image

I think it'll be better to just handle this as another issue, as this PR is for sure an improvement on what we have until now. The definition would be to have what we have in the backend without the "live poll" option (as that's not implemented for meetings created by participants)

I've opened this issue in #8570 - it'd be great to have that when you have time @entantoencuanto. This will be the killer feature for v0.26.0, so as soon as we have that solved we'll release with that

quinHD pushed a commit that referenced this pull request Dec 1, 2021
Co-authored-by: Andrés Pereira de Lucena <andreslucena@users.noreply.github.com>
@alecslupu alecslupu added this to the 0.26.0 milestone Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: meetings type: feature PRs or issues that implement a new feature
Projects
None yet
4 participants