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 OpenID Connect authentication #1164

Closed
wants to merge 5 commits into from
Closed

Conversation

apilat
Copy link

@apilat apilat commented Jun 22, 2023

Partially addresses #61 and #515 by adding support for OpenID Connect and generally a more flexible approach to authenticating with the server. Sister PR for server available as actualbudget/actual-server#219

The flow of the bootstrap/login process is

  • client sends /bootstrap request which now accepts an additional parameter openid: {issuer, client_id, client_secret, server_hostname}.
  • client can request GET /login-methods to receive a list of supported login methods (currently password or openid)
  • client sends POST /login-openid with return_url parameter (pointing back to the frontend), server returns redirect_url leading to the openid issuer, client redirects
  • user authenticates with openid provider, which redirects to /login-openid/cb on server
  • server verifies openid authentication, then redirects to /login/openid-cb on client, passing the token
  • client saves token and registers sign in

Some of the changes in the code and not backwards compatible (most importantly server SQL schema and /account/bootstrap behavior). I'm not sure what the project's approach to such changes is - it might be possible to add in special cases to allow a client to connect to an older server. However, this might not be necessary if users are expected to self-host instances and upgrade the server and client in tandem.
A more pressing issue is database schema migration - I couldn't find any precedent for how this is done in the server so I would appreciate the maintainer's view on the best approach to do this.

For a user that doesn't use OpenID, this PR slightly changes the bootstrap behavior for passwords as well. Namely, after setting up the password in /bootstrap, the user isn't instantly granted access but instead redirected to /login. This is quite an important change for OpenID as otherwise the user has no way of knowing whether the OpenID configuration works at all. In the case of password authentication, misconfiguration is less likely but nevertheless double-checking the user remembers the password doesn't seem too inconvenient to me.

I am not very familiar with React, so let me know if I have broken any coding conventions. Likewise with web design, I welcome improvements to the presentation of the login/bootstrap screens.

apilat added 3 commits June 22, 2023 00:03
The server has a backwards incompatible change of no longer returning a
valid token during bootstrap. This is an intentional choice to force the
user to log in after configuring the server, in order to make sure the
configuration is valid.
@netlify
Copy link

netlify bot commented Jun 22, 2023

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit 28c323e
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/649b2cbf3e1c4a0008f483b9
😎 Deploy Preview https://deploy-preview-1164--actualbudget.netlify.app
📱 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

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

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

This is awesome! I have a few comments here and a few more on the server PR but overall this approach seems solid and we’d like to have OIDC support built in. (I am also excited to use the new flexibility on the server to support passkeys)

packages/loot-core/src/server/main.ts Outdated Show resolved Hide resolved
@MatissJanis
Copy link
Member

👋 Sorry for the slow review. Overall this looks very exciting!

Some issues:

  • can you add a vertical separator between the regular login option and the "advanced" login? Currently it seems like the openid button is part of the usual registration flow, but it is actually different and hence should be more easily indistinguishable as such
Screenshot 2023-07-20 at 19 31 35
  • can you replace the "actual is super fast..." text with set-up instructions for openid? Think: if I'm a new user and don't know what openid is - how can we explain easily what it is, why use it and how to set it up (maybe link to docs set-up guide?)
  • can you make the buttons side-by-side: left side - "back" (with an arrow) - and right side - ok (primary button)?
  • if the form is not filled and you press "ok" the next page will break and there is no way to return back
Screenshot 2023-07-20 at 19 33 18
  • "change password" link is visible in the app even though I had set up openId

How can I fully test the openid solution? Do I need to set-up a local server? Maybe there's an easier way to use a 3rd party service (for testing purposes)? Could you provide a bit more information so I could test the end-to-end flow?

@carkom
Copy link
Contributor

carkom commented Jul 29, 2023

If you don't have an openid server then https://oauth.tools/ is a good resource for testing.

@MatissJanis
Copy link
Member

👋 This PR seems to have gone stale with many conflicts. If you resume working on it - please open a new PR and we'll happily work with you to get this merged ASAP.

@MissakaI
Copy link

This seems as a welcoming change for actual. Quick question, How can a user with existing budget files move to a new instance with this openid configuration.

Also @apilat I would love to contribute to this as well.

@k8ieone
Copy link

k8ieone commented Jan 2, 2024

I'd love to see this implemented, this is currently the one missing feature that keeps me from hosting Actual. Is there any way a non-developer like me could help?

@RefineryX
Copy link

Would also LOVE to see this implemented!!

@lelemm lelemm mentioned this pull request Jun 26, 2024
@lelemm lelemm mentioned this pull request Nov 22, 2024
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.

7 participants