Skip to content

Integrated OpenID Connect - #287

Open
sibios wants to merge 11 commits into
membermatters:devfrom
ctrlh:oidc-rp
Open

Integrated OpenID Connect#287
sibios wants to merge 11 commits into
membermatters:devfrom
ctrlh:oidc-rp

Conversation

@sibios

@sibios sibios commented Jan 1, 2025

Copy link
Copy Markdown

Implements #285 enabling MemberMatters to be an OpenID client

@sibios

sibios commented Jan 1, 2025

Copy link
Copy Markdown
Author

It looks like I messed up my version control and implemented this on top of the Slack integration branch (#286) instead of from my main fork. Sorry about that!

I did make certain design decisions based upon anticipated usage for my use case (CtrlH/PDX Hackerspace). For example, enabling OIDC_RP will prevent registration through Django (we don't presently plan on using MemberMatters as part of our member application process); in retrospect I probably should have implemented that behavior as a separate flag. Additionally, the changes I made to the frontend are pretty barebones to just enable use of OIDC. I'm sure that someone with proper design background could refactor this to make it more sensible.

@jabelone

jabelone commented Jan 11, 2025

Copy link
Copy Markdown
Member

Hey @sibios thanks for your patience. Sorry for the delay in getting back to this one. I wanted to get a new GitHub Actions workflow happening that allows us to automatically build and push a docker image for external PRs to make testing much easier. This was a bit of a pain to do "securely" as any untrusted contributor can open a PR with potentially malicious code to extract repo secrets etc.

Are you able to pull the latest from our dev branch back into the branch of this PR? I think this should be enough to trigger the new workflow. Thanks :D

Scratch that, it seems to have already worked it out. I'll do some testing and see how it goes. :)

@github-actions

Copy link
Copy Markdown

Created image with name membermatters/membermatters:untrusted-pr-oidc-rp. WARNING: run this image at your own risk - it was created from a potentially untrusted PR.

@sibios

sibios commented Jan 11, 2025

Copy link
Copy Markdown
Author

No worries! I did do some testing with our space's standing Authentik IDP instance, but have not exhaustively tested with other providers. As mentioned, I expect that I made some design decisions that would reasonably require refactoring.

@bhartshorn

Copy link
Copy Markdown

I'm looking forward to using this feature - I might use the temporary image for testing for now. Does this need rebased or anything now that the slack portion has been merged? Cc @sibios @jabelone

@sibios

sibios commented Jan 26, 2025

Copy link
Copy Markdown
Author

@bhartshorn I'll defer to @jabelone on whether rebasing is appropriate, but I think it should merge into dev cleanly. Please let me know if you run into any bugs with your testing as I only did a limited set of tests.

@bhartshorn

bhartshorn commented Feb 9, 2025

Copy link
Copy Markdown

Hi @sibios, @jabelone - another member of my group did some testing and found an issue or two. He posted on the feature request (I think by accident): #285 (comment)

Testing this image now. When a user who already exists in our SSO database logs into Membermatters, that works. However, the user's privileges cannot be changed in their User instance in the Admin console because the password field is blank and required.

As the admin, I can supply the user with a password, allowing me to submit the form and perform other tasks (elevating their privileges).

SSO users also don't seem to get valid Profiles. Clicking on the name in the Profile model raises RelatedObjectDoesNotExist "User has no profile."

Cc @dterkanian

@sibios

sibios commented Feb 9, 2025

Copy link
Copy Markdown
Author

Oop, thanks for that @bhartshorn! I had totally missed @dterkanian 's comment on the feature request thread! I will work on repro-ing, but can you provide your OIDC_* config settings from memberportal/membermatters/settings.py (minus sensitive bits)? As long as OIDC_CREATE_USER is set to true an User and minimal Profile should be created; but perhaps I failed to properly link the two... Presently the user doesn't get a password to prevent sign-ins (because we're using Idc for auth), but the most straight forward work around would probably be to populate the password field with some high-entropy junk at creation time.

I will get started on tweaks. Thank you for taking the time to test and report issues!! :)

@github-actions

github-actions Bot commented Feb 9, 2025

Copy link
Copy Markdown

Created image with name membermatters/membermatters:untrusted-pr-image-ctrlh-oidc-rp. WARNING: run this image at your own risk - it was created from a potentially untrusted PR.

@jabelone jabelone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ahhh I'm so sorry, I thought I had already hit "submit" but my comments were sitting here pending and invisible! Looks mostly good, just have a few changes requested for making config easier. I've also setup our own authentik instance so I can test properly.

Comment thread memberportal/membermatters/settings.py Outdated
Comment thread memberportal/membermatters/settings.py Outdated
Comment thread src-frontend/src/i18n/en-AU/index.ts Outdated
@sibios

sibios commented Feb 25, 2025

Copy link
Copy Markdown
Author

Ahhh I'm so sorry, I thought I had already hit "submit" but my comments were sitting here pending and invisible! Looks mostly good, just have a few changes requested for making config easier. I've also setup our own authentik instance so I can test properly.

No worries! All variables related to this feature are now available via environment variables, documentation updated accordingly, and reworded phrasing on the registration lock-out message.

@github-actions

Copy link
Copy Markdown

Created image with name membermatters/membermatters:untrusted-pr-image-ctrlh-oidc-rp. WARNING: run this image at your own risk - it was created from a potentially untrusted PR.

@sibios
sibios requested a review from jabelone March 18, 2025 00:02
@github-actions

Copy link
Copy Markdown

Created image with name membermatters/membermatters:untrusted-pr-image-ctrlh-oidc-rp. WARNING: run this image at your own risk - it was created from a potentially untrusted PR.

@github-actions

Copy link
Copy Markdown

Created image with name membermatters/membermatters:untrusted-pr-image-ctrlh-oidc-rp. WARNING: run this image at your own risk - it was created from a potentially untrusted PR.

@G-Pereira

G-Pereira commented May 13, 2026

Copy link
Copy Markdown

Hi @sibios, is this in a good state or does it need a refactoring to be mergeable? I can help. At LCD we need this as well.

@G-Pereira

Copy link
Copy Markdown

Just filed a PR to the fork to fix this in order to work with Authentik! ctrlh#2

@G-Pereira

Copy link
Copy Markdown

@jabelone I see you requested some changes that were already introduced. Can we revisit this to get into a merge-able state?

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.

4 participants