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

Attempt at adding header auth. Ignore Settings #981 #1390

Conversation

toddejohnson
Copy link
Contributor

@toddejohnson toddejohnson commented Nov 17, 2023

toddejohnson Medium toddejohnson /REFACTOR/2.1.2_unified-config → Lissy93/dashy Commits: 4 | Files Changed: 9 | Additions: 214 Label Unchecked Tasks Powered by Pull Request Badge

Category: Feature... and hopefully some documentation to go with it.

Overview

TLDR: Attempt to implement #981 Add header authentication.

I needed to get req.headers['Remote-User'] from the server side header to authenticate my user so I didn't need another password. I think this needed a services/get-user.js to make that happen although I've never worked with vuejs, etc.

I found your 2.1.2 and it had a good start at the config race condition in main.js that I was running into with implementing headerAuth. I will say I'm not that good at async coding so there might be some big issues. I have it working for my use case which isn't what a maintainer likes to hear.

  1. It is currently hard coded for my REMOTE-USER mostly because I don't know how to access getAppConfig().headerAuth.userHeader in the nodejs services/get-user.js which is one of the few things I need to make this configurable.
  2. There is no documentation on how to use this. I'm using Authelia but this would work for Apache/NGINX basic/ldap auth too. Even Apache and mod_auth_mellon(SAML) would allow adding a username header.
  3. I'm using the built in users 'database' to get the hash and fake like a normal user instead of the bolt on that is Keycloak with it's own auth level.

This is my first js project in years so I'm open to direction and LMGTFY feedback.

Issue Number #981

New Vars

appConfig:
  auth:
    enableHeaderAuth: true
    headerAuth:
      userHeader: Remote-User
      proxyWhitelist:
        - 127.0.0.1
        - '::1'

Code Quality Checklist (Please complete)

  • All changes are backwards compatible
  • All lint checks and tests are passing
  • There are no (new) build warnings or errors
  • (If a new config option is added) Attribute is outlined in the schema and documented
  • (If a new dependency is added) Package is essential, and has been checked out for security or performance
  • Bumps version, if new feature added

Copy link

vercel bot commented Nov 17, 2023

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

Name Status Preview Comments Updated (UTC)
dashy ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 24, 2023 6:15am

Copy link

viezly bot commented Nov 17, 2023

Changes preview:

Legend:

👀 Review pull request on Viezly

@toddejohnson
Copy link
Contributor Author

I have no clue if you'd prefer to pull in the config globally server side or parse it per request? This is quickly creeping in scope. I just have documentation and I think this will be ready.

Copy link
Owner

@Lissy93 Lissy93 left a comment

Choose a reason for hiding this comment

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

Looks awesome, I'll test it out properly either this evening or tomorrow evening
Once reviewed, is it ready for merge on your side?

@toddejohnson
Copy link
Contributor Author

Adding documentation of how to configure this is the last task I have then. Yay!!!

@toddejohnson
Copy link
Contributor Author

I was starting to write the documentation and realized I'm still missing the proxyWhitelist implementation. That is an important feature but your web server/reverse proxy should be securing that header for you too.

@toddejohnson toddejohnson changed the title WIP: Attempt at adding header auth. Ignore Settings #981 Attempt at adding header auth. Ignore Settings #981 Nov 25, 2023
.catch(() => window.location.reload());
}
store.dispatch(Keys.INITIALIZE_CONFIG).then((thing) => {
console.log('main', thing);
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe don't need this here 😉

Copy link
Owner

Choose a reason for hiding this comment

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

Oh just realized it was me who left that there ! 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Likely slept since then :-)

I can remove the comment and push again. It was helpful with the race condition.

@toddejohnson
Copy link
Contributor Author

Anything I can do to help get this ready?

@Lissy93 Lissy93 merged commit 2d350ae into Lissy93:REFACTOR/2.1.2_unified-config Dec 17, 2023
7 of 8 checks passed
@dschmidt
Copy link

afaict :latest docker images point to the latest release and not to main branch builds - when is this going to be released and going to be available from a docker image? :)

@Lissy93
Copy link
Owner

Lissy93 commented Dec 24, 2023

@dschmidt - I'll get a new release pushed out within the next week, I just need to test everything to avoid anything breaking.

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.

3 participants