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

Add support for oidc #389

Merged
merged 9 commits into from Mar 16, 2024
Merged

Add support for oidc #389

merged 9 commits into from Mar 16, 2024

Conversation

Nighmared
Copy link
Contributor

@Nighmared Nighmared commented Jan 8, 2023

These changes add support for OIDC authentication using mozilla-django-oidc

Fixes #177

While this isn't merged, there is an image at https://hub.docker.com/r/nighmared/linkding-oidc :)

image

@AlphaJack
Copy link

This configuration worked for my Authentik provider:

LD_ENABLE_OIDC=True
OIDC_RP_SIGN_ALGO=HS256
OIDC_OP_JWKS_ENDPOINT=https://auth.example.org/application/o/linkding/jwks/
OIDC_OP_AUTHORIZATION_ENDPOINT=https://auth.example.org/application/o/authorize/
OIDC_OP_TOKEN_ENDPOINT=https://auth.example.org/application/o/token/
OIDC_OP_USER_ENDPOINT=https://auth.example.org/application/o/userinfo/
OIDC_RP_CLIENT_ID=XXXXXX
OIDC_RP_CLIENT_SECRET=YYYYYY

The "change password" link is still visible after a OIDC login, should it be removed in this case?

@Nighmared
Copy link
Contributor Author

Nighmared commented Jan 17, 2023

Allowing pw change for users logged in with oidc does seem unintuitive, I will take another look at this sometime soon-ish

@AlphaJack
Copy link

In the shared bookmark page, I see a gibberish user name when I filter per user.
I have the same SSO setup with Miniflux and it receives my user email as user name from my IDP, so maybe something can be done in that regard in our case?

@Nighmared
Copy link
Contributor Author

That seems viable. I'll make sure to look into this, thanks a lot for your review!

Disclaimer: I'll only have time to take care of the issues in about 3 weeks.

@Nimamoh
Copy link

Nimamoh commented May 29, 2023

I'd love to see this merged. is it possible ?

@Nighmared
Copy link
Contributor Author

I've been on vacation since march and didn't have time to look into the issues brought up. I might have some time to clean this up in the next couple days or at the very latest in September...

So until its cleaned up probably not ready to merge

@dnnp362
Copy link

dnnp362 commented Oct 24, 2023

Hi!,I'm very interested in this feature, any news?

@Eamourinho
Copy link

I made a modified version of the patch that doesn't have the conflicts with the current version but am currently getting a 500 error, the stack trace below:

2023-12-01 01:32:07,640 ERROR Internal Server Error: /oidc/callback/
Traceback (most recent call last):
  File "/opt/venv/lib/python3.10/site-packages/django/core/handlers/exception.py", line 56, in inner
    response = get_response(request)
  File "/opt/venv/lib/python3.10/site-packages/django/core/handlers/base.py", line 197, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/opt/venv/lib/python3.10/site-packages/django/views/generic/base.py", line 103, in view
    return self.dispatch(request, *args, **kwargs)
  File "/opt/venv/lib/python3.10/site-packages/django/views/generic/base.py", line 142, in dispatch
    return handler(request, *args, **kwargs)
  File "/opt/venv/lib/python3.10/site-packages/mozilla_django_oidc/views.py", line 130, in get
    return self.login_success()
  File "/opt/venv/lib/python3.10/site-packages/mozilla_django_oidc/views.py", line 60, in login_success
    auth.login(self.request, self.user)
  File "/opt/venv/lib/python3.10/site-packages/django/contrib/auth/__init__.py", line 144, in login
    user_logged_in.send(sender=user.__class__, request=request, user=user)
  File "/opt/venv/lib/python3.10/site-packages/django/dispatch/dispatcher.py", line 176, in send
    return [
  File "/opt/venv/lib/python3.10/site-packages/django/dispatch/dispatcher.py", line 177, in <listcomp>
    (receiver, receiver(signal=self, sender=sender, **named))
  File "/etc/linkding/./bookmarks/signals.py", line 11, in user_logged_in
    tasks.schedule_bookmarks_without_snapshots(user)
  File "/etc/linkding/./bookmarks/services/tasks.py", line 98, in schedule_bookmarks_without_snapshots
    if is_web_archive_integration_active(user):
  File "/etc/linkding/./bookmarks/services/tasks.py", line 22, in is_web_archive_integration_active
    user.profile.web_archive_integration == UserProfile.WEB_ARCHIVE_INTEGRATION_ENABLED
  File "/opt/venv/lib/python3.10/site-packages/django/db/models/fields/related_descriptors.py", line 463, in __get__
    raise self.RelatedObjectDoesNotExist(
django.contrib.auth.models.User.profile.RelatedObjectDoesNotExist: User has no profile.

... any ideas? Would love to get this working to have one more app in my arsenal locked behind Authentik ':D

@Nighmared
Copy link
Contributor Author

Nighmared commented Dec 19, 2023

Todo:

  • verify it actually works after merge
  • hide "change password" link for oidc users
  • fix name of oidc users

@Nighmared
Copy link
Contributor Author

Nighmared commented Dec 19, 2023

Does anyone have an idea on how to detect whether the current user logged in via oidc?
I don't really have the time for a deep dive on this and it would be very useful for deciding whether "change password" should be shown or not :)

@Nighmared
Copy link
Contributor Author

I think this is ready for review
@sissbruecker @AlphaJack

# Conflicts:
#	bookmarks/views/settings.py
#	requirements.prod.txt
#	requirements.txt
#	siteroot/settings/base.py
#	siteroot/urls.py
@sissbruecker
Copy link
Owner

Taking a look at this now, will rebase and push some changes shortly.

@sissbruecker
Copy link
Owner

Made some changes:

  • Based on the discussion in Code authorization flow with PKCE mozilla/mozilla-django-oidc#397 and testing with Zitadel, which shows using a client secret as "not recommended", I've enabled PKCE auth as the default instead of using a client secret. Users can still disable PKCE and configure a client secret if they want to.
  • Added HS256 as the default sign algo
  • Removed the logic for conditionally hiding the password change link. It looked a bit sketchy and I don't think showing this is a big deal. There's also nothing stopping you from setting a password for a user in the admin UI, at which point you could also change it.

A basic configuration for Zitadel should now look something like this, assuming that PKCE is used:

LD_ENABLE_OIDC=True
OIDC_OP_AUTHORIZATION_ENDPOINT=http://localhost:8080/oauth/v2/authorize
OIDC_OP_TOKEN_ENDPOINT=http://localhost:8080/oauth/v2/token
OIDC_OP_USER_ENDPOINT=http://localhost:8080/oidc/v1/userinfo
OIDC_RP_CLIENT_ID=258574559115018243@linkding

@sissbruecker sissbruecker merged commit 39782e7 into sissbruecker:master Mar 16, 2024
2 checks passed
@helmut72
Copy link

Removed the logic for conditionally hiding the password change link

This also make sense for header based authentication.

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.

OIDC/SSO support in general?
7 participants