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 configs for web embed mode #421

Merged
merged 3 commits into from
Nov 22, 2023
Merged

add configs for web embed mode #421

merged 3 commits into from
Nov 22, 2023

Conversation

JammingBen
Copy link
Contributor

@JammingBen JammingBen commented Nov 8, 2023

Description

Adds 3 new configs for the Web embed mode that were recently added to oCIS (see owncloud/ocis#7670 and owncloud/ocis#7766):

  • embed.enabled Defines if embed mode is enabled.
  • embed.target Defines how Web is being integrated when running in embed mode.
  • embed.messagesOrigin Defines a URL under which Web can be integrated via iFrame.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

@dj4oC
Copy link

dj4oC commented Nov 10, 2023

@JammingBen
may I ask you to add a second section to the helm chart instead?
the current version would eigther deploy embedd mode or web - not both.

@JammingBen
Copy link
Contributor Author

@JammingBen may I ask you to add a second section to the helm chart instead? the current version would eigther deploy embedd mode or web - not both.

As far as I understood this PR only gives the ability to set this config, but it will default to empty (=not being set). I don't know much about helm charts though, maybe some of our experts can verify this?

If my assumption is correct it means Web will run in normal mode. The embed mode can then always be activated via the query param mode=embed (and optionally embed-target=location).

Copy link
Contributor

@wkloucek wkloucek left a comment

Choose a reason for hiding this comment

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

I gave it a try after updating to oCIS 5.0.0-alpha.2 (because I heard on a different channel, that this version is needed).

When setting services.web.config.mode to embed, the regular oC Web that you can reach on whatever you have set in externalDomain will be replaced by the embeddable, stripped down oC Web. It looks like this:
image

In order to proceed with this PR we need to answer:

  • should the chart always ship a full oC Web on the externalDomain? (I think it wouldn't make much sense to not ship a full oC Web!? Reasons welcome why it makes sense to only ship oC Web embed)
  • there is a way to use oC Web embed

When it comes to the usage of oC Web embed, I have some questions:

  1. https://owncloud.dev/clients/web/embed-mode/ is providing an example how to integrate oC Web embed:
    <iframe src="<web-url>?mode=embed"></iframe>.
    Does that mean I can integrate a oC Web, that doesn't have the WEB_OPTION_MODE option set to embed? This is also what I got from

    The embed mode can then always be activated via the query param mode=embed (and optionally embed-target=location).

    Also appending ?mode=embed results in a embed view in my tests. Maybe the question is: will this be supported until depreciated?

  2. The oC Web embed seems to use /config.json as configuration. Is there documentation about whether a integrating application can overwrite some settings like e.g. tokenStorageLocal or providing access tokens instead of a OIDC login flow done by oC Web embed? Because if the config.json is the only mean of configuration, we might have a conflict between oC Web and oC Web embed or even multiple oC Web embed use cases (that can be solved by deployment in some cases).

@dj4oC
Copy link

dj4oC commented Nov 13, 2023

In order to proceed with this PR we need to answer:

should the chart always ship a full oC Web on the externalDomain? (I think it wouldn't make much sense to not ship a full oC Web!? Reasons welcome why it makes sense to only ship oC Web embed)

YES please always ship Web.

@JammingBen
Copy link
Contributor Author

  1. https://owncloud.dev/clients/web/embed-mode/ is providing an example how to integrate oC Web embed:
    <iframe src="<web-url>?mode=embed"></iframe>.
    Does that mean I can integrate a oC Web, that doesn't have the WEB_OPTION_MODE option set to embed? This is also what I got from

    The embed mode can then always be activated via the query param mode=embed (and optionally embed-target=location).

Yes, you can always enable embed mode via the query param mode=embed. That means there is no need to run the Web instance with the WEB_OPTION_MODE config.

Also appending ?mode=embed results in a embed view in my tests. Maybe the question is: will this be supported until depreciated?

I don't quite unterstand the question, why would we deprecate it? Both ways to enable the embed modes are valid, just the use cases may differ.

The oC Web embed seems to use /config.json as configuration. Is there documentation about whether a integrating application can overwrite some settings like e.g. tokenStorageLocal or providing access tokens instead of a OIDC login flow done by oC Web embed? Because if the config.json is the only mean of configuration, we might have a conflict between oC Web and oC Web embed or even multiple oC Web embed use cases (that can be solved by deployment in some cases).

The token overwrite has not been implemented yet. We need the deployed instance for this so we can determine how exactly the integration looks like and how we need to implement it Web.

@JammingBen
Copy link
Contributor Author

JammingBen commented Nov 22, 2023

Pushed again because the structure of the config changed a bit with owncloud/ocis#7766.

Edit: also bumped oCIS to 5.0.0-alpha.3.

@wkloucek wkloucek merged commit dcbccc0 into next Nov 22, 2023
1 check passed
@wkloucek wkloucek deleted the add-embed-config branch November 22, 2023 10:43
@wkloucek wkloucek mentioned this pull request Mar 18, 2024
11 tasks
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.

3 participants