Add native token based authentication

[Confectrician]

This will add/change several things:

• Replace our outdated request library usage (at least on client side for now) with axios
• Introduce a native auth header configuration so basic auth settings won't need to handle this

Fixes #88 Fixes #171 Fixes #194 Fixes #233

[Confectrician]

Hey @CWempe here is my announced ping for you. Also to cc @ghys Maybe you could do me a favor and try this version out too.

The extension now checks for the presence of a configured openHAB auth token. If there is one available this will overrule any existing basic auth setting. If there is none basic auth should be used, when configured. When none of the both is configured it should be tried to access the api wothout authentication. This way we should be fully compatible to openHAB 2 versions.

I have changed the configuration options heavily, so there should be tons of warnings and errors, when using this extension version. But i have tried to keep all deprecated settings for now. Vscode can provide a warning for those.

Here is how it should look in settings UI and editor:

VSIX can be downloarded here: https://artprodsu6weu.artifacts.visualstudio.com/A9a899564-e2b9-49e3-bd3d-4101a64d20ed/82e39b03-2e63-4a34-84ca-3cb57be32202/_apis/artifact/cGlwZWxpbmVhcnRpZmFjdDovL29wZW5oYWIvcHJvamVjdElkLzgyZTM5YjAzLTJlNjMtNGEzNC04NGNhLTNjYjU3YmUzMjIwMi9idWlsZElkLzIwNi9hcnRpZmFjdE5hbWUvb3BlbmhhYi12c2NvZGU1/content?format=file&subPath=%2Fopenhab-0.8.2-pr-247-19b5420.vsix

Basically i just want to make sure that the extension runs for the basic things like items/things explorer, hovering action and base lsp stuff.

[Confectrician]

I have also changed some parts of the Remote LSP Server. It uses now the same Output Channel the extension uses, to prevent some Confusion for the many places where something may be kogged.

[CWempe]

Finally found out to download the latest file. I was not sure your link contained the commit you did a few minutes later. 😉

This is my config (openHAB 2.4)

Feedback

This is what I got after installing the new version (v0.8.2-pr-247-3630e69) without doing any changes to my config:

It might be correct that not connection to the server is an error. But the user experience is not good if the user did nothing wrong and gets a "big fat red warning". all of a sudden just by updating.

Maybe a nice warning like "There have been changes to the openHAB addon. Please edit your setting." would help. Is it even correct that the update introduces breaking changes? Maybe we can support the old config a little longer for a smoother transition. But maybe the user base is not big enough to need this. 😄

After clicking "Show Output" in the error dialog I get this error:

Again, I think this error is to technical. We should tell the user how to fix it. Not only what is not working. ;)

You might have done this already with https://github.com/openhab/openhab-vscode/pull/247/commits/02ff8685433f8ad8646fb323db0e98cee54e60cd. But I do not see it before looking at my settings.json. 🤔

Why are there hashes in the warnings? The user might try to copy them.

After changing my config, I can use the hover features, which indicates that the connection is working, I guess. 👍

  "openhab.connection.host": "192.168.1.149",
  "openhab.connection.port": 8080,
  "openhab.connection.basicAuth.username": "",

[Confectrician]

Hm we could use both configuration entries parallel for now, but that would be a ton of extra code. I will think about it and how to solve it without having too look for this in a newer version. Maybe we could add a rewrite script, that migrates the config itself. I will see if that is possible.

Again, I think this error is to technical. We should tell the user how to fix it. Not only what is not working. ;)

This is mainly meant for copypasting it into the forum. The output is produced by the api calls, and i did not invest much time to get every possible error and provide a nice description. Anyway i have started a different approach for the config errors and i will maybe adapt that for the requests too later.

Why are there hashes in the warnings?

Those hashes are coming from the markdown description. If you are Using the ui (first screenshot in the comment above) they will produce a proper link to the next setting.

Thanks for the early look. I will try to get the old config parameters in place.

[CWempe]

I just noticed the "No quick fix available" message.

Is it possible to add a "Quick fix" that adapts the config to the new syntax? Just an idea... 😄

Again: I do not know how many users are using this addon. Maybe it is not worth to invest that much time in these features. The average openHAB user might be able to solve this issues without hassle by themself.

[Confectrician]

I made some thoughts yesterday and decided to cut out config management into its own class. For now this is just aimed to replace the current usage of how we get config values.

Maybe such a function can be added there too. Or to be more precise. The function should be possible anyways, but i am not sure if i can promote this as a "quick fix" to the vscode UI.

My idea was to prompt a warning and add a button to do this like we already have with Show Output for the error messages.

