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

[WIP] new Keycloak Login Provider Plugin via OIDC #822

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lukas-staab
Copy link
Contributor

I have build a keycloak OpenID Connect Login Plugin. It works, but I have several questions for coding style. I comment on the code snippets for it.

@@ -28,6 +28,7 @@
"colinodell/json5": "^2.3",
"doctrine/annotations": "^1.14.3",
"guzzlehttp/guzzle": "^7.7",
"jumbojett/openid-connect-php": "^0.9.10",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it ok to require the OIDC Libary via composer? The Simplesaml Class from other Plugins is not required.

Copy link
Owner

Choose a reason for hiding this comment

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

I would rather not put it there. I try to only keep the dependencies in the main composer.json that will be part of the main distribution via ZIP file, without any plugins. So I don't want to put extra weight in there for this, the SAML or Redis parts. On the servers with those plugins, I do take some extra steps therefore, by manually installing yiisoft/yii2-redis predis/predis simplesamlphp/simplesamlphp after updating the dependencies.

Comment on lines +21 to +23
'https://keycloak.domain.com',
'antragsgruen.domain.com',
'supderdupersecret'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would love to set these credentials in the config.json - in the Admin panels would be ok too but if this would ever be the only way to log in (is this even possible right now?) then it would difficult to change these values if ever needed.

Copy link
Owner

Choose a reason for hiding this comment

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

Yea, I don't have a really good way of storing plugin-specific credentials. For the discourse-plugin (✝) I put the configuration in a separate file next to the config.json : https://github.com/CatoTH/antragsgruen/blob/main/plugins/discourse/Module.php#L31

Using the admin panel would work too, using something like https://github.com/CatoTH/antragsgruen/blob/main/plugins/member_petitions/Module.php#L65 , but might be an overkill there

@CatoTH CatoTH mentioned this pull request Oct 29, 2023
@CatoTH
Copy link
Owner

CatoTH commented Nov 11, 2023

Hi,
did you want to work more on the PR? Like not requiring the oidc-library in the main composer.json?

@lukas-staab
Copy link
Contributor Author

Yes, I will. But right now I was not able to find some time to work on it again :/

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.

2 participants