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

Alpha1 Preparations #140

Merged
merged 10 commits into from
Jul 2, 2024
Merged

Alpha1 Preparations #140

merged 10 commits into from
Jul 2, 2024

Conversation

Apollon77
Copy link
Owner

@Apollon77 Apollon77 commented Jul 2, 2024

  • GHA adjustments
  • fixing wrong export
  • make answer template configurable
  • add redirect to local server and use as startpoint to get certificate accepted at the beginning and not failing at the end
  • make cert dir configurable
  • add a way to receive the code by "other means" in a custom way without opening the server by the client. I need this for ioBroker because here we can use an already existing server

I decided to not return the Daikin URL as "Auth start URL" but the one from the local server that will also handle the response. Because of the self signed process it is better to let the user accept the certificate directly before he does it and not failing at the end because wondering :-)
So I added that the redirect happens there

* make answer template configurable
* add redirect to local server and use as startpoint to get certificate accepted at the beginning and not failing at the end
* make cert dir configurable
@Apollon77
Copy link
Owner Author

@jacoscaz @JeroenVdb WDYT?

.. to allow cases where e.g. a webserver is already opened or such
@JeroenVdb
Copy link
Contributor

Looks good!

@JeroenVdb
Copy link
Contributor

@Apollon77 what is actually the difference with oidc_callback_server_baseurl and oidc_callback_server_addr? Should the ip/domain and ports not be the same for both?

@Apollon77
Copy link
Owner Author

Apollon77 commented Jul 2, 2024

@JeroenVdb The oidc_callback_server_addr is the "local binding address" for the server and so kind of also the network interface to use or such. The oidc_callback_server_baseurl is the complete "https url how this server is reachable".

So e.g. also for docker users you migt bind to 0.0.0.0 which is in the docker container. Then you need to forward the port and your docker then has an external IP/name (if not host mode is used). so the oidc_callback_server_baseurl always need to be https://ip-or-name:port where the server is "reachable from your computer where the browser is used for the authentication".

We could rename the oidc_callback_server_addr to include "bind" to make it more clear and we could have oidc_callback_server_baseurl to include" external" or such?

We could also allow oidc_callback_server_baseurl to bne empty when we start the server and we automatically create it as https://${oidc_callback_server_addr}:${oidc_callback_server_port} for these cases that would be possible too

Opinions?

@jacoscaz
Copy link
Collaborator

jacoscaz commented Jul 2, 2024

General comments before I look at the code:

I decided to not return the Daikin URL as "Auth start URL" but the one from the local server that will also handle the response. Because of the self signed process it is better to let the user accept the certificate directly before he does it and not failing at the end because wondering :-)

This is some excellent user-oriented thinking! +1!

We could rename the oidc_callback_server_addr to include "bind" to make it more clear and we could have oidc_callback_server_baseurl to include" external" or such?

+1 to renaming to oidc_callback_server_bind_addr.

We could also allow oidc_callback_server_baseurl to bne empty when we start the server and we automatically create it as https://${oidc_callback_server_addr}:${oidc_callback_server_port} for these cases that would be possible too

+1 to defaulting to the concatenation if oidc_callback_server_baseurl is null or undefined.

Copy link
Collaborator

@jacoscaz jacoscaz left a comment

Choose a reason for hiding this comment

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

Just nitpicks - LGTM!

src/onecta/oidc-utils.ts Outdated Show resolved Hide resolved
src/example.ts Outdated Show resolved Hide resolved
@jacoscaz
Copy link
Collaborator

jacoscaz commented Jul 2, 2024

Also, whilst this change wasn't introduced in this PR, I think having the prepare script in package.json run through the build goes against normal assumptions (npm install is not expected to trigger a build). prepublish would be a better place.

@Apollon77
Copy link
Owner Author

@jacoscaz One other question: Would you be ok if I change the naming convention for the config object to camel case as "more natural" in Nodejs /JS environments? Would be ideal place to do that now directly before breaking later

@jacoscaz
Copy link
Collaborator

jacoscaz commented Jul 2, 2024

@Apollon77 absolutely, go ahead. I tend to use snake_case for variables in all of my (recent) projects but I'm aware that it goes against the current.

@Apollon77
Copy link
Owner Author

I will adjust this later and release a first alpha likely tonigth

* allows to specify external address (ip/domain) and then redirect url is build automatically
* allows to specify initial tokenSet in config - then no file is used
* emits event on token update
* splits cert path in two properties
* camelCase the config
@Apollon77
Copy link
Owner Author

Ok pushed the comments and some more stuff ... @jacoscaz have a look if you like ... now camel cased it and using real JS "private" with #name

@Apollon77 Apollon77 merged commit c086193 into main Jul 2, 2024
29 checks passed
@Apollon77
Copy link
Owner Author

Ok, released as @next on npm ... or ... 2.0.0-alpha.1

https://www.npmjs.com/package/daikin-controller-cloud?activeTab=versions

@Apollon77
Copy link
Owner Author

I have found two issues with busing it and will tweak something tomorrow morning first thing and publish alpha 2

@jacoscaz
Copy link
Collaborator

jacoscaz commented Jul 3, 2024

Tested alpha.2, everything looks good!

@Apollon77
Copy link
Owner Author

@jacoscaz alpha 3 is also coming ... will add a method on the main class to update all devices and adds update event to the device class to save requests with these hard rate limits here.
If you like contact me in DIscord (Apollon77) and we can have more direct communication there :-)

JeroenVdb added a commit to JeroenVdb/daikin-controller-cloud that referenced this pull request Jul 4, 2024
* origin/main:
  chore: release v2.0.0-alpha.1
  gha
  Changelog and license year
  Alpha1 Preparations (Apollon77#140)
  Bump braces from 3.0.2 to 3.0.3 (Apollon77#135)
  Bump actions/checkout from 3 to 4 (Apollon77#119)
  Bump actions/setup-node from 3 to 4 (Apollon77#121)
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