-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add deauthentication #2
Conversation
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.
LGTM, just a couple/few comments that I think require action on providers
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.
Just one comment about the util fn, otherwise LGTM!!!
api/app/src/routes/util.ts
Outdated
export const getProviderMapFromConnectUserOrFail = (connectedUser: ConnectedUser, provider: ProviderOptions) => { | ||
if (!connectedUser.providerMap) throw new UnauthorizedError(); | ||
const providerData = connectedUser.providerMap[provider]; | ||
if (!providerData) throw new UnauthorizedError(); | ||
|
||
return providerData; | ||
} |
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.
Good job identifying and reusing code! 🙌
I just suggest we keep these util functions to a minimal - usually not domain-related, otherwise we can quickly have a ton of them; as well, exposing the ConnectedUser's internals adds coupling across the app.
In this case, since we need this behavior across multiple files, maybe move this function to be a command related to ConnectedUser? It could throw NotFoundError
and we have the route rethrowing it as an UnauthorizedError
.
Also, the function returns a Provider's data, not the map, so maybe rename to getProviderDataOrFail()
or something similar?
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.
Small detail 🕵️
api/app/src/providers/oauth2.ts
Outdated
@@ -16,6 +16,7 @@ export const oauthUserTokenResponse = z.object({ | |||
|
|||
export interface OAuth2 { | |||
getAuthUri(state: string): Promise<string>; | |||
revokeProviderAccess(connectedUser: ConnectedUser): Promise<void | string>; |
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.
Curious, why returning void | string
? It seems we don't need to return string
(maybe just add a await
on Withings':
if (status === 0) {
await this.oauth.revokeLocal(connectedUser);
return;
}
Ideally we wouldn't adjust the interface's contract bc of a specific implementation.
api/app/src/providers/withings.ts
Outdated
@@ -100,7 +100,7 @@ export class Withings extends Provider implements OAuth2 { | |||
const status = await this.revokeToken(client_id, client_secret, timestamp, nonce, providerData.token) | |||
|
|||
if (status === 0) { | |||
return this.oauth.revokeLocal(connectedUser); | |||
this.oauth.revokeLocal(connectedUser); |
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.
await?
api/app/src/providers/oura.ts
Outdated
async revokeProviderAccess(connectedUser: ConnectedUser): Promise<string> { | ||
return this.oauth.revokeLocal(connectedUser); | ||
async revokeProviderAccess(connectedUser: ConnectedUser): Promise<void> { | ||
this.oauth.revokeLocal(connectedUser); |
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.
why the removed return?
api/app/src/providers/whoop.ts
Outdated
async revokeProviderAccess(connectedUser: ConnectedUser) { | ||
return this.oauth.revokeLocal(connectedUser); | ||
async revokeProviderAccess(connectedUser: ConnectedUser): Promise<void> { | ||
this.oauth.revokeLocal(connectedUser); |
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.
ditto
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.
🚢 🚢 🚢 🚢 🚢 🚢
Patch Inbound DQ Response #2
RELEASE 2024-03-06 #2
RELEASE 2024_05_17 #2
RELEASE 2024_05_30 #2
No description provided.