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.

Sign up for GitHub

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

Jump to bottom

1624: Add role for verein360 to create api tokens #1794

Merged
merged 8 commits into from
Dec 4, 2024
Merged

1624: Add role for verein360 to create api tokens #1794

merged 8 commits into from
Dec 4, 2024

Conversation

ztefanie
Copy link
Member

@ztefanie ztefanie commented Nov 20, 2024 •
edited
Loading

Short description

Added a new role for verein360 admins

Proposed changes

  • added a new role to create api tokens for verein360
  • added new column "type" for api tokens
  • adjusted project settings
  • new external-user role can only create tokens of type "verified_application"
  • project admins of koblenz create tokens of type "user_import"
  • only api tokens of type user-import are allowed to use koblenz csv endpoint

Side effects

  • may break anything releated to api token generation of koblenz project admins or csv upload endpoint

Testing

  • create and delete tokens for different roles, check if type is set correctly
  • check if endpoint for koblenz csv shows forbidden exception when used with api token of type external-verified
  • add and remove user with external-api role

Resolved issues

Fixes: #1624

@ztefanie ztefanie force-pushed the 1624-add-role-for-verein-360-api-tokens branch from cb033dc to 604ab4d Compare November 20, 2024 13:14
codescene-delta-analysis[bot]
codescene-delta-analysis bot reviewed Nov 20, 2024
View reviewed changes
Copy link

@codescene-delta-analysis codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

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

❌ Code Health Quality Gates: FAILED

Change in average Code Health of affected files: -0.02 (9.57 -> 9.55)

  • Declining Code Health: 4 findings(s) 🚩

  • Affected Hotspots: 1 files(s) 🔥

View detailed results in CodeScene

administration/src/bp-modules/NavigationBar.tsx Outdated Show resolved Hide resolved
administration/src/bp-modules/project-settings/ProjectSettingsController.tsx Show resolved Hide resolved
administration/src/bp-modules/project-settings/UserEndpointSettings.tsx Show resolved Hide resolved
backend/src/main/kotlin/app/ehrenamtskarte/backend/auth/database/repos/ApiTokensRepository.kt Show resolved Hide resolved
backend/src/main/kotlin/app/ehrenamtskarte/backend/userdata/webservice/UserImportHandler.kt Show resolved Hide resolved
backend/src/main/kotlin/app/ehrenamtskarte/backend/userdata/webservice/UserImportHandler.kt Show resolved Hide resolved
codescene-delta-analysis[bot]
codescene-delta-analysis bot reviewed Nov 20, 2024
View reviewed changes
Copy link

@codescene-delta-analysis codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

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

❌ Code Health Quality Gates: FAILED

Change in average Code Health of affected files: -0.02 (9.57 -> 9.55)

  • Declining Code Health: 4 findings(s) 🚩

  • Affected Hotspots: 1 files(s) 🔥

View detailed results in CodeScene

@ztefanie ztefanie force-pushed the 1624-add-role-for-verein-360-api-tokens branch from bc28a51 to 4e24ce6 Compare November 20, 2024 13:47
codescene-delta-analysis[bot]
codescene-delta-analysis bot reviewed Nov 20, 2024
View reviewed changes
Copy link

@codescene-delta-analysis codescene-delta-analysis bot left a comment

Choose a reason for hiding this comment

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

❌ Code Health Quality Gates: FAILED

Change in average Code Health of affected files: -0.02 (9.57 -> 9.56)

  • Declining Code Health: 4 findings(s) 🚩

  • Affected Hotspots: 1 files(s) 🔥

View detailed results in CodeScene

@ztefanie ztefanie force-pushed the 1624-add-role-for-verein-360-api-tokens branch from d969cbb to bba18e1 Compare November 26, 2024 09:20
f1sh1918
f1sh1918 reviewed Nov 26, 2024
View reviewed changes
Copy link
Contributor

@f1sh1918 f1sh1918 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The concept looks really good to me 👍
Just found some issues

image

  1. In the dashboard the item for "Projekt verwalten" is missing when i login with external api user

  2. When i click on "Projekt verwalten" i get a confusing headline

image

I think the headline should be somehow connected with the user role to indicate the purpose of the token dynamically

  1. Is it intended that Koblenz can also create an external api user?
    I mean it could make sense to separate the user import right from project admin to a specific role (external api users) but is in our scenario for koblenz not really needed
image

I think it would make sense to even add a check in the backend that only certain ROLES and projects can create external api user f.e. mayCreateExternalAPIUser fkt same for edit

administration/src/bp-modules/NavigationBar.tsx Outdated Show resolved Hide resolved
@ztefanie
Copy link
Member Author

ztefanie commented Nov 26, 2024 •
edited
Loading

  1. In the dashboard the item for "Projekt verwalten" is missing when i login with external api user

good find, fixed it.

  1. When i click on "Projekt verwalten" i get a confusing headline

fixed this by changeing the headling to "Api token"

  1. Is it intended that Koblenz can also create an external api user?
    I mean it could make sense to separate the user import right from project admin to a specific role (external api users) but is in our scenario for koblenz not really needed

I think it would make sense to even add a check in the backend that only certain ROLES and projects can create external api user f.e. mayCreateExternalAPIUser fkt same for edit

Good point. I added this check to the existing "mayCreateUser" function.

@ztefanie ztefanie requested a review from f1sh1918 November 26, 2024 12:34
@ztefanie
1624: Fix formatting
4487008
@ztefanie
Merge branch 'main' into 1624-add-role-for-verein-360-api-tokens
e28c088 
# Conflicts:
#	backend/src/test/kotlin/app/ehrenamtskarte/backend/helper/TestData.kt
#	backend/src/test/kotlin/app/ehrenamtskarte/backend/userdata/UserImportTest.kt
f1sh1918
f1sh1918 approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@f1sh1918 f1sh1918 left a comment

Choose a reason for hiding this comment

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

One small change i would really recommend because i found it difficult to understand the code.
Works fine. Nicely done 👍

administration/src/bp-modules/users/RoleSelector.tsx Outdated Show resolved Hide resolved
@seluianova seluianova self-assigned this Dec 3, 2024
@seluianova
Copy link
Contributor

seluianova commented Dec 3, 2024 •
edited
Loading

🙃 description of the new role is missing here
image

seluianova
seluianova approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@seluianova seluianova left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm, just shared a couple of minor thoughts

@seluianova seluianova removed their assignment Dec 3, 2024
@ztefanie
Copy link
Member Author

ztefanie commented Dec 4, 2024

lgtm, just shared a couple of minor thoughts

Thanks for the review. Good points. Fixed all of them.

@ztefanie ztefanie merged commit bd1424d into main Dec 4, 2024
1 of 2 checks passed
@ztefanie ztefanie deleted the 1624-add-role-for-verein-360-api-tokens branch December 4, 2024 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codescene-delta-analysis codescene-delta-analysis[bot] codescene-delta-analysis[bot] left review comments

@seluianova seluianova seluianova approved these changes

@f1sh1918 f1sh1918 f1sh1918 approved these changes

@steffenkleinle steffenkleinle Awaiting requested review from steffenkleinle steffenkleinle is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add role and option to create api tokens for verein 360
3 participants
@ztefanie @seluianova @f1sh1918