feat: extra auth providers & tests for previously added providers #269
Conversation
No valid API data, removed from models so that there is no inaccurate data
This isn't working and needs changes for getting user data
Takes code from supabase#238 and finished implementation, still needs improvements in provider impl file
|
Thanks a lot for the work ! |
- Forgot to change values after copy & paste
No problems, all those issues have been fixed now. Should be good unless I have missed something else 😅 |
|
Nice work @HarryET - just a tip: usually lots of smaller PR's are better than a big one (becuase sometimes one small thing can hold up the merge) |
|
@kiwicopple for the context @HarryET was the only person to help on my PR #238 for Linkedin. |
|
definitely ! this is just feedback for future PRs - not criticism at all. We love the work Harry's doing :) |
|
Feedback is important and you are the first person i see in open source with a nice way to say it. |
Yeah, of course @kiwicopple. For future reference would you have recommended I split this PR into 4 smaller ones? WorkOS, LinkedIn, TikTok and Tests for other providers? Just better to know for future reference as I'd love to keep contributing more 😀 |
|
Hey @HarryET, thanks so much for all of these oauth provider contributions 💪 (Also, @riderx for kick-starting the linkedin provider implementation). Yup it would definitely be easier for us to review the changes as well if they were split into 4 smaller ones. A guideline I like to follow would be to split the PR by feature or by individual bug fixes. Refactoring work should also be kept separate from feature additions or bug fixes. Will start testing them out soon, apologies for the delay - we've been pretty busy recently ramping up for launch week! |
Hey @kangmingtay, of course makes alot more sense now I think about it sorry for giving you guys extra work 😅 |
|
Might be worth adding that to the contribution guide? And hopefully having a section for setting up a local test instance as j haven't been able to do that just yet |
|
yeah, sorry about the lack of guidance in the contribution section, @dthyresson and I have talked about it and i'll definitely look to get something up this week! |
|
@dthyresson i understand why that important i took the fix @HarryET did an reopen my branch for Linkedin provider, check it there |
Hey @dthyresson sorry for the late reply, I'll separate them asap. Been busy doing some other work |
| ) | ||
|
|
||
| const ( | ||
| defaultWorkOSAPIBase = "https://api.workos.com" |
There was a problem hiding this comment.
you can leave out the https:// because gotrue will use https to make the redirect call
| ClientID: ext.ClientID, | ||
| ClientSecret: ext.Secret, | ||
| Endpoint: oauth2.Endpoint{ | ||
| AuthURL: apiPath + "/sso/authorize", |
There was a problem hiding this comment.
Some issues with the workos api, i tried setting up an account and faced with this message after calling /sso/authorize.
I tried adding the domain query param manually and it seemed to work so maybe its not optional even though it says so in their api. I've already gave my feedback to workos, hopefully they offer a workaround of some sort.
There was a problem hiding this comment.
Hey @kangmingtay, did the WorkOS team ever get back to you about your feedback?
There was a problem hiding this comment.
hmm i haven't heard back from them yet, will reach out again to check!
|
Thanks once again @HarryET for the oauth provider contributions so far! I've only tested out workos and linkedin for now because im waiting on tiktok to approve my developer app 🤞 Also, for linkedin, it could just be me but i can't seem to sign-in with linkedin using my linkedin account because my email is not being exported even though this setting is toggled on. Did anyone else face this issue too?
|
Oh awesome! As for LinkedIn I haven't personally tried as to not having an account. I was previously told to split this PR, are we still going ahead with that or just leave it now - with you having almost finished testing - and in future split it? |
Ofc, Will try do it asap. Just been busy with a few other things. When I move WorkOS to its own PR i'll try fix the issues :) |
|
@kangmingtay you don't need to add your email to data export for Linkedin auth, it should work directly. |
|
I'd happily get on a call with you to help you get your local instance running! Reach out via twitter or discord - HarryET#2954 |
|
@HarryET thanks just added you ! |
|
@kangmingtay, Would I be okay to just fix this PR and merge it as one big PR with the 3 providers? Currently I don't have the time to separate them all out but should be able to fix the misc issues. |
I really think that there should be a PR per provider implementation with tests and/or instructions how to setup/configure and test. That way can ensure that working code is added to the main GoTrue implementation. This also makes it easier to merge in without conflicts. |
|
@dthyresson For Linkedin the PR exist since 17 Oct 2021, sadly it doesn't look to help on the process. |
|
ok I reread the thread and decided to delete the past two messages and posted this new one. @HarryET would it be okay if I cherry-picked the LinkedIn or WorkOS changes into a separate branch? Would like to familiarize with the codebase so I'm quite keen to do so. I promise to give you full credit and will tag you for review once done. Totally understand if you prefer to finish it up though! Lmk! |
There are 3 providers in this PR currently,
LinkedIn was origninaly by @riderx, However, I finished it and added it to this PR. It was then cherry picked back out into @riderx's PR. TikTok's implementation is solid so should be ready to be pulled off into it's own PR while WorkOS still has a few issues that @kangmingtay pointed out in a review above. This PR also includes tests for Slack and Spotify that I added in a previous PR #245 which also need to get merged in. I'm happy for anyone to cherry pick them out, make separate PRs on the one conditon I am added as a Co-Author to the Commits that have the cherry picked code :) If @J0 wants to do that feel free, but LinkedIn has already been done with this PR #238. The LinkedIn PR brings up another point that I feel could do with its own discussion or issue addressing the ease of creating providers and what needs to be done for the team to review as that has been left dead since the 26th of November when myself and @riderx spent ~1hr trying to get @riderx's local environment setup and there has been no response for the GoTrue team. I understand it is hard for @kangmingtay being the only auth engineer right know - to my knowlage. However, I feel even just a message/reply on that PR to acknowlage the issues and the fact the team are looking at the PR in some way may have been helpful. Thanks again to the GoTrue team for all their help and @J0 for offeering to split this up as I wouldn't have been able to do it for another ~3 weeks. |
|
First of all, thank you and great job on putting together auth for the multiple providers -- think it takes substantial effort to do so especially out of your own time especially given that you'd both probably have other commitments outside of OSS. I'd initially thought there were a few fixes remaining before LinkedIn could go in but sounds like it is mostly done. Seems like you've got a good handle on what needs to be done so will leave the PRs to you unless there are urgent requests for Slack/Spotify. Will look at implementing one of the other providers! Feel free to reach out if there's anything else you need help with though! |
|
It's unfortunate that both of you had to go through significant trouble setting up the environment. One suggestion could be a checklist of security considerations/things that need to be considered/tested/done in With regards to setup: I will be setting up GoTrue for the first time soon and I can record/document the process + workarounds if that'd make it easier. That said, I also recognize that lots of issues will be environment specific and a video/document might not help much -- perhaps we could have a #gotrue additional channel on discord(if not already present) for people to reach out about setup. Just throwing out ideas, would love to hear any thoughts you have as well. |
These are great suggestions @J0! |
I am in the process of writing a blog post on adding providers! |
|
The LinkedIn PR has been merged! |
|
Also @J0, feel free to cherrypick off @HarryET's work to implement the WorkOS provider if you're interested! I've just gotten the approval from TikTok for the oauth app created so i will begin testing the TikTok provider soon. I think the WorkOS implementation might still need quite abit of work. I haven't looked into it in detail yet but it seems like it's more related to SSO rather than oauth2 (https://workos.com/docs/reference/sso)? |
|
@kangmingtay sure! I haven't quite read into the context surrounding it but I'll start looking into it on Saturday GMT+8 @liaujianjie feel free to let me know if there's anything I should take note of. Will tag you for review once the PR is done :) @HarryET I'll be sure to give due credit to you for laying the groundwork for the PR. Feel free to ping me if there's anything I should take note of or if you'd prefer to finish it up! |
|
@J0 @kangmingtay do we want this to be closed? |
|
Closing due to being way out of sync and mostly covered by other people's PRs |
What kind of change does this PR introduce?
Implement SoundCloud OAuth, add SoundCloud OAuth Provider #258What is the current behavior?
No SoundCloud OAuthWhat is the new behavior?
Added Soundcloud OAuthAdditional context