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

Add client authz policies #221

Closed
wants to merge 26 commits into from

Conversation

yspotts
Copy link
Contributor

@yspotts yspotts commented Feb 12, 2020

This PR adds all the client authorization policies keycloak provides (except for rule based which is deprecated).

Copy link
Contributor

@mrparkers mrparkers left a comment

Choose a reason for hiding this comment

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

Wow, this is a lot of new resources! Thanks for the contribution!

Before this gets merged, I'd like to have at least one acceptance test for each of these new resources that tests that some basic configuration works as expected. I'd also appreciate it if you could add some example HCL for these in examples/main.tf.

I won't force you to write docs for these, but that would be awesome if you wanted to take the time to do that. If not, I can try and get them backfilled at some point later.

@chopshop1
Copy link

I'll take over the tests!

@chopshop1
Copy link

@mrparkers I just finished the tests for the new providers, and they all pass locally.
But for some reason the JS Policy is failing on the pipeline. We are having trouble figuring this out.
Could you please help us diagnose this failure.

@yspotts
Copy link
Contributor Author

yspotts commented Feb 28, 2020

@mrparkers @Amad27 I was able to get more info from the server response. Here is the error:

"error":"Script upload is disabled"

This is because JS policies are in feature preview and thus need to be enabled when keycloak runs. We do run that way in production, however, the image in the test pipeline does not (as that is the default). Would you prefer that we run with preview features on, or comment out the tests for the JS policy?

@chopshop1 chopshop1 force-pushed the add-client-authz-policies branch from f62a37a to a159296 Compare March 3, 2020 16:00
@chopshop1
Copy link

@yspotts @mrparkers
I got the tests to pass in CircleCi!
just needed to toggle -Dkeycloak.profile.feature.upload_scripts

@chopshop1
Copy link

Examples are up too! @mrparkers could you please take another look at this pr when you get a chance.

@mrparkers
Copy link
Contributor

Thanks for adding the tests! I don't have a problem with passing that flag to keycloak during tests, nice work.

I'm trying to look at the diff that GitHub is showing me, but a lot of the changes that are showing are already in master, and it's making this difficult to review. Would you be able to fix the git history for this branch, or make a new branch and cherry-pick your commits to it so the diff only displays what files were changed?

Thanks!

@chopshop1
Copy link

@mrparkers i'm making a new pr to remove the merged code to make it easier to read

@chopshop1
Copy link

@mrparkers just moved it to a new pr hopefully that will make it easier to read.
#246

@mrparkers
Copy link
Contributor

Closed via #246

@mrparkers mrparkers closed this Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants