Allow choosing the OIDC remote user claim and scopes to request from the identity provider#5481
Conversation
|
Follow-up of #5351 |
e2a8890 to
91a7f75
Compare
|
Alright, I cleaned up my commits a little, and finally got something to work for using default values for unset environment variables. The end-result may not be very pretty, if anyone more knowledgeable than me has suggestions to make this better, I'm all ears! |
Docker/FreshRSS.Apache.conf
Outdated
| # Workaround to be able to check whether an environment variable is set | ||
| # See: https://serverfault.com/questions/1022233/using-ifdefine-with-environment-variables/1022234#1022234 | ||
| Define VStart "${" | ||
| Define VEnd "}" | ||
|
|
There was a problem hiding this comment.
Can that be put inside the <IfDefine OIDC_ENABLED> block?
There was a problem hiding this comment.
It certainly can. I put those there because I considered using the same trick to get rid of the -D OIDC_ENABLED in the Dockerfiles, but decided not to so it wouldn't further "pollute" the PR. I can still do either (move these Define into the OIDC_ENABLED conditional, or use the trick to get rid of the conditional in the Dockerfiles). I'll leave that to you.
There was a problem hiding this comment.
I have not fully understood your response :-)
If possible, I would suggest the following:
<IfDefine OIDC_ENABLED>
<IfModule !auth_openidc_module>
Error "The auth_openidc_module is not available. Install it or unset environment variable OIDC_ENABLED."
</IfModule>
# Workaround to be able to check whether an OS environment variable is set
# https://serverfault.com/questions/1022233/using-ifdefine-with-environment-variables/1022234#1022234
Define VStart "${"
Define VEnd "}"
OIDCProviderMetadataURL ${OIDC_PROVIDER_METADATA_URL}
OIDCClientID ${OIDC_CLIENT_ID}
OIDCClientSecret ${OIDC_CLIENT_SECRET}
OIDCRedirectURI /i/oidc/
OIDCCryptoPassphrase ${OIDC_CLIENT_CRYPTO_KEY}
Define "Test_${OIDC_REMOTE_USER_CLAIM}"
<IfDefine Test_${VStart}OIDC_REMOTE_USER_CLAIM${VEnd}>
OIDCRemoteUserClaim preferred_username
</IfDefine>
<IfDefine !Test_${VStart}OIDC_REMOTE_USER_CLAIM${VEnd}>
OIDCRemoteUserClaim "${OIDC_REMOTE_USER_CLAIM}"
</IfDefine>
Define "Test_${OIDC_SCOPES}"
<IfDefine Test_${VStart}OIDC_SCOPES${VEnd}>
OIDCScope openid
</IfDefine>
<IfDefine !Test_${VStart}OIDC_SCOPES${VEnd}>
OIDCScope "${OIDC_SCOPES}"
</IfDefine>
OIDCRefreshAccessTokenBeforeExpiry 30
</IfDefine>There was a problem hiding this comment.
I have not fully understood your response :-)
Apologies, I don't know how best to express this without actually showing what I mean in a diff. But I was referring to this:
Line 68 in 666e951
The OIDC_ENABLED variable is defined on the commandline inside each of the Dockerfiles, and it doesn't need to be if we use the same pattern for checking whether an environment variable is set or not.
I applied your suggestion anyway. Let me know if there's anything else 👍
There was a problem hiding this comment.
This OIDC_ENABLED variable is special in the sense that, as far as I can see, it is the only one that changes the Apache command line (we want to add a -D OIDC_ENABLED), so I do not believe it is the same situation than the other environment variables
There was a problem hiding this comment.
But why do you want to add a -D OIDC_ENABLED? Instead (and I'm not saying this is winning any beauty contests, but still), it could've been done like so (without the -D OIDC_ENABLED):
Define VStart "${"
Define VEnd "}"
Define "Test_${OIDC_ENABLED}"
<IfDefine !Test_${VStart}OIDC_ENABLED${VEnd}>
# [...]
</IfDefine>
IMO, Apache's config doesn't lend itself well to this kind of runtime logic. Maybe some kind of templating solution would be better, but then you'd have one more moving piece in your docker image. I'm not sure what the best solution would be, honestly. What you have now works fine, so just keep that if you want 🙂
…the identity provider
…side IfDefine block
7a61ce4 to
ab15057
Compare
|
Looks all good to me now 👍🏻 |
|
@otaconix Please add a line for you in https://github.com/FreshRSS/FreshRSS/blob/edge/CREDITS.md |
Changes proposed in this pull request:
OIDC_SCOPESenvironment variable to control what scopes to request from the OIDC identity provider. This defaults toopenid.OIDC_REMOTE_USER_CLAIMenvironment to control what claim to use as the username. This defaults topreferred_username.How to test the feature manually:
Docker/Dockerfile) to use Authentik as the OIDC identity provider. Use the following environment variables besides the ones needed to get OIDC in FreshRSS:OIDC_SCOPES=openid profile(profileis required to get thepreferred_usernameclaim)Pull request checklist: