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: adds discord oidc provider #767

Merged
merged 7 commits into from Oct 15, 2020
Merged

Conversation

NickUfer
Copy link
Contributor

Related issue

Closes #533
Closes #534

Proposed changes

Adds discord as an oidc provider. This is a continuation of the stale PR #534.

Checklist

  • I have read the contributing 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 green light (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

I will improve the documentation with further direct code references once we have a final commit id or release.

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.

Let me think about how to test this stuff tomorrow. But looks good, thanks for adding the documentation 👍

Comment on lines 3 to 13
import (
"context"
"fmt"
"github.com/bwmarrin/discordgo"
"github.com/ory/herodot"
"github.com/ory/x/stringslice"
"github.com/ory/x/stringsx"
"github.com/pkg/errors"
"golang.org/x/oauth2"
"net/url"
)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you didn't run make format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... make format seems to use lintx...when I try to install lintx it wants to install setupdocx, which has a dependency on setuplibs which has (how it looks to me) again a circular dependency on setupdocx... I think i have to do this tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normally my IDE formats everything correct but not this time haha

Copy link
Member

Choose a reason for hiding this comment

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

you have to

$ go install github.com/sqs/goreturns

goreturns just orders imports differently than the default by grouping them

Copy link
Contributor Author

@NickUfer NickUfer Oct 15, 2020

Choose a reason for hiding this comment

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

Done. Somehow github.com/ory/x/tools/listx was not installed and I thought lintx was this. But it seems lintx is supposed to be installed automatically?

Copy link
Member

Choose a reason for hiding this comment

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

There are some hacks in the Makefile but it does not work for me reliably either....

@NickUfer
Copy link
Contributor Author

NickUfer commented Oct 15, 2020

Hmm... I just found out that the discordgo library uses the API version v6 of discord, but it is already deprecated and the current version is v8: Discord Reference. And the issuer url is also hardcoded to v6. That could be problematic when they stop v6 in the near future.

Discord Lib Code

@zepatrik
Copy link
Member

zepatrik commented Oct 15, 2020

Do you think that discordgo will update to v8? That would be the best approach, even if they do it later on.
v6 is marked as default currently.

@NickUfer
Copy link
Contributor Author

Do you think that discordgo will update to v8? That would be the best approach, even if they do it later on.
v6 is marked as default currently.

Yeah they will probably update it. But we should not forget to update once it uses v8 by default.

@zepatrik
Copy link
Member

Looks like the authentication URLs do not strictly require the API version. They will probably always work without the version: https://discord.com/developers/docs/topics/oauth2#shared-resources-oauth2-urls

@zepatrik
Copy link
Member

$ curl https://discord.com/api/v4/oauth2/authorize | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100    49  100    49    0     0    262      0 --:--:-- --:--:-- --:--:--   262
{
  "message": "Invalid API version",
  "code": 50041
}
$ curl https://discord.com/api/v6/oauth2/authorize | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100    41  100    41    0     0    253      0 --:--:-- --:--:-- --:--:--   253
{
  "client_id": [
    "This field is required"
  ]
}
$ curl https://discord.com/api/v8/oauth2/authorize | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   156  100   156    0     0   1006      0 --:--:-- --:--:-- --:--:--  1006
{
  "code": 50035,
  "errors": {
    "client_id": {
      "_errors": [
        {
          "code": "BASE_TYPE_REQUIRED",
          "message": "This field is required"
        }
      ]
    }
  },
  "message": "Invalid Form Body"
}
$ curl https://discord.com/api/oauth2/authorize | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100    41  100    41    0     0    232      0 --:--:-- --:--:-- --:--:--   232
{
  "client_id": [
    "This field is required"
  ]
}

Looks like the URL without an explicit version will always point to the default? So it should always work. Maybe add a PR to discordgo to use the authentication URL without a version?

@NickUfer
Copy link
Contributor Author

Looks like the authentication URLs do not strictly require the API version. They will probably always work without the version: https://discord.com/developers/docs/topics/oauth2#shared-resources-oauth2-urls

Yes, but we use the /users/@me endpoint where we need the api version: https://discord.com/api/v6/users/@me

@zepatrik
Copy link
Member

Alright, let's continue with this as is. We can open an issue in discordgo that request v8 support and add the issue URL in a code comment. https://github.com/ory/closed-reference-notifier will tell us when the issue is resolved 😉

@aeneasr aeneasr merged commit 487296d into ory:master Oct 15, 2020
@aeneasr
Copy link
Member

aeneasr commented Oct 15, 2020

Awesome 🎉

Thank you for your contribution!

@NickUfer NickUfer deleted the socials-discord branch October 17, 2020 19:22
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.

Support Discord as OIDC Provider
4 participants