[conneryn]

This PR is intended to resolve #614 . This is also an attempt for me to get more familiar with the code base, I'm hoping to be able to contribute in more meaningful ways, eventually.

Primary goal:
Add a /.well-known/ file to the web server project that can direct clients (web and mobile) to the appropriate backend API URL.

Solution:

1. Adds an IMMICH_SERVER_PUBLIC_URL environment variable that can be used to override the default /api relative path for the API. I can be another relative path, or a separate domain altogether (ex: api-immich.example.com)
2. Adds a /.well-known/immich route to the web project that returns a JSON payload (ex: { "api": { "endpoint": "https://api-immich.example.com" } })
3. Updates the mobile app to lookup the .well-known file first, and use the endpoint, if found. If not found, the app fallbacks on assuming the URL provided by the user already points to the API (for backwards compatibility).

In order to make it possible to serve the API via a separate domain name, a few other changes were required:

1. Add an ALLOW_CORS_ORIGIN environment variable, which can be used to enable cors (previously, only development builds had this enabled).
2. Update the websockets connection management to remove assumptions about the domain and path to the API.

Questions for reviewers:

1. Naming of things, I made a best guess for path, property, and method names, but I am very open to suggestions.
2. I am still getting familiar with where/how business logic should be organized, so let me know if anything is in the wrong place. For example, I have some duplicated code in multiple places:

   final serverEndpoint = await apiService.resolveEndpoint(serverUrl.toString());
   apiService.setEndpoint(serverEndpoint);
   Hive.box(userInfoBox).put(serverEndpointKey, serverEndpoint);
   

   I considered moving this all within the apiService (ex: as a resolveAndSetEndpoint() method), but I wasn't sure if that would be an acceptable location to be accessing Hive boxes.
3. I do not have a local setup that uses oauth, so any help testing/validating that flow works as expected would be much appreciated.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
immich	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Jan 19, 2023 at 3:33AM (UTC)
immich-code-coverage	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Jan 19, 2023 at 3:33AM (UTC)

[alextran1502]

If I understand correctly, the user still needs to fill in the server URL input on the mobile app, correct?

[jrasm91]

Looks good to me. I haven't figured out how to build the mobile app locally yet, otherwise I would have tested OAuth, but looking at the code I don't see why there would be any problems.

RE: duplicate code. I'm not a mobile developer, but maybe it'd be worth adding a resolve: boolean as a second argument to setEndpoint() and then moving the validate call inside of that method. It looks like it runs pretty much all the time, except when the backup service loads the value.

[conneryn]

If I understand correctly, the user still needs to fill in the server URL input on the mobile app, correct?

Yes, the user will still need to enter a server URL, but this URL can now be the base web URL rather than the full API endpoint (ex: https://immich.example.com/ instead of https://immich.example.com/api). It also allows the API to be exposed over custom paths and/or domains.

Also, as per the discussions in #614, it allows several short hands. Namely, if only a hostname[:port] is provided, it defaults to https. So if a user only enters: immich.example.com it will look for https://immich.example.com/.well-known/immich

RE: duplicate code. I'm not a mobile developer, but maybe it'd be worth adding a resolve: boolean as a second argument to setEndpoint() and then moving the validate call inside of that method. It looks like it runs pretty much all the time, except when the backup service loads the value.

Thanks for the feedback! I'd be happy to combine them if we want. A few minor thoughts on why I feel it may be better to kept them separate, though:

1. resolve needs to be async (has network calls), while setEndpoint is not.
2. resolve consists of business logic, while setEndpoint is simply setting a service configuration.
3. I think the backup service alone is a good enough reason to keep the simple setEndpoint as is.
4. I wanted a clear(er) distinction between "serverUrl" (an input the user provides) and the actual API endpoint. Creating a combined method with just a boolean flag could make this more confusing.

Instead of removing/changing setEndpoint, how about renaming resolveEndpoint to resolveAndSetEndpoint, which internally also calls setEndpoint? That way all the instances that call resolveEndpoint don't need to also call setEndpoint, but setEndpoint still exist in it's simplest form (for when resolving isn't necessary).

[alextran1502]

Let me know when the PR is ready for a throughout review

[jrasm91]

LGTM

[jrasm91]

I think translations are pulled from an external source, right @alextran1502 ?

[alextran1502]

Correct

…

On Sat, Jan 14, 2023 at 8:09 PM Jason Rasmussen ***@***.***> wrote: I think translations are pulled from an external source, right @alextran1502 <https://github.com/alextran1502> ? — Reply to this email directly, view it on GitHub <#1308 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGONL7XFBIKTZ6A2H7CQSKTWSNL53ANCNFSM6AAAAAATYWIKCI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[conneryn]

I think translations are pulled from an external source, right @alextran1502 ?

Good to know, thanks! In the context of this PR what do I need to do then? Do I only update the en-US translations, and once the PR is merged (and localizely pulls in the change), the other languages will be edited/set through localizely? Or do I need to do everything through localizely? Something else?

Let me know when the PR is ready for a throughout review

@alextran1502 Other than updating the translations, I think this PR is now ready for a more thorough review.

[alextran1502]

Do I only update the en-US translations, and once the PR is merged (and localizely pulls in the change), the other languages will be edited/set through localizely

Correct, you only need to update the en-US file, so you can revert the changes in other translation file

[alextran1502]

The environmental name is a bit confusing :-? I wonder if we have any other wording that can make it clearer to provide the example in the info box?

[conneryn]

Yeah, I definitely struggled to think of a good name here, largely because IMMICH_SERVER_URL and IMMICH_WEB_URL were already taken to mean the internal/private locations for those services, and this is effectively the public facing version of those. A few alternatives I'd been considering: IMMICH_PUBLIC_SERVER_URL, IMMICH_EXTERNAL_SERVER_URL, IMMICH_SERVER_URL_EXTERNAL, etc. Alternatively, we could be super explicit with somethineg like: IMMICH_WELLKNOWN_SERVER_URL.... but I am very open to suggestions!

I also tried updating the comment and example a bit, to hopefully make it clearer. Let me know if you think that helps at all.

[Mortein]

I prefer IMMICH_API_URL_EXTERNAL, as it's for the API, not the server. Otherwise, IMMICH_SERVER_URL_EXTERNAL might work?

It's distinct from IMMICH_SERVER_URL (used for internal communication), and distinct from the web URL (which doesn't need/use a variable).
In some cases, all three URLs may be the same, but that's not necessarily always the case.

[conneryn]

Thanks, that sounds good to me, I've switched it to "IMMICH_API_URL_EXTERNAL". I really like the idea of adding API in there, as that truly is what the URL represents, but I originally wasn't sure if we should introduce a new term (seems how everywhere else its still referenced as the more ambiguous "server").

[alextran1502]

Everything looks good! I test on Oauth instance and it can detect the OAuth login button. Besides a small comment about the variable name and revert back the changes for other translation file besides English. We are good to go. I will merge this in once we resolved the topics above.

[alextran1502]

I believe we made some mistakes here. I've tested this scenario and the web cannot be accessed. I am accessing the web through http://local-ip:2283

And for any optional config, shouldn't it be starting out as a commented line?

[conneryn]

Re: optional configuration: I commented out the default value from the .env.example. Although, it doesn't look like any other optional environment variables are commented it, so would just leaving "blank" be more inline with the current convention?

As for accessing from the web being broken, looks like you are right, thanks for catching that. I had previously only tested as far as logging in, and didn't actually try any actions once logged in. I now see that all the client-side API requests return 401s when the API is served on a separate domain name. This is because the web project (both browser and server side) never actually set the immich-access-token cookie, and instead relies on the API server to directly set this (which doesn't work when on separate domain names).

