feat: Implement auth-keycloak plugin #188
Conversation
|
@fumblehool Thank you. We'll review your PR soon |
|
hi, @abrl91 please review the PR 😄 |
plugins/auth-keycloak/README.md
Outdated
|
|
||
| ## Prerequisites | ||
|
|
||
| - Running [Keycloak](https://www.keycloak.org/) instance. You can use the `docker-compose.dev.yaml`` file dor dev container. |
There was a problem hiding this comment.
| - Running [Keycloak](https://www.keycloak.org/) instance. You can use the `docker-compose.dev.yaml`` file dor dev container. | |
| - Running [Keycloak](https://www.keycloak.org/) instance. You can use the `docker-compose.dev.yml`` file or dev container. |
Also the docker-compose.dev.yml is created after the plugin is run. Due to this settings regarding keycloak cannot be provided before that leading to circular dependency.
| - `KEYCLOAK_REALM`: Name of the realm to be used for authentication (or the one you just created) | ||
| - `KEYCLOAK_CLIENT_ID`: Client ID of the client to be used for authentication (or the client we just created) | ||
| - `KEYCLOAK_CLIENT_SECRET`: Client Secret of the client to be used for authentication. |
There was a problem hiding this comment.
These can only be provided after the keycloak is setup right?
There was a problem hiding this comment.
Right. These are manually created by Admin.
Keycloak allows admin to create multiple realms and in order to connect, these are required.
There was a problem hiding this comment.
But this is needed in plugin, so I suggest to let the users start up keycloak themselves by using a docker-compose file provided by you. Maybe can include the steps in README. Then they can provide these values to plugin.
| @@ -0,0 +1,33 @@ | |||
| import { ConfigService } from "@nestjs/config"; | |||
| import { PassportStrategy } from "@nestjs/passport"; | |||
| import { KeycloakStrategy } from "@exlinc/keycloak-passport"; | |||
There was a problem hiding this comment.
Not added in created package.json.
There was a problem hiding this comment.
Also, this is creating multiple problems as it doesn't have types and needs a declaration file.
There was a problem hiding this comment.
Some fields like authorizationURL, tokenURL are missing. Could you look into this?
|
@fumblehool are you working on the comments from @Shurtu-gal? |
|
Hi @mulygottlieb, |
fumblehool
force-pushed
the
fumblehool/feat/plugin-keycloak-auth
branch
from
01dc257 to
2c3edb4
Compare
fumblehool
force-pushed
the
fumblehool/feat/plugin-keycloak-auth
branch
from
2c3edb4 to
0810f2a
Compare
|
@fumblehool as we added some automation to this repo, it would be a great idea to merge the latest |
There was a problem hiding this comment.
Hi @fumblehool , how are you ? thank you for the plugin. I started my review and I noticed that your plugin demands changes on auth-jwt plugin. as a rule of thumb, we would like to keep the plugins decoupled one from another. the reason is that each plugin should function on a specific level of logic without the knowledge of the rest of the changes. so, in order to continue with this plugin, we need to remove the changes on auth-jwt and specify them on auth-keycloak plugin. we design the process that it enable you the access to all files, so you can take the related file and change it so it will fits your needs.
|
@fumblehool any update? |
PR for Keycloak-Auth-Plugin - amplication/amplication#7044