-
Notifications
You must be signed in to change notification settings - Fork 13
Feature/1182 collaborator api communication with keycloak #1184
base: master
Are you sure you want to change the base?
Feature/1182 collaborator api communication with keycloak #1184
Conversation
jenkins build please |
Das ist leider nach meinem Erkenntnisstand nicht möglich. Die Admin REST API erfordert einen Login über den Admin Account. Das hab ich nur mit den variablen die ich definiert habe geschafft.
Das verstehe ich nicht. Die role (member) kommt aus dem Token. Die anderen user kommen über ein request an die admin rest api. Man kann wsl auch die role mit der Admin api abfragen und dann filtern was als Suchergebnis zurück kommt. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work! ❤️
Some suggestions how I would prefer the keycloak config to be but I didn't check if keycloak actually provides this possibility. So maybe the current way already is the best. 🚀
"owner" renames etc. could also be in follow up (and is probably even better).
Please always try to say why something is draft or what is open, it helps in a review.
Das ist leider nach meinem Erkenntnisstand nicht möglich. Die Admin REST API erfordert einen Login über den Admin Account. Das hab ich nur mit den variablen die ich definiert habe geschafft.
Thx for clarification.
Damit könnte man dann auch steuern, dass nur Members andere Members sehen können (und nicht die Gäste).
Das verstehe ich nicht. Die role (member) kommt aus dem Token. Die anderen user kommen über ein request an die admin rest api. Man kann wsl auch die role mit der Admin api abfragen und dann filtern was als Suchergebnis zurück kommt.
Yes, looks like you already do what I meant. I meant that the person who is requesting to search for others should be restricted if (s)he is not a member.
@markus2330 I think this PR is ready now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just reviewed this PR and it overall it looks solid to me 👍
One problem is it breaks the devcontainer (missing the new AUTH_HOST env var)
Also had some minor suggestions / thoughts.
} | ||
|
||
futures | ||
// TODO: restrict concurrency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we create a ticket for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I do not know if it is problematic to potentially send an unbounded amount of update queries to the DBMS.
We could open a discussion in the diesel_async
repo and ask for guidance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just reviewed this PR and it overall it looks solid to me 👍
Thanks for the review. Nice catches :)
@lukashartl and I will try tomorrow to fix the env vars and do the Keycloak setup. |
Ping me if you need assistance. |
@markus2330 |
2068e67
to
0a25c97
Compare
I'll start testing when also frontend is ready. We can merge this when @lukashartl has fixed our keycloak+infrastructure to accept the env vars. We probably even need 3 steps:
|
Ok. I have the frontend ready on a separate branch that is based on this branch.
Alright, I also have a fix for #755, the first two of #1093 and #480. |
0a25c97
to
ec5e929
Compare
jenkins build please |
Keycloak was now reconfigured for this PR, let us see if the build succeeds. |
Failed with:
|
@markus2330 |
Basics
close #X
, are in the commit messages and changelogChecklist
First Time Checklist
Review