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

[Enhancement]: Add group syncing to Discourse SSO #345

Open
1 task done
MatthewL246 opened this issue Oct 5, 2024 · 10 comments · May be fixed by #363
Open
1 task done

[Enhancement]: Add group syncing to Discourse SSO #345

MatthewL246 opened this issue Oct 5, 2024 · 10 comments · May be fixed by #363
Labels
approved The topic is approved by a developer enhancement An update to an existing part of the codebase

Comments

@MatthewL246
Copy link
Member

Checked Existing

  • I have checked the repository for duplicate issues.

What enhancement would you like to see?

It would be convenient to have groups on the forum that are automatically synced based on account state.

This would include:

  • Developer (access level 3)
  • Juxtaposition moderator (access level 2)
  • Tester/supporter (access level 1)

It may also be possible to check if the PNID's linked Discord account has a moderator role in the server, which could be used to create a Discord moderator group.

Any other details to share? (OPTIONAL)

DiscourseConnect has support for adding and removing groups with SSO. It would probably be best to use the add_groups and remove_groups attributes instead of the discourse connect overrides groups setting, as that would break forum-only groups like category moderators.

@MatthewL246 MatthewL246 added enhancement An update to an existing part of the codebase awaiting-approval Topic has not been approved or denied approved The topic is approved by a developer and removed awaiting-approval Topic has not been approved or denied labels Oct 5, 2024
@jonbarrow
Copy link
Member

Does this make sense as part of the SSO or as something we do via the API? The only flaw I see with this is that group syncing only happens during authentication, meaning that for someone to update their groups they would need to login again. I can see this causing some friction if someone's group needs to change while they are logged in (forcing them to logout and back in), and also if someone is logged out and their group changes (so, an admin being demoted), then it will look like they are still in said group(s) until they login again

@jonbarrow
Copy link
Member

Also does Discourse support purely visual groups? Or do they have to be tied to an access level. It might be nice to have groups for stuff like donor status, like we have on the website

@MatthewL246
Copy link
Member Author

MatthewL246 commented Oct 7, 2024

The only flaw I see with this is that group syncing only happens during authentication, meaning that for someone to update their groups they would need to login again.

That's definitely a concern. The main pro to this method is that it would be very easy to implement. This could be mitigated by reducing the session timeout, but of course, making all users need to log in more often is just annoying. I agree that handling this by calling the Discourse API when the subscription status changes sounds more reasonable and addresses the flaw better than that. Handling it in SSO would still be necessary for current supporters who aren't changing their subscription status, though.

Handling developer/moderator status changes sounds like it would need to be in the admin panel?

Also does Discourse support purely visual groups?

Yes, you can create as many visual groups as you want without any ties to permissions. Donation tier group(s) could be added in addition to the tester/supporter group I mentioned. Groups also have some interesting visual features, like allowing group members to set the group name as their title or putting a small icon of the group logo next to their username.

@ExperiencersInternational
Copy link
Contributor

Yes, you can create as many visual groups as you want without any ties to permissions. Donation tier group(s) could be added in addition to the tester/supporter group I mentioned. Groups also have some interesting visual features, like allowing group members to set the group name as their title or putting a small icon of the group logo next to their username.

Sorry I haven't seen this thread yet. I definitely agree with the idea to sync groups depending on role on website since it isn't clear for things like ban appeals that a user might be a staff member.

I don't really see an issue with syncing during authentication, just get the user to sign out and back in. That's already the case for updating profile pictures and it isn't really a hassle.

@MatthewL246
Copy link
Member Author

MatthewL246 commented Oct 13, 2024

I don't really see an issue with syncing during authentication, just get the user to sign out and back in.

Personally, my main concern is more about people purposefully exploiting this fact to keep themselves in a group they shouldn't be in. For example, a dev/mod who has been demoted or a supporter who canceled their subscription.

@ExperiencersInternational
Copy link
Contributor

I don't really see an issue with syncing during authentication, just get the user to sign out and back in.

Personally, my main concern is more about people purposefully exploiting this fact to keep themselves in a group they shouldn't be in. For example, a dev/mod who has been demoted or a supporter who canceled their subscription.

Ah of course, I didn't consider that possibility to be honest. Yeah, I completely agree with you here.

@MatthewL246
Copy link
Member Author

MatthewL246 commented Nov 9, 2024

After reading the DiscourseConnect setup guide, I see a couple of potential solutions.

Manually syncing SSO

There is an API endpoint specifically for syncing SSO. This is pretty self-explanatory: send an API request with the SSO payload to manually sync it to the user.

Logging users out

Logging off users

You can use the POST admin endpoint /admin/users/{USER_ID}/log_out to log out any user in the system if needed.

To configure the endpoint Discourse redirects to on logout search for the logout redirect setting. If no URL has been set here you will be redirected back to the URL configured in discourse connect url.

Search users by external_id

User profile data can be accessed using the /users/by-external/{EXTERNAL_ID}.json endpoint. This will return a JSON payload that contains the user information, including the user_id which can be used with the log_out endpoint.

So, the website or admin panel could forcibly log out a user whenever they are removed from a sensitive group. This might be a simpler solution because it would prevent duplication of group-membership-handling code into multiple repos. All membership checks could be handled in the SSO login here, while everything else would just be a simple single API call to the log_out endpoint.

MatthewL246 added a commit that referenced this issue Dec 9, 2024
@MatthewL246
Copy link
Member Author

MatthewL246 commented Dec 10, 2024

Since we have started to make more of an effort to separate different moderator types on Discord, this should also carry over to the forum. This means adding a few more groups and updating the group syncing logic.

  • Juxtaposition mods can be from the PNID's access level
  • Discord mods can be from the moderator role IDs, which should be possible using the existing Discord connection that's used to add supporter/tester roles
  • Network mods can use the same Discord role IDs method
  • I don't think it's necessary add the Discord VC mods

Additionally, the subscription tier can be selected using pnid.connections.stripe.tier_name or pnid.connections.stripe.tier_level.

@ExperiencersInternational
Copy link
Contributor

ExperiencersInternational commented Dec 10, 2024

  • I don't think it's necessary add the Discord VC mods

Regarding Discord VC mods, I agree with you to not add it but a generic Pretendo staff group could work too for that (and any potential future additions)

@MatthewL246
Copy link
Member Author

I'm not sure what the use of that would be. Being "staff" in general is less important than what specifically a staff member has control over, and I think it would be a little confusing to have someone who is only "generic staff." It could definitely be added if there was a specific use for it.

@MatthewL246 MatthewL246 moved this from Todo to In Progress in Pretendo Tasks and Goals Dec 12, 2024
@MatthewL246 MatthewL246 linked a pull request Dec 12, 2024 that will close this issue
4 tasks
@MatthewL246 MatthewL246 linked a pull request Dec 12, 2024 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved The topic is approved by a developer enhancement An update to an existing part of the codebase
Projects
Status: In Progress
Development

Successfully merging a pull request may close this issue.

3 participants