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

Multiuser and OpenId #447

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

Multiuser and OpenId #447

wants to merge 20 commits into from

Conversation

lelemm
Copy link

@lelemm lelemm commented Sep 4, 2024

No description provided.

Copy link

netlify bot commented Sep 4, 2024

Deploy Preview for actualbudget-website ready!

Name Link
🔨 Latest commit 2508f20
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget-website/deploys/676a297f31d7060008c1d828
😎 Deploy Preview https://deploy-preview-447.www.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.

docs/advanced/oauth-auth.md Fixed Show fixed Hide fixed
docs/advanced/oauth-auth.md Fixed Show fixed Hide fixed
docs/advanced/oauth-auth.md Fixed Show fixed Hide fixed
docs/advanced/oauth-auth.md Fixed Show fixed Hide fixed

This comment has been minimized.

This comment has been minimized.

docs/advanced/oauth-auth.md Fixed Show fixed Hide fixed

This comment has been minimized.

Copy link
Member

@RubenOlsen RubenOlsen left a comment

Choose a reason for hiding this comment

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

I hope I have not been to crass in my comments and suggestions.

Also please do not use the backtick when referring to elements in the UI, use italics or bold to reference the elements and bold or italics to reference the content of the elements. Italics are preferred over bold to reference elements.

docs/advanced/multi-user.md Outdated Show resolved Hide resolved
docs/advanced/multi-user.md Outdated Show resolved Hide resolved
docs/advanced/multi-user.md Outdated Show resolved Hide resolved
docs/advanced/multi-user.md Outdated Show resolved Hide resolved
docs/advanced/multi-user.md Outdated Show resolved Hide resolved
docs/advanced/oauth-auth.md Outdated Show resolved Hide resolved
docs/advanced/oauth-auth.md Outdated Show resolved Hide resolved
docs/advanced/oauth-auth.md Outdated Show resolved Hide resolved
docs/advanced/multi-user.md Outdated Show resolved Hide resolved
docs/advanced/multi-user.md Outdated Show resolved Hide resolved
@lelemm
Copy link
Author

lelemm commented Nov 27, 2024

@RubenOlsen thanks for your dedication on this. Took this long for me to notice that you reviewed this.
There was multiple changes on how multiuser and openid was implemented since this PR. I'm waiting for the frontend PR be approved, than I will come back to this.

docs/advanced/multi-user.md Outdated Show resolved Hide resolved
docs/advanced/oauth-auth.md Outdated Show resolved Hide resolved
@lelemm
Copy link
Author

lelemm commented Dec 16, 2024

@MikesGlitch this doc is completely wrong atm. I have to redo it. Since code review started I changed the functionality so much.
For example: there is no more env variables/config.json for this for 2 reasons:

  • multiuser was removed. We now consider that if openid is enabled, multiuser is enabled by default
  • the openid config.json configurations was removed because now we have fallback authentication. In other words, you ALWAYS have to do the server password bootstrap to enable openid from the settings after.

lelemm and others added 4 commits December 17, 2024 08:28
@lelemm lelemm marked this pull request as ready for review December 17, 2024 12:41
@actual-github-bot actual-github-bot bot added 🔍 Ready for review Someone needs to look into this. and removed 🚧 WIP labels Dec 17, 2024
@lelemm
Copy link
Author

lelemm commented Dec 17, 2024

@RubenOlsen @MikesGlitch
This has a conflict because the "Advanced" menu does not exists anymore.
Where This two pages must go in the menu so I can resolve the conflict?

@youngcw
Copy link
Member

youngcw commented Dec 17, 2024

@RubenOlsen @MikesGlitch This has a conflict because the "Advanced" menu does not exists anymore. Where This two pages must go in the menu so I can resolve the conflict?

I think it should go under Installation and Configuration

@lelemm
Copy link
Author

lelemm commented Dec 17, 2024

@RubenOlsen @MikesGlitch This has a conflict because the "Advanced" menu does not exists anymore. Where This two pages must go in the menu so I can resolve the conflict?

I think it should go under Installation and Configuration

done

This comment has been minimized.

@matt-fidd
Copy link
Contributor

@RubenOlsen @MikesGlitch This has a conflict because the "Advanced" menu does not exists anymore. Where This two pages must go in the menu so I can resolve the conflict?

I think it should go under Installation and Configuration

@youngcw - do you have any objection to this being moved under "Experimental" so it can't be misconstrued as stable?

@youngcw
Copy link
Member

youngcw commented Dec 23, 2024

@RubenOlsen @MikesGlitch This has a conflict because the "Advanced" menu does not exists anymore. Where This two pages must go in the menu so I can resolve the conflict?

I think it should go under Installation and Configuration

@youngcw - do you have any objection to this being moved under "Experimental" so it can't be misconstrued as stable?

Yeah that would be best

Copy link
Contributor

@matt-fidd matt-fidd left a comment

Choose a reason for hiding this comment

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

A few comments

Could you update the images with the recent UI changes too please?
There's also one spellcheck alert left

Thanks for your work on this

docs/advanced/multi-user.md Outdated Show resolved Hide resolved
docs/advanced/multi-user.md Outdated Show resolved Hide resolved
@@ -0,0 +1,114 @@
# Authenticating With OpenID Provider
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be moved under experimental and have the experimental banner (can be found in the other pages) added to the top please?

docs/advanced/oauth-auth.md Outdated Show resolved Hide resolved
docs/advanced/oauth-auth.md Outdated Show resolved Hide resolved
docs/advanced/oauth-auth.md Outdated Show resolved Hide resolved
docs/advanced/oauth-auth.md Outdated Show resolved Hide resolved
docs/advanced/oauth-auth.md Outdated Show resolved Hide resolved
docs/advanced/oauth-auth.md Outdated Show resolved Hide resolved
docs/advanced/multi-user.md Outdated Show resolved Hide resolved
@actual-github-bot actual-github-bot bot added ⚠️ Changes requested and removed 🔍 Ready for review Someone needs to look into this. labels Dec 24, 2024
@actual-github-bot actual-github-bot bot added 🔍 Ready for review Someone needs to look into this. and removed ⚠️ Changes requested labels Dec 24, 2024

This comment has been minimized.

This comment has been minimized.

@lelemm
Copy link
Author

lelemm commented Dec 24, 2024

moved to experimental, accepted all suggestions, added banner, and some changes for github authentication.

please check it now

Copy link
Member

@youngcw youngcw left a comment

Choose a reason for hiding this comment

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

Its still a bit spartan, but it seems fine for now. We should have something in by the time of the release, so feel free to add to this before then. If not its approved and ready to merge at release.

@actual-github-bot actual-github-bot bot added ✅ Approved Used in conjunction with Merge at Release. This means that the PR has been approved but not merged. and removed 🔍 Ready for review Someone needs to look into this. labels Dec 24, 2024
@youngcw youngcw added the Merge at Software Release DO NOT MERGE unless the feature is released label Dec 24, 2024
@youngcw
Copy link
Member

youngcw commented Dec 24, 2024

Is the config file or env vars optional? I made a config file, but then the in app setup asked for the same information. It those extra config settings are optional, then maybe that should be made clear in the docs.

@lelemm
Copy link
Author

lelemm commented Dec 24, 2024

Is the config file or env vars optional? I made a config file, but then the in app setup asked for the same information. It those extra config settings are optional, then maybe that should be made clear in the docs.

They are optional. Or you config using the file plus command line or all using the ui

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge at Software Release DO NOT MERGE unless the feature is released ✅ Approved Used in conjunction with Merge at Release. This means that the PR has been approved but not merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants