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(mobile): Add support for Basic Authentication #6840

Merged
merged 1 commit into from Feb 4, 2024

Conversation

rovo89
Copy link
Contributor

@rovo89 rovo89 commented Feb 1, 2024

Use the x-immich-user-token header instead of Authentication: Bearer <token> to allow for Basic Authentication credentials in the server URL (e.g. https://user:pass@example.org). Most of the codebase handles the latter automatically, however the Authorization: Basic ... header used to be overwritten.

Only exception: socket_io_client doesn't seem to support getting credentials this way, so we have to add them via manually crafted Authorization header.

Tested on Android against nginx reverse proxy. With these changes, the nginx log doesn't show any unauthenticated requests anymore, so I hope I found all places.

While I was at it, I changed all variable names to accessToken. Some were called authToken, token or even jwtToken.

@danieldietzler
Copy link
Member

Please look for other PRs before opening one :) (#6617)

@danieldietzler
Copy link
Member

danieldietzler commented Feb 1, 2024

Sorry just read your comment on the other PR

@rovo89
Copy link
Contributor Author

rovo89 commented Feb 1, 2024

Sorry just read your comment on the other PR

Maybe I should have mentioned the other PR in the first comment. Didn't expect someone to react that fast. 😉

By the way, I didn't touch mobile/openapi/lib/auth/oauth.dart because it seems to be auto-generated. Not sure if that file is relevant for mobile at all - if yes, it might mean that OAuth and Basic Authentication can't be used at the same time (unless the generator can be told to set the other header instead).

@jpg0
Copy link

jpg0 commented Feb 2, 2024

I see that you manually add the basic auth Authorization header in the websocket connection. Are you 100% sure that the header will be added for the normal http requests? I don't believe that they were when I tested, plus I cannot see any logic in the dart http lib which would do so.

Other than that it looks good to me.

@rovo89
Copy link
Contributor Author

rovo89 commented Feb 2, 2024

Yes, because I tested it. Would be great if you could also test with your setup.

This might be the place where the credentials get added: https://github.com/dart-lang/sdk/blob/2a88162b841b5b93a36f698336404dc8f4e9b150/sdk/lib/_http/http_impl.dart#L2162

@jpg0
Copy link

jpg0 commented Feb 2, 2024

I just tested with my setup and indeed it does work!

The source that I was looking at is https://github.com/dart-lang/http/tree/master/pkgs/http - which I thought was being used as the import looked like a package rather than part of the SDK. I've never used Dart before so probably got this wrong. Regardless, as long as it works, and it certainly appears to, I'm happy.

@alextran1502 alextran1502 merged commit 5061c35 into immich-app:main Feb 4, 2024
29 checks passed
@rovo89 rovo89 deleted the mobile_basic_auth branch February 5, 2024 02:30
@kocane
Copy link

kocane commented Mar 13, 2024

Sorry for what might be a dumb question but is this supported in the lastest version of the app?

@danieldietzler
Copy link
Member

Sorry for what might be a dumb question but is this supported in the lastest version of the app?

Yes, it should be.

@kocane
Copy link

kocane commented Mar 13, 2024

Yes, it should be.

EDIT: Had to get the server up to date (derp) and it works now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants