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

feat: browser extension, custom server support #1220

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

alclary
Copy link
Contributor

@alclary alclary commented Jan 3, 2024

Hello, I think this is now in a good place to share! I've worked to implement custom server support into the manifest v3 webextension, per julianpoy/RecipeSage-selfhost#65. I experimented with a few ways to present this option visually to users and have worked to minimize its impact on the majority of users who will continue to use the primary recipesage.com instance. Totally open to suggestions on your further preferences for UI presentation. Seems to be working well on Firefox (desktop and mobile) and Chrome.

Changes include:

  • Allows a user-defined API URL (https only) to be provided at the time of initial login if the user does not prefer to use the primary recipesage.com instance
  • Additional form validation for initial login
  • Extension Settings page allows user-defined entry of "Base URL" for their site
    -- Note: the base URL is automatically inferred by stripping the api subdomain from the supplied API URL. That is, if the user supplies api.yourdomain.tld the base URL yourdomain.tld will be inferred. This works so long as the user's API is accessible via an API subdomain one level higher than the base URL. However, in situations where this is not the case, the Extension Settings page can be utilized to set a custom 'Base URL.'

Considerations to note:

  • Extension currently only supports connecting to an API accessible via an https scheme, otherwise the Manifest v3 default Content-Security-Policy (CSP) is violated. For some self-hosted users that only want to access their instance on LAN and don't want to deal with self-signed certs or DNS + reverse proxy to setup https, this may be an annoyance. For those users, the default CSP can be overridden in the manifest.json file. I.E. Manifest v3 defaults to "script-src 'self'; upgrade-insecure-requests;", but the upgrade-insecure-requests policy will prevent an http address to be reached. Additionally, they would need to remove the tiny form validation if (!server.startsWith("https://")) in action.js that ensures that only https API addresses are supplied.
  • The Extension Settings page update includes an API URL field, but I have set it to disabled by default as it seems like it only opens more opportunity for configuration errors at the moment. The API URL can always be customized on that first login and it's easier to derive the base URL from the API URL than vice-versa (see my note above). Please let me know if you would just prefer this field to be removed.

image
image
image

@julianpoy
Copy link
Owner

Fantastic - I'll take a look at this shortly, really appreciate the time and care you put into this!

@julianpoy
Copy link
Owner

Extension Settings page allows user-defined entry of "Base URL" for their site
-- Note: the base URL is automatically inferred by stripping the api subdomain from the supplied API URL. That is, if the user supplies api.yourdomain.tld the base URL yourdomain.tld will be inferred. This works so long as the user's API is accessible via an API subdomain one level higher than the base URL. However, in situations where this is not the case, the Extension Settings page can be utilized to set a custom 'Base URL.'

I like the fact that you're inferring here since it means less configuration for the user, but all self-hosted instances use yourdomain.tld/api/ as the base URL for the API (unless some custom routing scheme is designed). I think we should ask the user for their RecipeSage hostname during signup rather than the API address, and update the inferring logic to add /api to the end of that address.

@julianpoy
Copy link
Owner

The Extension Settings page update includes an API URL field, but I have set it to disabled by default as it seems like it only opens more opportunity for configuration errors at the moment. The API URL can always be customized on that first login and it's easier to derive the base URL from the API URL than vice-versa (see my note above). Please let me know if you would just prefer this field to be removed.

I agree with you about the configuration issues bit. I do not officially support users changing the routing configuration supplied in the selfhost configs. Many other things will break if they do, and there's really no reason for them to change it. Imo it's completely fine for us to assume that the API address will always be https://yourdomain.tld/api/. The only thing we should allow for is if they host RecipeSage on a subpath. For instance, if they supply https://yourdomain.tld/recipesage we should make the API address https://yourdomain.tld/recipesage/api.

);
});
chrome.storage.local.set(
{ token: null, api_url: null, base_url: null },
Copy link
Owner

Choose a reason for hiding this comment

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

[non-blocking] Do we perhaps want to preserve their custom domain if they have an expired session? It might be a little frustrating if every time a session expires they have to re-type their custom domain

@alclary
Copy link
Contributor Author

alclary commented Jan 4, 2024

Ah, great points. I totally forgot that I configured a reverse proxy for my instance to utilize the api subdomain. I should have a chance to work on your comments in the next day or two. Thanks.

@julianpoy
Copy link
Owner

Given that, I don't mind keeping the setting for overriding the API URL as it stands now tucked away in the settings page for users who go full-custom such as yourself.

@bedelaitre
Copy link
Contributor

Hi,

Congratulations!!! It's just what I need

@julianpoy
Copy link
Owner

I totally forgot that I configured a reverse proxy for my instance to utilize the api subdomain

I actually wonder if your selfhosted instance is using that api subdomain. Did you change any of the frontend source files or is it by chance still using /api?

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