I have created a "temporary" fix for this, by setting the cookie from the web project, and ensuring setAccessToken is called both in browser and on the server side (of the web project). HOWEVER, I think a more permanent solution should be consider to address this properly. Do you think I should look to address this here, or make a separate Issue to improve this later?

[vercel]

@conneryn is attempting to deploy a commit to the immich Team on Vercel.

A member of the Team first needs to authorize it.

[jrasm91]

I don't think we need to set the access token in the web anywhere because it should use https secure cookies, etc.

[jrasm91]

The server API uses an access token, the client API uses http only, secure cookies for authentication.

https://github.com/immich-app/immich/blob/main/web/src/routes/+layout.server.ts

[alextran1502]

I don't think we need to set the access token in the web anywhere because it should use https secure cookies, etc.

This is correct

[conneryn]

@jrasm91, there are a few issues with the current usage of cookies and client access:

1. the cookie is ONLY set by the API project currently, which means if the API and WEB projects are running on different domains, then the web project will not have access to this data (aka: request.headers.get('cookie') in +layout.server.ts will always return empty)
2. The web's client-side usage of the api client (import { api } from '@api'), never sets an accessToken on the client, so it never includes the Authorization bearer token, and therefore it fully relies on cookies being properly sent to the API. So again, if the web and server projects are on different domains this won't happen as expected.
3. the web's sever side usage of the api client (import { apiServer } from '@api') is a global variable, therefore shared across ALL requests on the server. For example calling apiServer.setAccessToken(...) in an on load event for one request will actually shared across all requests. Although the race conditions may be rare (and most users probably only have one or two users using their instances of immich anyway), this could definitely present a security issue.

[alextran1502]

The cookie is ONLY set by the API project currently, which means if the API and WEB projects are running on different domains, then the web project will not have access to this data (aka: request.headers.get('cookie') in +layout.server.ts will always return empty)

Have you tried testing this scenario I believe the three variables below are for this purpose

I have a feeling this PR has gone a bit too far from its original intention. I think the idea is to have the .well-known api endpoint to return the current server endpoint for the endpoint url in the mobile app. let's stick with the simplest implementation to get to that point. Other issues can be addressed in a separate PR

[jrasm91]

For clarification each request that svelte handles has its own context/scope if I'm not mistaken. Every request passes through layout.server.ts and parses/sets the access token. It is not shared between users, requests, etc., and not a secretary concern.

Yes, different domains would indeed cause an issue, I missed that was what you were trying to solve. I think there are multiple ways to solve this problem though, and I don't know if this is the right forum to have that discussion and make that decision.

Svelte itself can function as a reverse proxy, and still uses cookies from the server, we could send API requests with include credentials, or we could switch to access token via bearer auth. There are pros and cons to each and it might be worthwhile to investigate them and address that specific use case (API on a different domain) in another PR.

I'd be down to continue a conversation about implementing in discord though if you want to start a thread on the topic.

[conneryn]

@alextran1502 I'm fine with simplifying this PR to only address the .well-known use-case on mobile. I've removed all the changes related to supporting distinct web and server URLs (including cors).

Re: Service Addresses. Unfortunately, the three variables mentioned won't quite help in this case. I think those are intended to be internal addresses, as they are used by NGINX (and other services) for backend routing, and not actually exposed to end-user/clients.