feature: add new WorkOS provider #402
Conversation
This allows referencing of query parameters when constructing new provider objects.
This module can be used to easily convert interfaces to structs without going through the JSON serialization/deserialization dance.
The `provider` query provider is already passed to the `/authorize` route, so an alternative query parameter (`workos_provider`) is chosen instead.
… `avatar` Certain providers (eg. WorkOS, Azure) do not return an avatar URL in their user profile data. Hence, it is possible for `user.UserMetaData["avatar_url"]` to be `nil`. However, the `assertAuthorizationSuccess` function only accepts a `string` to compare against the stored avatar URL in a test, causing certain tests to fail when a This PR tweaks the test for avatar URL to check for `nil` if the supplied expected input is `""` (the empty string). Various tests are also modified for the Azure provider, which previously had workarounds including already creating a user before the signup occurred, which meant the initial login flow wasn't thoroughly tested. The alternative would be to change the type of `avatar` to `*string`, which is a lot more troublesome to write for in the common case of having an avatar URL, since you can't use the `&` referencing syntax for string constants.
bnjmnt4n
changed the title
There was a problem hiding this comment.
It looks good to me. Thanks for the comment in the commit 20c was helpful in understanding the PR.
| Subject: u.ID, | ||
| Name: strings.TrimSpace(u.FirstName + " " + u.LastName), | ||
| Email: u.Email, | ||
| EmailVerified: true, |
There was a problem hiding this comment.
i assume emails returned by workos are guaranteed to be verified?
There was a problem hiding this comment.
Actually they aren't: should I delete that line? I saw that some other providers had that too, even if their providers didn't guarantee so, so I did the same. I couldn't figure out the importance of that though: what does this affect?
There was a problem hiding this comment.
Right, confirmation emails are disabled for us so this wasn't on my mind. I can remove it if necessary, although should that be done for the other providers as well?
There was a problem hiding this comment.
other providers don't return the email at all if the email is not verified, do you happen to know if this is the case for workos as well?
There was a problem hiding this comment.
I believe WorkOS returns the email based on what the SAML SSO it's integrated with returns, without verifying if the email actually exists. However, I don't think this is much of an issue, since users have to manually add these SAML SSO integrations into WorkOS themselves, and if they do so it means that they trust these SAML SSO providers on the other end to return the correct results.
I think it might make sense to drop the EmailVerified: true though. WDYT?
There was a problem hiding this comment.
Ah i see, then in that case, i actually think it's fine for EmailVerified: true. The original SAML implementation by netlify also assumes that the email used is verified.
There was a problem hiding this comment.
👍, I've changed back to that behaviour.
|
🎉 This PR is included in version 2.6.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
What kind of change does this PR introduce?
Feature.
What is the current behavior?
No provider for WorkOS.
What is the new behavior?
A new provider for WorkOS is added.
Additional context
Based on initial work done by @HarryET in #269 and @J0 in #346.
Points to note:
/authorizeroute as well to be passed through. This required a tweak toApi.Providerto accept another argument of type*url.Values, which can be used to pass through query parameters for any providers requiring them.map[string]interface{}, I added a module (https://github.com/mitchellh/mapstructure) which helps to automatically convert it into a struct, instead of having to do the JSON serialize/deserialize dance.assertAuthorizationSuccesstest utility, which are explained in detail in the commit message here: 20c18c3.