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

feat: added patreon oidc provider #3021

Merged
merged 5 commits into from Jan 18, 2023
Merged

feat: added patreon oidc provider #3021

merged 5 commits into from Jan 18, 2023

Conversation

AUSBird
Copy link
Contributor

@AUSBird AUSBird commented Jan 13, 2023

This PR is intending to implement another OIDC provider for Patreon to Kratos since Patreon has non-standard paths to it's auth and token endpoints and doesn't have a discovery endpoint

While I was there I also added missing OIDC provider types to a code comment to update it with missing ones (Not sure if it matters or no)

Related issue(s)

I never made an issue to for this because it's just a new provider and the issue templates only has one for non-trivial features. (If I should have made one anyway, tell me for next time :) )

I did bring it up months ago in slack but never got any response,

PR for Documentation: ory/docs#1211

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

This is my first ever go at golang so feel free to give me feedback on anything but I do ask you simplify a bit and if possible can I get pointers to documentation when comments are made to help me learn :)

@AUSBird AUSBird marked this pull request as ready for review January 13, 2023 13:24
@AUSBird AUSBird changed the title feat: Added patreon OIDC provider feat: added patreon oidc provider Jan 13, 2023
@AUSBird
Copy link
Contributor Author

AUSBird commented Jan 13, 2023

I don't know how or where that unit test broke, I had a look where it said and nothing around it seems to obviously connect to my changes.

Can someone please give me a small pointer to solving it?

Edit: Merging master in fixed the test

Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

Nice, this looks very good 👍
I just have one minor remark.

selfservice/strategy/oidc/provider_patreon.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jan 16, 2023

Codecov Report

Merging #3021 (f296012) into master (ba1aecf) will decrease coverage by 0.11%.
The diff coverage is n/a.

❗ Current head f296012 differs from pull request most recent head d649c88. Consider uploading reports for the commit d649c88 to get more accurate results

@@            Coverage Diff             @@
##           master    #3021      +/-   ##
==========================================
- Coverage   77.41%   77.29%   -0.12%     
==========================================
  Files         310      311       +1     
  Lines       19268    19317      +49     
==========================================
+ Hits        14916    14932      +16     
- Misses       3198     3234      +36     
+ Partials     1154     1151       -3     
Impacted Files Coverage Δ
selfservice/strategy/oidc/provider_config.go 32.65% <0.00%> (-1.39%) ⬇️
selfservice/strategy/oidc/provider_patreon.go 0.00% <0.00%> (ø)
persistence/sql/persister_courier.go 86.50% <0.00%> (+3.17%) ⬆️
courier/courier_dispatcher.go 59.21% <0.00%> (+15.78%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@AUSBird AUSBird requested review from zepatrik and removed request for aeneasr January 16, 2023 21:16
Copy link
Contributor

@jonas-jonas jonas-jonas left a comment

Choose a reason for hiding this comment

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

Looks very good to me. Thanks for your contribution. :)

Waiting for @aeneasr or @zepatrik to confirm & merge.

Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@zepatrik zepatrik merged commit 20ea29e into ory:master Jan 18, 2023
@vinckr
Copy link
Member

vinckr commented Jan 18, 2023

Hello @AUSBird
Congrats on merging your first PR in Ory 🎉 !
Your contribution will soon be helping secure millions of identities around the globe 🌏.
As a small token of appreciation we send our first time contributors a gift package to welcome them to the community.
Please drop me an email and I will forward you the form to claim your Ory swag!

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.

None yet

4 participants