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

Added Header Authentication - Client Part #2362

Merged
merged 28 commits into from
May 4, 2024

Conversation

joewashear007
Copy link
Contributor

Client Side modifications for actualbudget/actual-server#312 for #1092

This change added the check for the "loginMethod" from the /account/use-bootstrap endpoint. It will then adjust the login page to automatically call the login endpoint if the loginMethod == "header".

I was having trouble getting this built and tested, so I would appreciate pointers or testing tips.

Thanks!

Copy link

netlify bot commented Feb 15, 2024

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit 8b0a5b0
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/663682797e2e430008716e02
😎 Deploy Preview https://deploy-preview-2362.demo.actualbudget.org
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

github-actions bot commented Feb 15, 2024

Bundle Stats — desktop-client

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count Total bundle size % Changed
9 4.7 MB 0%

Changeset

No files were changed

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

No assets were bigger

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
static/js/indexeddb-main-thread-worker-e59fee74.js 13.5 kB 0%
static/js/resize-observer.js 18.37 kB 0%
static/js/BackgroundImage.js 122.29 kB 0%
static/js/narrow.js 59.7 kB 0%
static/js/usePreviewTransactions.js 790 B 0%
static/js/AppliedFilters.js 20.41 kB 0%
static/js/wide.js 263.53 kB 0%
static/js/ReportRouter.js 1.22 MB 0%
static/js/index.js 2.99 MB 0%

Copy link
Contributor

github-actions bot commented Feb 15, 2024

Bundle Stats — loot-core

Hey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle.

As this PR is updated, I'll keep you updated on how the bundle size is impacted.

Total

Files count Total bundle size % Changed
1 1.2 MB 0%

Changeset

No files were changed

View detailed bundle breakdown

Added

No assets were added

Removed

No assets were removed

Bigger

No assets were bigger

Smaller

No assets were smaller

Unchanged

Asset File Size % Changed
kcab.worker.js 1.2 MB 0%

@youngcw
Copy link
Member

youngcw commented Feb 15, 2024

If the server gets set with the 'header' env var does that block using the password or just allow headers?

@joewashear007
Copy link
Contributor Author

It simply allows the header auth, and if that doesn't work it fail over to the regular password auth

@youngcw
Copy link
Member

youngcw commented Feb 15, 2024

Are you testing this with Authentik? Could you write up your steps to get that setup? Im missing something in how that should work

@joewashear007
Copy link
Contributor Author

I run authentik in docker with is pretty straight forward, but my setup also runs with nginx-proxy as my main reverse proxy. So my setup assumes that, other reverse proxy providers could be used. I also had to setup the main urls and such in various places.

  1. I setup a new Proxy Provider and set it to "Forward Auth", since I have nginx proxy manager instruction. I updated my nginx.conf with the auth and proxy_pass headers as in the documentation.
  2. I created a new Application for actual budget
  3. I updated the Embedded Output to select the proxy provider that I created.
  4. I created a new "finance" group and added following to the attributes block
additionalHeaders:
  X-ACTUAL-PASSWORD: <redacted>
  1. I assigned the "finance" group to actual budget application

Once this is in place, any call to my actual instance get checked for valid Authentik logins (through the magic of nginx). Authentik should then automagically pass on X-ACTUAL-PASSWORD header to my actual instance.

I hope that is good enough descriptions, I am happy to give my details on my setup, but not in the public github thread. I also haven't been able to test this end to end fully yet since I wasn't sure how to build a docker image that had my change from both the server and the client repo.

Copy link
Contributor

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Mar 17, 2024
@youngcw youngcw removed the Stale label Mar 17, 2024
@joewashear007
Copy link
Contributor Author

I am still trying to find time to work on for actualbudget/actual-server#312.

@joewashear007 joewashear007 force-pushed the master branch 3 times, most recently from 885ef0e to 4fc71e6 Compare March 27, 2024 20:53
@joewashear007
Copy link
Contributor Author

@youngcw , I think this is ready for a review. I had it working nicely in my local env. I tested the following scenarios:

  • Password login work
  • header login works
  • header with invalid password -> redirect to password login
  • invalid header or bad proxy trust -> redirect to password login

@twk3
Copy link
Contributor

twk3 commented Apr 10, 2024

I do have an authentik test environment working with this, but it took me all weekend to get setup and working due to various issues. So instead I wanted to throw up a test case for others that is a bit easier to test. So if anyone wants to additionally review this and the server side of the change, you can just have nginx sit as a proxy in front of your actual server and have it pass your password as a header. No authentik required for just testing purposes.

Here is an example for the nginx proxy: https://github.com/twk3/actual-auth-header-example

@joewashear007 now that I have a test environment setup, I can take a closer look at this.

@trafico-bot trafico-bot bot added ⚠️ Changes requested Pull Request needs changes before it can be reviewed again 🔍 Ready for Review and removed 🔍 Ready for Review ⚠️ Changes requested Pull Request needs changes before it can be reviewed again labels Apr 15, 2024
@joewashear007 joewashear007 requested a review from twk3 April 15, 2024 16:20
@twk3
Copy link
Contributor

twk3 commented Apr 25, 2024

@joewashear007 both PRs look ready for one final review (other than the merge conflict). And then I will merge. But now that we are in mergefreeze for the release, I am going to hold off on the final review until closer to when the freeze is lifted, so the review and a merge sanity check can happen at the same time.

Likely the end of next week, or over that weekend.

@mioiox
Copy link

mioiox commented Apr 26, 2024

Thanks for bringing up this feature. I personally can confirm it's an important one since I have a Kemp Virtual Load Master (VLM) in front of Actual with LDAP preauthentication enabed on the VLM - and I was struggling to make it work. So this header passing the "authenticated" flag to Actual server is a super cool feature. I am looking forward to it.
@twk3, do you have an idea if it will make it for the next release and when this release will take place?

@joewashear007
Copy link
Contributor Author

Thank you @twk3 - I will be pretty excited to get this merged

Copy link
Contributor

@twk3 twk3 left a comment

Choose a reason for hiding this comment

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

Ran a final round of testing today. Looks good.

There is some weirdness around signout when using the header (as we just sign back in). But the fact that we have the change server menu option, allows you to instead use that to 'sign out'.

Thanks @joewashear007 for your long work on this contribution

@twk3 twk3 merged commit ffddd9e into actualbudget:master May 4, 2024
19 checks passed
@Shazib
Copy link
Contributor

Shazib commented Jun 2, 2024

Sorry to necro @joewashear007, Can you confirm if your proxy setup allows you to use the desktop app and web app without any sever connection issues?

I've using Authelia + Clourflare tunnels and have trouble getting the desktop app to work as the calls to the API actually return a 302 redirect which at the moment the fetch calls don't handle well.

@joewashear007
Copy link
Contributor Author

@Shazib - so the PWA/service worker network caching for offline usage is a separate issue (which is quite annoying).

This PR works with passing the password via hreader from the proxy. A fix for the offline usage with an auth proxy would need to happen with the service workers. It needs a URL endpoint that can detect if the proxy is expired that is not cached. A work around that I have been using on desktop browsers is to unregister the service worker and then reload.

I would like to make a PR to fix this, but I haven't gotten the time yet

@Shazib
Copy link
Contributor

Shazib commented Jun 2, 2024

Thanks for the info @joewashear007!

I will give adding the header a go.

For now, to solve the other issue, I've added a bypass rule using cloudflare's 'warp' client. So if my specific PC tries to access my actual instance it bypasses the auth layer entirely.

I guess in a perfect would we would add the OAuth2 capabilities directly to Actual. in the browser the auth redirect works ok, but the desktop app needs additional handling.

@max-tet
Copy link

max-tet commented Jun 5, 2024

Is there a way to completely remove the need to set a password?
As far as I understand, this feature allows you to pass the password in a header value but when accessing the server for the first time, a password must still be set, right?

For a hosting setup using a reverse proxy and auth provider, this should also be unneccessary. In fact, IMO there should be no password at all.

@Shazib
Copy link
Contributor

Shazib commented Jun 5, 2024

Is there a way to completely remove the need to set a password? As far as I understand, this feature allows you to pass the password in a header value but when accessing the server for the first time, a password must still be set, right?

For a hosting setup using a reverse proxy and auth provider, this should also be unneccessary. In fact, IMO there should be no password at all.

This sort of circles back to my original query, and would require Actual to natively support OIDC so it can pass auth responsibilities off to your auth provider.

This is not something that is currently supported so someone would need to implement it and submit a PR, it will likely not be a priority for the core team.

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.

6 participants