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

website/integrations: Update discord integration with guild and role check #5701

Merged
merged 6 commits into from
May 21, 2023

Conversation

Aterfax
Copy link
Contributor

@Aterfax Aterfax commented May 20, 2023

Details

Adds two sections to this document describing how the required expression policies needed to check users are a member of a certain guild or a member of a certain guild with a certain role.

I've tried to keep this closely aligned with the same syntax / style used for the equivalent Github OAuth social login page.

I've tested these and both appeared to work as intended. I'd recommend that someone else also give these a second check for functionality before merging.

Please squash merge.

I tried to name this PR according to the styleguide but it was a bit unclear what the 'package' would be.

Changes

New Features

  • N/A - documentation update.

Breaking Changes

  • N/A - documentation update.

If applicable

  • The documentation has been updated
  • The documentation has been formatted (make website)

Adds two sections to this document describing how the required expression policies needed to check users are a member of a certain guild or a member of a certain guild with a certain role.

Signed-off-by: Aterfax <Aterfax@users.noreply.github.com>
@Aterfax Aterfax requested a review from a team as a code owner May 20, 2023 21:26
@netlify
Copy link

netlify bot commented May 20, 2023

Deploy Preview for authentik ready!

Name Link
🔨 Latest commit 91de0f8
🔍 Latest deploy log https://app.netlify.com/sites/authentik/deploys/646a09758d1e24000816cd0f
😎 Deploy Preview https://deploy-preview-5701--authentik.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@BeryJu BeryJu left a comment

Choose a reason for hiding this comment

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

Couple of minor adjustments to the expression code, thanks for the PR!

website/integrations/sources/discord/index.md Outdated Show resolved Hide resolved
website/integrations/sources/discord/index.md Outdated Show resolved Hide resolved
website/integrations/sources/discord/index.md Outdated Show resolved Hide resolved
website/integrations/sources/discord/index.md Outdated Show resolved Hide resolved
website/integrations/sources/discord/index.md Outdated Show resolved Hide resolved
website/integrations/sources/discord/index.md Outdated Show resolved Hide resolved
website/integrations/sources/discord/index.md Outdated Show resolved Hide resolved
Refactor as per BeryJu's suggestions.

Co-authored-by: Jens L. <jens@beryju.org>
Signed-off-by: Aterfax <Aterfax@users.noreply.github.com>
@BeryJu BeryJu changed the title website/integrations: Update integrations/sources/discord/page with example expression policies website/integrations: Update discord integration with guild and role check May 21, 2023
@codecov
Copy link

codecov bot commented May 21, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.01 🎉

Comparison is base (b4a3b26) 92.58% compared to head (91de0f8) 92.59%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5701      +/-   ##
==========================================
+ Coverage   92.58%   92.59%   +0.01%     
==========================================
  Files         546      546              
  Lines       26198    26198              
==========================================
+ Hits        24254    24255       +1     
+ Misses       1944     1943       -1     
Flag Coverage Δ
e2e 51.94% <ø> (+0.04%) ⬆️
integration 26.39% <ø> (ø)
unit 89.48% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@BeryJu BeryJu merged commit d8de60b into goauthentik:main May 21, 2023
@Keyinator
Copy link
Contributor

Keyinator commented May 21, 2023

In the later code If the user is not in said guild you will get the error builtins.KeyError: 'roles' since guild_member_object does not have that key. guild_member_object will be mem-info:{'message': 'Unknown Guild', 'code': 10004}

I suggest implementing some error handling for that case.

@Aterfax
Copy link
Contributor Author

Aterfax commented May 21, 2023

In the later code If the user is not in said guild you will get the error builtins.KeyError: 'roles' since guild_member_object does not have that key. guild_member_object will be mem-info:{'message': 'Unknown Guild', 'code': 10004}

I suggest implementing some error handling for that case.

My intended idea for the role checking expression policy would be a denied access for not having the role, so not being a member of the guild is certainly a different case of that too.

If no one objects I think there should be an earlier check that the user is a member of the guild and if not, the expression ends immediately with return False. That said I'd need to take a look at how the Discord API responds when forming the guild_member_object response when a user is not a member to figure out what logic to use or to make two requests with the earlier one checking the same logic as the other guild member checking expression.

