Skip to content

local oauth fixes#3

Open
NeonDaniel wants to merge 2 commits intodevfrom
FIX_OauthFixes
Open

local oauth fixes#3
NeonDaniel wants to merge 2 commits intodevfrom
FIX_OauthFixes

Conversation

@NeonDaniel
Copy link
Copy Markdown

Fix oauth_callback write of token to local storate
Update local webserver to run on 0.0.0.0 by default Add flask to requirements.txt

Update local webserver to run on 0.0.0.0 by default
Add flask to requirements.txt
@NeonDaniel NeonDaniel requested a review from JarbasAl December 2, 2022 19:44
Comment thread ovos_config_assistant/app.py Outdated
Comment thread ovos_config_assistant/app.py Outdated
Comment thread ovos_config_assistant/app.py Outdated
@JarbasAl
Copy link
Copy Markdown
Contributor

JarbasAl commented Dec 4, 2022

the source of this issue is that this was not completely ported over https://github.com/OpenVoiceOS/ovos-personal-backend/blob/d4e3247dee2fa730e3511c1e84faa956ce7846ed/ovos_local_backend/database/oauth.py#L1

perhaps we should just move over the whole class instead? these 2 repos are very similar in many aspects, we could look into sharing all this code better, maybe make OCA a dependency of backend-manager or move this to backend-client instead?

thoughts?

@JarbasAl
Copy link
Copy Markdown
Contributor

JarbasAl commented Dec 5, 2022

both calls to the JsonStorage objects should be replaced with OpenVoiceOS/ovos-backend-client#24 instead, these dbs are shared across some projects and this ensures we can keep things compatible more easily

internal projects sharing this db are:

  • personal backend
  • OCA
  • backend manager
  • oauth plugin

@NeonDaniel
Copy link
Copy Markdown
Author

the source of this issue is that this was not completely ported over https://github.com/OpenVoiceOS/ovos-personal-backend/blob/d4e3247dee2fa730e3511c1e84faa956ce7846ed/ovos_local_backend/database/oauth.py#L1

perhaps we should just move over the whole class instead? these 2 repos are very similar in many aspects, we could look into sharing all this code better, maybe make OCA a dependency of backend-manager or move this to backend-client instead?

thoughts?

Maybe it would make sense for the config utils in OBC to move to ovos_config? I'm not sure where the web server makes the most sense, but we definitely should have it in one place where its imported..

@JarbasAl
Copy link
Copy Markdown
Contributor

JarbasAl commented Dec 5, 2022

the source of this issue is that this was not completely ported over https://github.com/OpenVoiceOS/ovos-personal-backend/blob/d4e3247dee2fa730e3511c1e84faa956ce7846ed/ovos_local_backend/database/oauth.py#L1
perhaps we should just move over the whole class instead? these 2 repos are very similar in many aspects, we could look into sharing all this code better, maybe make OCA a dependency of backend-manager or move this to backend-client instead?
thoughts?

Maybe it would make sense for the config utils in OBC to move to ovos_config? I'm not sure where the web server makes the most sense, but we definitely should have it in one place where its imported..

these are not config utils, these are local databases. they originate in personal backend because thats where it was implemented first. then also used by backend manager which manipulates the dbs directly. OCA is basically a fork of backend-manager which again shares the database. finally core itself will use this db if not using a backend, this is done via backend-client and is where the source of truth originates, so it makes sense to move these shared helper databases from personal backend repo to the client repo

the new PHAL Oauth plugin also uses these databases, it is an abstraction layer to provide oauth even for headless devices

@NeonDaniel
Copy link
Copy Markdown
Author

these are not config utils, these are local databases. they originate in personal backend because thats where it was implemented first. then also used by backend manager which manipulates the dbs directly. OCA is basically a fork of backend-manager which again shares the database. finally core itself will use this db if not using a backend, this is done via backend-client and is where the source of truth originates, so it makes sense to move these shared helper databases from personal backend repo to the client repo

the new PHAL Oauth plugin also uses these databases, it is an abstraction layer to provide oauth even for headless devices

I don't think any of the oauth should depend on a backend, but if its tied to code in the backend client then I guess it makes sense to go there. I'm still not entirely sure what OCA should be handling; I thought it was just for accessing/troubleshooting configuration, but I think that all makes more sense in ovos-config now that config handling is moved to that module

@JarbasAl
Copy link
Copy Markdown
Contributor

JarbasAl commented Dec 5, 2022

these are not config utils, these are local databases. they originate in personal backend because thats where it was implemented first. then also used by backend manager which manipulates the dbs directly. OCA is basically a fork of backend-manager which again shares the database. finally core itself will use this db if not using a backend, this is done via backend-client and is where the source of truth originates, so it makes sense to move these shared helper databases from personal backend repo to the client repo
the new PHAL Oauth plugin also uses these databases, it is an abstraction layer to provide oauth even for headless devices

I don't think any of the oauth should depend on a backend, but if its tied to code in the backend client then I guess it makes sense to go there. I'm still not entirely sure what OCA should be handling; I thought it was just for accessing/troubleshooting configuration, but I think that all makes more sense in ovos-config now that config handling is moved to that module

OCA is a standalone external tool, a UI for headless devices basically, it should never be a dependency. ovos-config is an internal lib but oauth is unrelated to config...

OAuth explicitly depends on a backend, as the selected backend is a oauth token store, if a user wants to enter the tokens in selene thats possible (and only mycroft official way)

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.

2 participants