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

Remove python-keycloak #352

Merged
merged 20 commits into from
Mar 7, 2024
Merged

Remove python-keycloak #352

merged 20 commits into from
Mar 7, 2024

Conversation

FedericoNegri
Copy link
Contributor

@FedericoNegri FedericoNegri commented Mar 5, 2024

Background:

  • python-keycloak versions < 3.9.1 depend on python-jose which in turn depends on ecdsa, which has safety vulnerabilities
  • because of the above, python-keycloak >= 3.9.1 replaced python-jose with jwcrypto which is LGPL.

I propose to get rid of python-keycloak as a PyHPS dependency (we'd only keep using it for tests). The package was only used in the AuthApi as a convenience to query/create/modify/delete users as well as query their groups and roles.

Since by default HPS users do not have the realm management manage-users role, it seems just misleading to expose the create/modify/delete users endpoints. I therefore removed those and rather added an example in the doc showing how to use the KeycloakAdmin client from python-keycloak to do so.

Nightly build run with Python 3.9 - 3.12 tests: https://github.com/ansys/pyhps/actions/runs/8158129451

Breaking change

ansys.hps.client.auth.authenticate moved to ansys.hps.client.authenticate to avoid circular dependencies

@github-actions github-actions bot added documentation Improvements or additions to documentation dependencies Related with project dependencies maintenance Package and maintenance related labels Mar 5, 2024
Copy link

codecov bot commented Mar 5, 2024

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 93.36%. Comparing base (cd4f63d) to head (4dab70a).

Files Patch % Lines
src/ansys/hps/client/auth/api/auth_api.py 93.54% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #352      +/-   ##
==========================================
+ Coverage   92.82%   93.36%   +0.54%     
==========================================
  Files          61       61              
  Lines        2258     2232      -26     
==========================================
- Hits         2096     2084      -12     
+ Misses        162      148      -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@FedericoNegri FedericoNegri marked this pull request as ready for review March 5, 2024 14:44
@FedericoNegri FedericoNegri requested a review from PipKat March 5, 2024 14:47
@FedericoNegri FedericoNegri requested a review from nezgrath March 5, 2024 15:07
Copy link
Member

@PipKat PipKat left a comment

Choose a reason for hiding this comment

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

@FedericoNegri Only two comments, with only one of those being a correction! This approval is only on doc content--but what you did was excellent. I've never only had one suggested correction, even on PRs much smaller than this one!

FedericoNegri and others added 3 commits March 6, 2024 09:00
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
@FedericoNegri
Copy link
Contributor Author

@FedericoNegri Only two comments, with only one of those being a correction! This approval is only on doc content--but what you did was excellent. I've never only had one suggested correction, even on PRs much smaller than this one!

@PipKat your comment made we wonder and I realized I forgot to include the new "managing users" doc guide! please review this one https://github.com/ansys/pyhps/pull/352/files#diff-f835861f0b7fc9c9c1098462eb63089cd78a308d47e973efa1fb1037eaaf98a9, I'm sure you'll have more corrections :-)

Copy link
Member

@PipKat PipKat left a comment

Choose a reason for hiding this comment

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

Minor edits suggested.

FedericoNegri and others added 9 commits March 6, 2024 18:15
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
Co-authored-by: Kathy Pippert <84872299+PipKat@users.noreply.github.com>
@FedericoNegri FedericoNegri merged commit fdcf111 into main Mar 7, 2024
21 checks passed
@FedericoNegri FedericoNegri deleted the fnegri/remove_python_keycloak branch March 7, 2024 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Related with project dependencies documentation Improvements or additions to documentation maintenance Package and maintenance related
Projects
None yet
3 participants