Migrate existing OpenID Connect strategy to openid-client.

[mstrhakr]

This should be ready to look at now. I've squashed most of the bugs and cleaned up my mistakes, even wrote some documentation I just need someone else to give it a look. As I normally do, I've made a docker image available to anyone interested in testing these features themselves.

Docker Image :mstrhakr/meshcentral:dev
Build Command: docker build -f docker/Dockerfile --force-rm --build-arg DISABLE_MINIFY=yes --build-arg DISABLE_TRANSLATE=yes -t mstrhakr/meshcentral:dev .

Documentation

Overview

• Introduction
• Definitions
• Terms

Configuration

• Basic Config

Advanced Config

• Issuer
• Client
• Custom
• Groups

Presets Config

• Azure
• Google

Old Configs

• Basic
• Advanced
• Presets
• Upgrading to this version

original post below

This is working, but I have not done enough testing to say it is merge ready, I can all but guarantee there will be more commits.

Not to mention until @Ylianst returns I assume this can't/shouldn't be merged. I'm only putting it here so anyone who is interested can inspect this for issues or comment on how things should be setup.

I passed through all of the potential options for openid-client's Client and Issuer classes through to the config file so the schema for oidc has grown by an order of magnitude. I figured we give the power to the people.

I've also set it up so existing configs will be parsed correctly, although the new config style is preferred. (Now that I mention it, I should rearrange things so the new config options override the old ones if both are set.)

[lgtm-com]

This pull request introduces 1 alert when merging 6c5cac7 into c864662 - view on LGTM.com

new alerts:

• 1 for Duplicate property

[mstrhakr]

This is a major upgrade to SSO on MeshCentral that will allow group support for any previously OAuth2.0 IdP's that support it among other things. Hopefully the way I went about it is okay. Now I just need to do a bunch of testing with azure, google, and all the other preset options.
This is still a work in progress but I've made some great progress.

Docker: mstrhakr/meshcentral:dev

[Ylianst]

Oh dear. Indeed, a quick look at this, it's clear this would cause all existing servers setup with these authentication techniques to stop working when updating. What I would like if to keep the existing code but create a new config.json key for this feature.

[mstrhakr]

Oh dear.

Yea.. This isn't even close to ready yet. My goal is to have this be ready for review shortly before you return. I wouldn't look too close in the meantime.

Indeed, a quick look at this, it's clear this would cause all existing servers setup with these authentication techniques to stop working when updating.

I have Azure working now, but I still need to go through the rest of the strategies and test them, but the worst is behind me at this point.
EDIT2: I plan on testing all the strategies including making sure upgrading works as expected. I don't want to push any more bugs to production lol.

What I would like is to keep the existing code but create a new config.json key for this feature

I've got it setup in such a way that all (only works for oidc and azure so far) the old configs are imported and setup automatically.

I even figured out how to update the userid for azure users by cloning the user, remaking the db entry, then deleting the original user. I'm still working out what issues this could cause but I'm not there yet.

I really appreciate you taking time from your vacation to review this, but again, I don't consider this ready for review quite yet. Once I'm done, you should be able to merge this without worrying about issues from an update.

EDIT: Even if none of this ends up being merged down the road, I've learned a ton working on this project, and for me, that is reason enough to continue.

[lgtm-com]

This pull request introduces 2 alerts when merging 22b97b6 into 41e67b0 - view on LGTM.com

new alerts:

• 1 for Implicit operand conversion
• 1 for Variable not declared before use

[mstrhakr]

What I would like if to keep the existing code but create a new config.json key for this feature.

I removed all the changes I made to the other strategies. Hopefully this looks okay, I tested it with a bunch of different combinations of configs. The only untested feature at the moment is groups for the azure and google presets, they worked once but I haven't fully tested that again since all these changes.

[lgtm-com]

This pull request introduces 3 alerts when merging a1edd11 into 7c31245 - view on LGTM.com

new alerts:

• 1 for Unreachable statement
• 1 for Expression has no effect
• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request introduces 2 alerts when merging 0cc45ab into 7c31245 - view on LGTM.com

new alerts:

• 1 for Expression has no effect
• 1 for Unused variable, import, function or class

[lgtm-com]

This pull request fixes 1 alert when merging 8165d03 into 4294e55 - view on LGTM.com

fixed alerts:

• 1 for Useless assignment to local variable

[lgtm-com]

This pull request fixes 1 alert when merging fb6f72b into 4294e55 - view on LGTM.com

fixed alerts:

• 1 for Useless assignment to local variable

[lgtm-com]

This pull request fixes 1 alert when merging 5791e5c into 4294e55 - view on LGTM.com

fixed alerts:

• 1 for Useless assignment to local variable

[lgtm-com]

This pull request fixes 1 alert when merging 7ac69c8 into 4294e55 - view on LGTM.com

fixed alerts:

• 1 for Useless assignment to local variable

[lgtm-com]

This pull request fixes 1 alert when merging 80aff4f into 6c34978 - view on LGTM.com

fixed alerts:

• 1 for Useless assignment to local variable

[lgtm-com]

This pull request fixes 1 alert when merging dfcdb79 into 6c34978 - view on LGTM.com

fixed alerts:

• 1 for Useless assignment to local variable

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging 51d6f4f into 6c34978 - view on LGTM.com

new alerts:

• 1 for Ignoring result from pure array method

fixed alerts:

• 1 for Useless assignment to local variable

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging c4f69c5 into 6c34978 - view on LGTM.com

new alerts:

• 1 for Ignoring result from pure array method

fixed alerts:

• 1 for Useless assignment to local variable

[lgtm-com]

This pull request fixes 1 alert when merging c3cfa9d into 6c34978 - view on LGTM.com

fixed alerts:

• 1 for Useless assignment to local variable

[mstrhakr]

@Ylianst This is ready for you to look at whenever. If you want any work done before merging let me know. I'm more than happy to clean up my own messes.

[lgtm-com]

This pull request fixes 1 alert when merging 6a7c0b7 into 8f84fa3 - view on LGTM.com

fixed alerts:

• 1 for Useless assignment to local variable

[lgtm-com]

This pull request introduces 1 alert and fixes 59 when merging 9887b42 into 99fc690 - view on LGTM.com

new alerts:

• 1 for Syntax error

fixed alerts:

• 37 for Unused variable, import, function or class
• 5 for Useless assignment to local variable
• 5 for Superfluous trailing arguments
• 5 for Prototype-polluting assignment
• 3 for Comparison between inconvertible types
• 1 for Inconsistent use of 'new'
• 1 for Variable not declared before use
• 1 for Unneeded defensive code
• 1 for Use of password hash with insufficient computational effort

[Ylianst]

I will close this one since it's older and somewhat risky to add. If you really want this, open a new PR and I will look at it again. My worry with this one is that it will break existing installations.