[Confectrician]

I will also do the same for api calls, so that we will have all api logic in one place for the future. But that's nothign for this PR. This PR should provide a working extension and a smooth transfer to our new config values.

[Confectrician]

OK @CWempe you can get the latest build, if you still know where to find it. 😛

It should work as following:

• Extension looks for the new config parameters (I had to remove some defaults unfortunately)
• Extension looks for deprecated config parameters if there are no new ones available currently.

Basically you should get a more or less working extension with this already without the need to configure anything.

One culprit:

I have left the default values for the LSP settings. So you may get LSP Errors/Warnings, even if you have disabled the remote LSP. But to be honest i am not willed to build a workaround for this too. Users may then check their config and see the warnings or have a look at the community. There we should have a proper FAQ in place when pushing the update.

[Confectrician]

Ah forgot:

I did not yet create the warning message. This will follow too of course.

[Confectrician]

There you go.

It will open up once every time you start up vscode if the extension recognizes usage of deprecated config parameters.

[ghys]

Also to cc @ghys Maybe you could do me a favor and try this version out too.

I used the new settings (host/API token) with the version provided above and the things & items appear properly.

But when I try to expand a group I get:

openHAB vscode extension has been activated
Could not reload items for Items Explorer
---
    Error:
        Error while connecting to openHAB REST API.

    Message:
        TypeError: Cannot read property 'map' of undefined
---
Could not reload items for Items Explorer

Try to debug in the developer tools but wasn't able to get a proper stacktrace.

[CWempe]

The warning appears after installing the new version (v0.8.2-pr-247-8a5af95) and reloading VScode as expected. Hovering in items works without changing the config.

Open config dialog works, but it would be better if it would directly open the openHAB settings if that is possible. Open config File (JSON) works, too. (File should be lower case, I guess.)

Looks good so far. 👍

[Confectrician]

if it would directly open the openHAB settings if that is possible.

Searched for this but did not find a solution yet.

@ghys

Indeed. It seems like wee did some kind of transformation with request promise that i have missed somehow. I have to check how this has been implemented in the past and do it here too.

[CWempe]

Just realized I have two other settings configured that seem to be deprecated.

I do not get a "deprecated" warning for these. They are just greyed out. I think they should be handled like the other config changes.

It appears the old value for karafCommand is read when I look in the config dialog.

But I do not find a setting to define sitemapPreviewUI for the new addon version.

To be honest: I do not know if I ever used the Karaf console in VScode. I would like to test it but even with the stable release I am not sure where to look.

It is the openHAB extension output terminal?

[CWempe]

Another thought: Should the new version be named 1.0.0?

https://semver.org/lang/de/

[Confectrician]

Yes this will be 1.0.0 That's the reason why i have included all api breaking changes here. :)

@ghys and @CWempe

Lastest version should fix the items tree view. Just a small thing i missed while migrating to axios.

[ghys]

> VSIX can be downloarded here: https://artprodsu6weu.artifacts.visualstudio.com/A9a899564-e2b9-49e3-bd3d-4101a64d20ed/82e39b03-2e63-4a34-84ca-3cb57be32202/_apis/artifact/cGlwZWxpbmVhcnRpZmFjdDovL29wZW5oYWIvcHJvamVjdElkLzgyZTM5YjAzLTJlNjMtNGEzNC04NGNhLTNjYjU3YmUzMjIwMi9idWlsZElkLzIwNi9hcnRpZmFjdE5hbWUvb3BlbmhhYi12c2NvZGU1/content?format=file&subPath=%2Fopenhab-0.8.2-pr-247-19b5420.vsix

Looks like it's the same for me.

Edit: never mind I figured out the Azure Pipelines artifacts.

Can confirm the item treeview is fixed, but I get the dialog above (https://github.com/openhab/openhab-vscode/pull/247#issuecomment-803620556) despite not having a deprecated parameter in settings. All I have is:

   "openhab.connection.authToken": "oh.vscode.SS4wBLhGG...",
   "openhab.connection.host": "192.168.1...."

Edit2: I get that:

openHAB vscode extension has been activated
Error: connect ECONNREFUSED 127.0.0.1:8080
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1141:16) {
  errno: 'ECONNREFUSED',
  code: 'ECONNREFUSED',
  syscall: 'connect',
  address: '127.0.0.1',
  port: 8080
}
Usage of deprecated config port detected.
Usage of deprecated config port detected.

I don't see a "port" anywhere in my settings.json. And I have a host configured so not sure why it connects to localhost.

[Confectrician]

I have seen that behavior too. I would guess that this is produced by the default settings of the deprecated config parameters. Not sure how i can exclude this properly.

I could remove the default for the deprecated port setting, but this will then break all users installations, where default port is used. (I would expect this to be a wide range of users.)

I could also supress the warning for port for now and/or provide a special check for the new port parameter and anly provide a warning when the new port parameter us null. (This would be the case for your setup too then.)

Edit:

Maybe it would indeed be better to provide a little migration script which updates the deprecated values with the values from their deprecated ancestor parameters.

[ghys]

I think you could implement the check for the "host" parameter only, since most users will probably have set it, and once they receive the warning they're likely to fix the rest of the config. (especially if they're prominently marked as deprecated).

Also I noticed the "Open config file (JSON)" button created a file in the current workspace in my case, which I didn't want and isn't ideal since my parameters are in the global settings.json file.

[CWempe]

Also I noticed the "Open config file (JSON)" button created a file in the current workspace in my case, which I didn't want and isn't ideal since my parameters are in the global settings.json file.

I am using a settings.json in my workspace and was surprised I was directed to this files, which was correct for me. But if the addon can only direct the user to one location of the file I guess the global settings.json should be used. But it would be best if you would be directed to the file where the openHAB config is defined of cause. 😉

[Confectrician]

This is really tricky. Currently there is no logic in place that tells the extension where a config value is coming from. (I am not sure if it is even possible to check this in a simple way. I think you can load different configuration scopes.)

I will have to do some research next week, how we can determine where the settings are from and save it to the correct location.

I have made a quick migration script meanwhile, which i will commit now. Feel free to test it. It will currently add all migrated configs to the workspace settings.

It will also recognize, when a username contains an auth token and will then use the new authTOken parameter instead of username.

[Confectrician]

We can of course remove that, if it won't fit our needs. I will try to think about a suiting solution for the different config scopes and how we can handle that with no maintenance in best case.

[Confectrician]

So....

Vscode can inspect single config values => https://code.visualstudio.com/api/references/vscode-api#WorkspaceConfiguration We could migrate the parameters and check each one for its current scope and store it in the same scope. This would be fairly easy if there is only one value set.

Here are the tricky ones:

• How about users that have global AND workspace settings with different values?
• What to do with the different places where i offer a "show config" button. => we will open the wrong location in about 50% of the cases

[Confectrician]

I made some thoughts and i think we should migrate a minimum config of host, port and username to authToken when one has set it this way.

This should solve most of the warnings in the majority of installations. I will provide a proper inspection for those values in the migration script and add them in all places where they have been set.

Also i will remove the "Open Config" Buttons for now, until i have a idea of how to open the "correct" configuration page properly.

[Confectrician]

So the migration script will now check where the old config value was stored (global or workspace) and add the new config into the same scope.

Or to be more precise: It will check if a global config exists and if the current value is identical/ is coming from there. I think this will fit into most use cases then.

[CWempe]

Just tried 0.8.2-pr-247-56f3210_215.

The dialog about the new config appeared and I clicked " Migrate now".

1. I expected the settings.json (or the settings GUI) to be opened automatically for the user to check the new parameters.
2. host and port were created with the new names. But username, karafCommand and sitemapPreviewUI were not migrated
3. Maybe we can disable or mark the old settings so the user gets remindet to delete the old parameters.
4. When reloading VScode with the new addon, I still get the warning. Even though the addon created two new settings.

5. Why do I get a second Starting config migration now.?

Usage of deprecated config username detected. openHAB vscode extension has been activated Starting config migration now. openhab.connection.host is already set, equals the old config or can't be migrated. openhab.connection.host is already set, equals the old config or can't be migrated. Starting config migration now.

6. After commenting/changing the old config I still get the warning:

When adding "openhab.connection.basicAuth.username": "" the warning disappears and I get item states when hovering. But I still get an error in th output:

openHAB vscode extension has been activated Error: getaddrinfo ENOTFOUND openhabianpi at GetAddrInfoReqWrap.onlookup [as oncomplete] (dns.js:66:26) { errno: 'ENOTFOUND', code: 'ENOTFOUND', syscall: 'getaddrinfo', hostname: 'openhabianpi' }

[Confectrician]

I expected the settings.json (or the settings GUI) to be opened automatically for the user to check the new parameters.

Maybe we can disable or mark the old settings so the user gets remindet to delete the old parameters.

I am not able to decide, which UI should be opened in some cases. For example when one hase settings defined in different scopes. Should we provide a "post-migration-message" with the recommendation to check the settings and remove the migrated ones?

host and port were created with the new names. But username, karafCommand and sitemapPreviewUI were not migrated

That's on purpose. See: https://github.com/openhab/openhab-vscode/pull/247#issuecomment-804266929

When reloading VScode with the new addon, I still get the warning. Even though the addon created two new settings.

This will appear until no old settings are found anymore. Maybe we should clarify the message for this and remind of deleting and changing the old parameters?

Why do I get a second Starting config migration now.?

After commenting/changing the old config I still get the warning:

I have to check those behaviors. For now it looks like you have/had set a username parameter (mabye parallel in a different scope)?!

When adding "openhab.connection.basicAuth.username": "" the warning disappears and I get item states when hovering. But I still get an error in th output:

This is a LSP Server Warning (i have just merged all output channels and it is shown within the extension now.) If you had remote LSP disabled before you will have to do this again. The default is true and overwritten by the new setting.

Should we check for a different setting than the default value and ask the user if he wants to disable it again?

[Confectrician]

Another thing @CWempe We have now 27 comments and are talking mainly about a migration path and most related to migration options.

Maybe we should make a cut here and avoid blowing up this PR endless. I would cur out the migration script for now and add this to a different PR because i would also like to make a migration page, which shows up after doing the 1.0.0 update in vscode.

But that will be another big change in the codebase.

So are you and maybe @ghys fine with this? I would say the main parts of this PR (config changes and token based auth) are working fine already.

[CWempe]

I would cur out the migration script for now and add this to a different PR because i would also like to make a migration page, which shows up after doing the 1.0.0 update in vscode.

Like I said in https://github.com/openhab/openhab-vscode/pull/247#issuecomment-803493910: Maybe we do not even need all this migration process. You already spent more time on this than I expected.

a migration page, which shows up after doing the 1.0.0 update in vscode.

I like this idea. It may be all we need to tell the users to change their configuration. After all it is just a couple of simple parameter names that need to be changed.

This is a LSP Server Warning (i have just merged all output channels and it is shown within the extension now.) If you had remote LSP disabled before you will have to do this again. The default is true and overwritten by the new setting.

Should we check for a different setting than the default value and ask the user if he wants to disable it again?

To be honest, I have no idea what you are talking about. 😅 I do not think I disabled any remote LSP.

This is all I had configured for the addon (in all scopes I think):

  "openhab.host": "192.168.1.149",
  "openhab.port": 8080,
  "openhab.username": "",
  "openhab.karafCommand": "ssh openhab@%openhabhost% -p 8101",
  "openhab.sitemapPreviewUI": "basicui",

[Confectrician]

Like I said in #247 (comment): Maybe we do not even need all this migration process. You already spent more time on this than I expected.

Yep, but that's because i thought it was worth it. We have a proper Configuration management logic now, which will bring benefits in the future. (At least that's what i hope from a code perspective)

Also the Migration Logic is something i think is worth the effort. There is a high change we will have cases like this in the future too. It is just out of scope here, that's why i asked. I would like to avoid breaking token auth things now, because of the migration helpers.

So i will try to cut out the migration stuff here and store it in another branch. THis way we get a clean codebase for this PR.

To be honest, I have no idea what you are talking about. 😅 I do not think I disabled any remote LSP.

Then you are running on default settings. (especially if you heard about this setting the first time now.) What is strange here is the fact, that somewhere the default configuration (openhabianpi) is used instead of the new parameter (192.168.... like you have configured in openhab.connection.host). Seems i forgot to change this somewhere.

[ghys]

If you're planning a major release I'd skip the obsolete config check and migration too. Deprecated config parameters are already clearly marked as such in the config UI and settings.json editor, the modal dialog which comes up sometimes even if your config is valid is rather annoying, and it's still not clear what the buttons should lead to. Just mention it in a change log/release notes and be done with it.

[Confectrician]

the modal dialog which comes up sometimes even if your config is valid is rather annoying

I have solved that in the last pr. It was showing those messages because they have a default value. I am now checking if there really is a manual configured setting somewhere besides the default.

I will do this in another PR with some minimal setup.

see #249 for the thoughts i made about it.

[Confectrician]

OK i think this is it for now.

I have readded karafCommand and SitemapPreview based on https://github.com/openhab/openhab-vscode/pull/247#issuecomment-803626802 to show a deprecation message and also include them in my checks.

I will merge, if the build is fine and then work on a proper migration/release notes page that will be included in vscode.

[ghys]

Heads up, I just checked and there's a new Authentication Provider API in VS Code that has just been promoted to stable last month; not sure what it can do right now but if it ends up easing the OAuth flows, then that could be very interesting: you could have a button to authorize VS Code to your openHAB instance, let the user authenticate on the standard authorize page, open a session, receive a refresh token and use that to get access tokens - the regular OAuth2 flow that the UI does. No need to have the user manually generate a token then.

[Confectrician]

I already have an eye on that, but it currently is only capable of github and microsoft as authentication providers. But chances are good that it can handle a proper oauth for custom providers too at some point. :)

https://code.visualstudio.com/api/references/vscode-api#authentication

[Confectrician]

Finally thanks to all of your feedback. I think it was pretty important to talk about this, because of the huge amount of changes and i think we improved my initial thoughts very well. :)

I will catch up on you for the beta then.

[Confectrician]

Migration script example code stored temporary if i want to use it again at some point.

    const logPrefix = `openHAB Extension: `
    let currentConfig = ConfigManager.getInstance().currentConfig
    let currentParameter = OH_CONFIG_PARAMETERS.connection.host

    const updatedMessage = `Updated openhab.${currentParameter}.`
    const checkParamMessage = `Check if openhab.${currentParameter} can be migrated`
    const migrationPossibleMessage = `openhab.${currentParameter} can be migrated safely.`
    const alreadySetMessage = `openhab.${currentParameter} is already set, equals the old config or can't be migrated.`
    const migrationStartMessage = `Starting config migration now.`
    const migrationFinishedMessage = `Starting config migration now.`

    console.info(logPrefix + migrationStartMessage)
    utils.appendToOutput(migrationStartMessage)

    let hostConfig = currentConfig.get(currentParameter)
    let hostConfigDeprecated = currentConfig.get('host')

    console.info(logPrefix + checkParamMessage)
    if(!hostConfig && hostConfigDeprecated != null) {
        console.info(logPrefix + migrationPossibleMessage)
        let depConfigInspectResult = currentConfig.inspect('host')

        ConfigManager.update(
            currentParameter,
            hostConfigDeprecated,
            (depConfigInspectResult.globalValue == hostConfigDeprecated) ?
                vscode.ConfigurationTarget.Global :
                vscode.ConfigurationTarget.Workspace
        )
        utils.appendToOutput(updatedMessage)
    }
    else {
        console.info(logPrefix + alreadySetMessage)
        utils.appendToOutput(alreadySetMessage)
    }

    currentParameter = OH_CONFIG_PARAMETERS.connection.port
    let portConfig = currentConfig.get(currentParameter)
    let portConfigDeprecated = currentConfig.get('port')

    console.info(logPrefix + checkParamMessage)
    if(!portConfig && portConfigDeprecated != null){
        console.info(logPrefix + migrationPossibleMessage)
        let depConfigInspectResult = currentConfig.inspect('port')

        ConfigManager.update(
            currentParameter,
            portConfigDeprecated,
            (depConfigInspectResult.globalValue == portConfigDeprecated) ?
                vscode.ConfigurationTarget.Global :
                vscode.ConfigurationTarget.Workspace
        )
        utils.appendToOutput(updatedMessage)
    }
    else{
        console.info(logPrefix + alreadySetMessage)
        utils.appendToOutput(alreadySetMessage)
    }

    currentParameter = OH_CONFIG_PARAMETERS.connection.authToken
    let authTokenConfig = currentConfig.get(currentParameter)
    let usernameConfigDeprecated = currentConfig.get('username') as string

    console.info(logPrefix + checkParamMessage)
    if(!authTokenConfig && usernameConfigDeprecated != null){
        console.info(logPrefix + `Checking if username setting exists and has been used as auth token`)

        // Check if given username is a openHAB 3 token
        let usernameSegments = usernameConfigDeprecated.split('.')
        if(usernameSegments.length === 3 && usernameSegments[0] === 'oh'){
            console.info(logPrefix + `Detected auth token in username setting. Using it for openhab.${currentParameter} now`)
            let depConfigInspectResult = currentConfig.inspect('username')

            ConfigManager.update(
                OH_CONFIG_PARAMETERS.connection.authToken,
                usernameConfigDeprecated,
                (depConfigInspectResult.globalValue == usernameConfigDeprecated) ?
                    vscode.ConfigurationTarget.Global :
                    vscode.ConfigurationTarget.Workspace
            )
            utils.appendToOutput(updatedMessage)
        }
    }
    else{
        console.info(logPrefix + alreadySetMessage)
        utils.appendToOutput(alreadySetMessage)
    }

    console.info(logPrefix + migrationFinishedMessage)
    utils.appendToOutput(migrationFinishedMessage)
    }