Edit: I suppose we could just check that the roles key does not exist but I'd prefer to check the Discord API for what the sensible choice is first.

There does not appear to be anything fancy going nor anything more detailed explained in the Discord API docs, so adding the following before evaluating the role is probably sufficient?

snip

Edit: Wasn't good code above - please instead see #5709

@Keyinator
Copy link
Contributor

# If the user is not in the guild return false
guild_matched = (guild_member_object["message"] == "Unknown Guild")
if guild_matched:
  return false

If I am correct this will fail if the user actually is in the guild since the dictionary will not have a key for message.

Imo my code above would be a nice way of handling that specific error and from my knowledge no other exception comes to mind that could throw further exceptions so we should be in the clear.

If you really wanted to be sure you could check 'roles' in guild_member_object to see if they key exists. Maybe:

if not 'roles' in guild_member_object
  return False

as a last resort.
(I'd still at least implement the above code-change since that returns a more user-friendly error)

@Aterfax
Copy link
Contributor Author

Aterfax commented May 21, 2023

As per our chat and exploration in Discord it seems like the following might be the best solution?

# The response for JSON errors is held within guild_member_object['code']
# See: https://discord.com/developers/docs/topics/opcodes-and-status-codes#json
# If the user isn't in the queried guild, it gives the somewhat misleading code = 10004.
if "code" in guild_member_object:
    if guild_member_object['code'] == 10004:
        ak_message(f"User is not a member of {GUILD_NAME_STRING}.")
    else:
        ak_message(f"Error, check https://discord.com/developers/docs/topics/opcodes-and-status-codes#json for error code {guild_member_object['code']}.")
    # Policy does not match if there is any error.
    return False

BeryJu added a commit that referenced this pull request May 21, 2023
…n error handling. (#5709)

* website/integrations: Update discord integration expression error handling

As per discussion in #5701 after merge, we could do with handling the case where the user is not in the guild being queried!

Signed-off-by: Aterfax <Aterfax@users.noreply.github.com>

* Correct lowercase f in False.

Signed-off-by: Aterfax <Aterfax@users.noreply.github.com>

* Update website/integrations/sources/discord/index.md

Co-authored-by: Jens L. <jens@beryju.org>
Signed-off-by: Aterfax <Aterfax@users.noreply.github.com>

---------

Signed-off-by: Aterfax <Aterfax@users.noreply.github.com>
Co-authored-by: Jens L. <jens@beryju.org>
ChandonPierre pushed a commit to ChandonPierre/authentik that referenced this pull request Jun 1, 2023
…check (goauthentik#5701)

* Update Discord OAuth instructions - index.md

Adds two sections to this document describing how the required expression policies needed to check users are a member of a certain guild or a member of a certain guild with a certain role.

Signed-off-by: Aterfax <Aterfax@users.noreply.github.com>

* Linting and styleguide amendments.

* Remove spurious empty lines.

* Add an extra line to space comments out.

* Moved warning in wrong place.

* Apply suggestions from code review

Refactor as per BeryJu's suggestions.

Co-authored-by: Jens L. <jens@beryju.org>
Signed-off-by: Aterfax <Aterfax@users.noreply.github.com>

---------

Signed-off-by: Aterfax <Aterfax@users.noreply.github.com>
Co-authored-by: Jens L. <jens@beryju.org>
ChandonPierre pushed a commit to ChandonPierre/authentik that referenced this pull request Jun 1, 2023
…n error handling. (goauthentik#5709)

* website/integrations: Update discord integration expression error handling

As per discussion in goauthentik#5701 after merge, we could do with handling the case where the user is not in the guild being queried!

Signed-off-by: Aterfax <Aterfax@users.noreply.github.com>

* Correct lowercase f in False.

Signed-off-by: Aterfax <Aterfax@users.noreply.github.com>

* Update website/integrations/sources/discord/index.md

Co-authored-by: Jens L. <jens@beryju.org>
Signed-off-by: Aterfax <Aterfax@users.noreply.github.com>

---------

Signed-off-by: Aterfax <Aterfax@users.noreply.github.com>
Co-authored-by: Jens L. <jens@beryju.org>
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.

3 participants