-
Notifications
You must be signed in to change notification settings - Fork 4
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
Companion-API oauth register/update user #7
base: master
Are you sure you want to change the base?
Conversation
pkg/companion-api/api/oauth.go
Outdated
func setup() { | ||
setupOauthOnce.Do(func() { | ||
oauthConfig = &oauth2.Config{ | ||
RedirectURL: requireEnv("OAUTH_MYAPP_ENDPOINT", "This app endpoint") + "/oauth/callback", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OAUTH_APP_ENDPOINT
sounds better to me
pkg/companion-api/backend/client.go
Outdated
@@ -7,6 +7,7 @@ import ( | |||
// UserData is a simple user struct with paswordhash and claims | |||
type UserData struct { | |||
PasswordHash string `json:"password"` | |||
OauthToken string `json:"oauth_token"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expected multiple oauth identities bound to a single account here (ie: 1 google, 1 github, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. It means that for each client account (ie: 1 google, 1 github, etc.), 1 companion-api will have to be deployed. Or it is also necessary that the companion-api can manage the "n-account" with several CLIENTID, ... etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say we want something like <prefix>/oauth/<provider>/<client-id>
pointing to the associated user account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what I mainly meant was to use this path as an etcd storage key, ie associating user foo
with clientid bar
in provider baz
is kv.Put("oauth/baz/bar", "foo")
(and the symmetric Get
to check a clientid knowning the source provider to get the user id to query).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the multiprovider support is also required and LGTM ;)
…associated user account.
second (better) try 😄 |
feat(oauth): association userID with clientID (clean) feat(oauth): association userID with clientID (clean)
No description provided.