Skip to content

MOBILE-4826 core-iframe: Add allow attribute to iframe #4507

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

MOBILE-4826 core-iframe: Add allow attribute to iframe #4507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/core/components/iframe/core-iframe.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@
<!-- allowfullscreen cannot be set dynamically using attribute binding, define 2 iframe elements. -->
@if (allowFullscreen) {
<iframe #iframe class="core-iframe" [attr.id]="id" [ngStyle]="{'width': iframeWidth, 'height': iframeHeight}" [src]="safeUrl"
allowfullscreen="true" [class.core-iframe-loading]="loading">
allowfullscreen="true" [class.core-iframe-loading]="loading" [allow]="allow()">
</iframe>
} @else {
<iframe #iframe class="core-iframe" [attr.id]="id" [ngStyle]="{'width': iframeWidth, 'height': iframeHeight}" [src]="safeUrl"
[class.core-iframe-loading]="loading">
[class.core-iframe-loading]="loading" [allow]="allow()">
Copy link
Contributor

Choose a reason for hiding this comment

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

This is throwing an error, that's probably why behats are failing:

Angular has detected that the allow was applied as a binding to an <iframe> (used in the 'CoreIframeComponent' component template). For security reasons, the allow can be set on an <iframe> as a static attribute only.
To fix this, switch the allow binding to a static attribute in a template or in host bindings section. Find more at https://angular.dev/errors/NG0910

Copy link
Contributor

Choose a reason for hiding this comment

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

With your patch, you let the component to specify the allow string, but we aren't using it anywhere so all the iframes will behave exactly as they did.

If Poodll (the issue reporter) is a site plugin using core-iframe then that's ok because it means they will be able to supply the "allow" information when using core-iframe in their plugin template. But if Poodl is used somewhere else where we use core-iframe ourselves then this solution will not work for them. I don't know how Poodll works so I don't know their use cases, I just thought it was worth mentioning to be sure this solution is fixing the problem.

Even if Poodll use case is covered with this change, maybe we want to allow these permissions in some other cases (e.g. a SCORM?). We might want to discuss this with Juan.

Copy link
Contributor

Choose a reason for hiding this comment

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

Finally, the testing instructions of the issue are wrong. The tests done in the testing instructions will work without your patch, because your patch doesn't affect iframes created in LMS content, those iframes don't use the core-iframe component.

If we decide to keep this solution and don't use the "allow" input in our app, only allow using it for plugins, then you will need to create a dummy plugin to test that this works properly.

</iframe>
}

Expand Down
2 changes: 2 additions & 0 deletions src/core/components/iframe/iframe.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
SimpleChange,
OnDestroy,
inject,
input,
} from '@angular/core';
import { SafeResourceUrl } from '@angular/platform-browser';

Expand Down Expand Up @@ -70,6 +71,7 @@ export class CoreIframeComponent implements OnChanges, OnDestroy {
this.initIframeElement();
}

readonly allow = input<string>();
@Input() src?: string;
@Input() id: string | null = null;
@Input() iframeWidth = '100%';
Expand Down
Loading