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

OIDC/Auth2 integration #18

Open
wants to merge 3 commits into
base: feature/keycloak-oidc
Choose a base branch
from

Conversation

boehlke
Copy link

@boehlke boehlke commented Sep 27, 2024

This PR contains a POC for a OIDC/oauth2 integration into OpenSlides. The dev setup is based on keycloak.

entrypoint Outdated
Comment on lines 22 to 23
KEYCLOAK_HOST="${KEYCLOAK_URL:-keycloak}" \
KEYCLOAK_PORT="${KEYCLOAK_URL:-8080}" \
Copy link
Member

Choose a reason for hiding this comment

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

This is not consistent with what's being evaluated in caddy_base.json

Comment on lines +10 to +18
# remove endpoints if not set
for ENDPOINT in ACTION_HOST PRESENTER_HOST AUTOUPDATE_HOST ICC_HOST SEARCH_HOST MEDIA_HOST MANAGE_HOST CLIENT_HOST VOTE_HOST KEYCLOAK_HOST; do
if [ -z "${!ENDPOINT}" ]; then
echo "Removing endpoint $ENDPOINT"
jq ".apps.http.servers.srv0.routes |= map(select(.handle | any(.upstreams[]?.dial | contains(\"$ENDPOINT\") | not)))" "$base" > "$base.out"
mv -f "$base.out" "$base"
fi
done

Copy link
Member

Choose a reason for hiding this comment

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

I see this construct can be helpful.

I see the following risk:
Until now this entrypoint script ensured that all endpoint env vars are set (at least to defaults). Now if an admin of OpenSlides somehow misses setting one of these, the service suddenly is no longer reachable. This can be desired, but should be documented maybe here.

Checking if the manage tool actually does set all these variables to defaults: no it does not! See default config. So this must be adjusted to include CLIENT_HOST and CLIENT_PORT or suddenly the client won't be proxied anymore.

Personally I'd also prefer a [ -z "${ENDPOINT}" ] check (without '!'). I think it is less confusing as the '!' expansion is not as widely known. Also it would enable one to override some SERVICE_HOST= without needing to actually unset it to disable the service. This would provide more flexibility IMO.

Comment on lines +21 to +26
for var in $(grep -o '\$[A-Za-z_][A-Za-z0-9_]*' $config | sort | uniq | sed 's/\$//'); do
echo "Checking variable $var"
if [[ -z "${!var}" ]]; then
echo "Variable $var is not set"
fi
done
Copy link
Member

Choose a reason for hiding this comment

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

This is just a redundant check to be sure no unset variables survived, right?
This seems fine though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants