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

Manual OAuth 2.0 #16

Merged
merged 13 commits into from
Jan 11, 2024
Merged

Manual OAuth 2.0 #16

merged 13 commits into from
Jan 11, 2024

Conversation

ryanwi
Copy link
Collaborator

@ryanwi ryanwi commented Jan 10, 2024

Implement the OAuth flow manually since the client-oauth2 library doesn't support PKCE for OAuth 2.0

ryanwi added 4 commits January 9, 2024 17:18
…, revoked, does not match the redirection URI used in the authorization request, or was issued to another client.'
@ryanwi ryanwi changed the title manual oauth wip manual oauth Jan 10, 2024
@ryanwi ryanwi changed the title manual oauth Manual OAuth 2.0 Jan 10, 2024
@ryanwi ryanwi marked this pull request as ready for review January 10, 2024 04:22
@ryanwi
Copy link
Collaborator Author

ryanwi commented Jan 10, 2024

Note, we've also been exploring using auth.js https://github.com/signalwire/call-fabric-client-beta/compare/joao/use_auth_js?expand=1

for the example/reference, maybe we want to show it more than rely on the lib, I dunno?

Copy link
Contributor

@iAmmar7 iAmmar7 left a comment

Choose a reason for hiding this comment

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

What problem does the library solve? I think ideally, we wouldn't want to rely on the library if we could easily build our own solution.

This looks good to me, approving this PR 🚀 with a few minor suggestions.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
params.append('redirect_uri', process.env.OAUTH_REDIRECT_URI);
params.append('code_verifier', req.session.verifier);

const response = await fetch(process.env.OAUTH_TOKEN_URI, {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have axios installed as well. I think for consistency, we should stick to one. Maybe we can remove the axios.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, seeing that auth.js relies on fetch made we wonder why we use axios still, I can convert the other method and remove axios if we think removing it is fine.

@jpsantosbh jpsantosbh mentioned this pull request Jan 10, 2024
@jpsantosbh
Copy link
Contributor

Note, we've also been exploring using auth.js https://github.com/signalwire/call-fabric-client-beta/compare/joao/use_auth_js?expand=1

for the example/reference, maybe we want to show it more than rely on the lib, I dunno?

IMO,

The we need to choose the implementation based on this question.

What is the audience for this app?

If internal only, I think I like the manual approach better.

if it's some thing to share with clients too, I'll prefer the Auth.js solution.

The reasoning behind it is that as a reference for clients we should aim to define the best practices, and in my experience security/auth best practices is alway to use a well-known open-source implementation. Other wise we need to always stay on top of possible security issues with our implementation.

Copy link
Contributor

@jpsantosbh jpsantosbh left a comment

Choose a reason for hiding this comment

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

just consider my comment before merging it

ryanwi and others added 2 commits January 10, 2024 07:46
Co-authored-by: Ammar Ansari <[email protected]>
Co-authored-by: Ammar Ansari <[email protected]>
@ryanwi
Copy link
Collaborator Author

ryanwi commented Jan 10, 2024

Good points, this is public and will be likely shared with clients I think.

That said, the oauth standard makes the client implementation fairly straightforward, it's not a custom auth scheme.

We can start with this maybe and then continue to explore auth.js more?

@ryanwi ryanwi requested review from jpsantosbh and iAmmar7 January 10, 2024 16:45
@ryanwi ryanwi merged commit 8dd81be into main Jan 11, 2024
3 checks passed
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