-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat: instagram oauth provider #2534
Conversation
Rebased and dist updated to v0.16.1 |
Co-authored-by: Pedro Costa <550684+pnmcosta@users.noreply.github.com>
Thanks! To avoid the back-and-forth, I've merged it locally in the The provider will be available as part of v0.17.0 release (there are no ETAs yet). |
Brilliant, thank you |
@ganigeorgiev reopening cause I found an issue: On https://github.com/pocketbase/pocketbase/blob/develop/tools/auth/instagram.go#L56C7-L56C7 we would need to sanitize the username from instagram to PB restriction (alphanumeric I believe) otherwise creating the auth record could fail. Is there any technical issue for why the PB auth record Username is only alphanumeric? |
I don't think it is necessary as by default PocketBase will use the username only if it is "valid", otherwise will generate its own.
There is no specific reason, it is this way just because that is my use case in Presentator. There are plans in the future to convert the |
Ok, so via oauth it will not fail insert even if the username has invalid chars? Also when updating on the admin screen? I'm asking cause I actually noticed the issue using the |
Yes, we have an extra check that validate the username before assigning it when the record is created via OAuth2. If it is invalid it will use an autogenerated one.
No, in this case the regular validation will be triggered. If you want to skip the username validation you can assign the username directly to the record model OR persist the record with the |
Ah, this will be a problem for me as I need the IG username regardless if it's PB valid or not. And as I think most using the provider would assume the same, I'm more inclined to have the provider not set the username, it can be grabbed from the RawUser if needed and set it on a text field
Would I be able to set the email and password this way? The reason I'm using the form upsert is cause of the |
I don't have strong opinion on this and I'm not sure what to recommend. It is possible to overwrite the record username in the OAuth2 hook (you can grab the raw provider username from the
Yes, the upsert form actually under the hood uses directly the Dao to save the record but with additional extra layer for validations. To set a password hash from a plain password, you can use |
I think this says it all:
I'm going to remove setting the PB username with IG's. And use the events to do so, it's a cleaner approach IMO. |
Fork updated: pnmcosta@d86fa99 |
Again, this may not be necessary. You can still overwrite the Record username if you want to and keep the Instagram username as default (when possible). This way the behavior would be consistent with the other providers. |
@ganigeorgiev added named api scenario test runs to my fork (d0154c1) so that: It's really handy for my project, would you consider adding it? Thank you |
Yes, I'll consider wrapping it in the near future. |
Turns out the user response from the Instagram Basic API (see docs) is not compatible with the existing OIDC provider.
It's a pretty straightforward change, I've included the UI resources and
ui/dist
build too.Let me know if you have any questions?