Add LDAP integration #509
Add LDAP integration #509
Conversation
… user.ldap.groupattribute
|
Hi! Thanks for the PR. (and the 2 others) This looks like a lot of work! Related to issue #171 |
Adapted the reolution of the ldap groups to not only work with the example ldap from forumsys but also with a freeIpa setup.
|
Hi XeR, currently reviewing the patches from Ceirced and working on improving error handling, bug fixes, and better integration of mattermost channels as well as gitlab repos. Cheers, |
Minei3oat
left a comment
Minei3oat
left a comment
There was a problem hiding this comment.
You should probably try to align your function/variable/... names better with the existing code base.
PS: I'm no maintainer, so feel free to ignore my feedback
| return "user_member"; | ||
| } | ||
| } | ||
|
|
| // Explicitly specify algorithm to prevent algorithm confusion attacks | ||
| const signedJwt = jwt.sign(jwtPayload, jwtSecret, { | ||
| algorithm: "HS256", | ||
| }); |
There was a problem hiding this comment.
If SESSION_SECRET is too short, CTFNote will generate a new secret on every start, but your jwtSecret will be the too short value. As a result, the JWT generated here will fail validation with invalid signature.
If SESSION_SECRET is not set, jwtSecret is empty, resulting in an error during signing:
Error: secretOrPrivateKey must have a value
|
|
||
| try { | ||
| // Additional check to ensure the user is recognized as logged in | ||
| const loginCheck = await isLogged(); |
There was a problem hiding this comment.
This returns false since there is no data in cache.
| jwt | ||
| } | ||
| } | ||
| `; |
There was a problem hiding this comment.
This mutation should be defined in /front/src/graphql/Auth.graphql
| LDAP_GROUP_ATTRIBUTE=ou | ||
| LDAP_ADMIN_GROUPS=mathematicians | ||
| LDAP_MANAGER_GROUPS=scientists | ||
| LDAP_USER_GROUPS=chemists |
There was a problem hiding this comment.
You should either use singular or support specifying multiple groups
| # Role Mapping | ||
| LDAP_DEFAULT_ROLE=user_guest | ||
| LDAP_MATHEMATICIANS_ROLE=user_member | ||
| LDAP_SCIENTISTS_ROLE=user_member |
There was a problem hiding this comment.
This file seems to be for an earlier version of this PR and seems wrong for the current version
There was a problem hiding this comment.
This is likely relevant for docker, so you should probably keep it
There was a problem hiding this comment.
This is likely relevant for local development/build on linux, so you should probably keep it
| query getAuthSettings { | ||
| localAuthEnabled | ||
| ldapAuthEnabled | ||
| } |
There was a problem hiding this comment.
This query should probably be moved into /front/src/graphql/Settings.graphql
| } | ||
| `; | ||
|
|
||
| export function useLdapLogin() { |
There was a problem hiding this comment.
After moving the graphql to the graphql file, you can just use similar code as useLogin.
| -- Add LDAP user authentication function | ||
| -- This function will be called from the LDAP plugin to handle user creation/update | ||
|
|
||
| CREATE OR REPLACE FUNCTION ctfnote.login_ldap( |
There was a problem hiding this comment.
This must be a private function, because if it is public, I can use it to request a JWT for arbitrary (non-existing) users with arbitrary roles:
[{"operationName":"LoginLdap","variables":{"username":"test","userRole":"USER_ADMIN"},"query":"mutation LoginLdap($username: String!, $userRole: Role!) {\n loginLdap(input: {username: $username, userRole: $userRole}) {\n jwt\n __typename\n }\n}"}]4 participants
Use env variable to enable ldap integration.
Working config with forumsys ldap.
Add env varibale to disable local auth.