Skip to content

Conversation

@sibios
Copy link

@sibios sibios commented Jan 1, 2025

Implements #285 enabling MemberMatters to be an OpenID client

@sibios
Copy link
Author

sibios commented Jan 1, 2025

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
Copy link
Member

jabelone commented Jan 11, 2025

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

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
Copy link
Author

sibios commented Jan 11, 2025

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

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
Copy link
Author

sibios commented Jan 26, 2025

@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
Copy link

bhartshorn commented Feb 9, 2025

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
Copy link
Author

sibios commented Feb 9, 2025

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
Copy link

github-actions bot commented Feb 9, 2025

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.

Copy link
Member

@jabelone jabelone left a comment

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.

@sibios
Copy link
Author

sibios commented Feb 25, 2025

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

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

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

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.

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