Skip to content

Conversation

@alexandrebodin
Copy link
Member

What does it do?

Fix linkedin auth provider in U&P

@alexandrebodin alexandrebodin added source: plugin:users-permissions Source is plugin/users-permissions package pr: fix This PR is fixing a bug labels Jun 25, 2024
@alexandrebodin alexandrebodin added this to the 4.25.2 milestone Jun 25, 2024
@alexandrebodin alexandrebodin self-assigned this Jun 25, 2024
@alexandrebodin alexandrebodin requested a review from Convly June 25, 2024 14:02
@lswartsenburg
Copy link

Backward incompatible with people not on openid. Please don't merge this. It will cause a lot of problems for people that have linked auth setup preciously

@vercel
Copy link

vercel bot commented Jun 25, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
contributor-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 25, 2024 2:11pm

@alexandrebodin
Copy link
Member Author

@lswartsenburg If you have any suggestion that can work let us know. right now it was the only way we could make it work

Copy link
Member

@derrickmehaffy derrickmehaffy left a comment

Choose a reason for hiding this comment

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

Didn't test with LinkedIn but looks fine to me. Looks like LinkedIn finally fix their non-standard scopes.

@alexandrebodin
Copy link
Member Author

Didn't test with LinkedIn but looks fine to me. Looks like LinkedIn finally fix their non-standard scopes.

Yeah it was the only way this could work. Considering linkedin connection is currently broken anyway I don't see how this would be an issue to change it as nobody can make it work right now because they removed the old endpoints & access

@lswartsenburg
Copy link

@alexandrebodin just create a new provider like the original PR to continue supporting the legacy client until it's officially deprecated

@lswartsenburg

This comment was marked as abuse.

@alexandrebodin
Copy link
Member Author

alexandrebodin commented Jun 25, 2024

@alexandrebodin just create a new provider like the original PR to continue supporting the legacy client until it's officially deprecated

Grant doesn't work like this sadly, you cannot just add providers with custom names. They have to match with what grant knows. This is why your original PR didn't work and was getting Grant missconfiguration errors.

The old provider is not working so who is this keeping support for ?

@derrickmehaffy
Copy link
Member

@lswartsenburg I understand you might be frustrated but your comment is in violation of our CoC: https://github.com/strapi/strapi/blob/develop/CODE_OF_CONDUCT.md

It's fine to disagree with us, but we expect users to be civil on our repos. This will be your only warning, the next one will be a ban from our repos and community platforms.

@lswartsenburg
Copy link

lswartsenburg commented Jun 25, 2024

@derrickmehaffy fair I'll tune it down. But I hope you also realize how uncivil it is to leave open source contributors hanging for months despite repeated outreach (including to yourself).

@alexandrebodin
Copy link
Member Author

@lswartsenburg If you are willing to help us get this PR merged with your suggestion and make it work that would be very helpful.
Starting from your previous PR grant was just not doing anything. did you actually got it to work on your side previously? Feel free to make a PR on top of this branch if you find a way

@lswartsenburg
Copy link

@alexandrebodin happy to, but I realistically won't get to it till Friday (sorry, I'm responding on my phone here because I'm so strapped for time).

And yeah, you can see on the screenshot of the original PR that the callback working fine. I should have added a UI pic too, but I guess I forgot.

@alexandrebodin
Copy link
Member Author

@alexandrebodin happy to, but I realistically won't get to it till Friday (sorry, I'm responding on my phone here because I'm so strapped for time).

And yeah, you can see on the screenshot of the original PR that the callback working fine. I should have added a UI pic too, but I guess I forgot.

Ok so either sth changed on linkedin or I missconfigured linkedIn somehow. No rush we are focus on the next major right now and have barely time to do anything else

@derrickmehaffy
Copy link
Member

Yeah Strapi 5 is eating up almost all of our time so community PRa have been lower priority. We hope to change that later this year as we know we haven't been great it.

Any help you can give us would be great thank you.

@alexandrebodin
Copy link
Member Author

FYi I think I figured out the missing piece: simov/grant#220

We need to provide all the auth options when adding an unknown provider to grant so we could duplicate the base linkedin config.

@bn-pshulga
Copy link

Thank you for taking my pr, @alexandrebodin

@alexandrebodin
Copy link
Member Author

Thank you for taking my pr, @alexandrebodin

you are welcome, it was just missing that openid scope. We have to figure out if the old api is still usable somhow or if we can just fix the main provider. it's unclear to me if they actually just deprecated it or completely dropped it and I can't find any doc for it

@bn-pshulga
Copy link

Thank you for taking my pr, @alexandrebodin

you are welcome, it was just missing that openid scope. We have to figure out if the old api is still usable somhow or if we can just fix the main provider. it's unclear to me if they actually just deprecated it or completely dropped it and I can't find any doc for it

Same , I can't find any docs related to changes.
Only auth0 docs help me to figure out what part was throwing error.

@Marc-Roig Marc-Roig removed this from the 4.25.2 milestone Jul 3, 2024
@Marc-Roig Marc-Roig added this to the 4.25.3 milestone Jul 3, 2024
@alexandrebodin alexandrebodin removed this from the 4.25.3 milestone Jul 4, 2024
@strapi-bot
Copy link

This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/strapi-wont-update-providers/40292/2

@MannJadwani
Copy link

Any update on when this is going to be merged

@alexandrebodin
Copy link
Member Author

This is staled right now, If you read the discussion you might be able to help us and share some insights @MannJadwani

@Convly Convly removed their request for review August 7, 2024 07:43
@bn-pshulga
Copy link

@alexandrebodin
Copy link
Member Author

alexandrebodin commented Sep 12, 2024

Considering this is currently broken for current & previous users I don't see how this change could be a BC 🤔

-> The question is "Is the v1 still accessible ?".

I release an experimental version 0.0.0-experimental.b067fc6e53021c0b865a20ad6ad63823ee6db793 would you mind trying ?

@alexandrebodin
Copy link
Member Author

Hey @bn-pshulga did you get a chance to test the experimental version ?

@bn-pshulga
Copy link

Sorry @alexandrebodin , didn't have time to test.

@alexandrebodin
Copy link
Member Author

@bn-pshulga No worries I'll leave it here for now if someone with linkedin access can test it sometime too

@oismaelash
Copy link
Contributor

This fix is needed,
or can have two provider for linkedin v1 and v2 on providers page

@alexandrebodin and @lswartsenburg

@alexandrebodin
Copy link
Member Author

This fix is needed, or can have two provider for linkedin v1 and v2 on providers page

@alexandrebodin and @lswartsenburg

@oismaelash I could not find a solution to have a second linkeding config, if you want to try sth feel free to start from this PR to validate it's working for you.

@oismaelash
Copy link
Contributor

@alexandrebodin I work on this bug,
I show the input of scope on frontend, by default, the scope is the linkedin v1, but the user can change for scope linkedin v2 (current)

Image preview:
image

WIP here:
https://github.com/oismaelash/strapi/commits/fix/linkedin-custom-scope/

@hanpaine
Copy link
Contributor

hanpaine commented Nov 5, 2024

Nice one @oismaelash, let us know if/when you've cracked it or we'll keep this thread posted if we come back to this soon 🙏

@oismaelash
Copy link
Contributor

Nice one @oismaelash, let us know if/when you've cracked it or we'll keep this thread posted if we come back to this soon 🙏

Next week, I release a PR

@bn-pshulga

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

flag: don't merge This PR should not be merged at the moment pr: fix This PR is fixing a bug source: plugin:users-permissions Source is plugin/users-permissions package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants