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

[STCOR-835] refactor getUserTenantsPermissions to leverage roles instead of permissions #1543

Merged
merged 9 commits into from
Oct 16, 2024

Conversation

ryandberger
Copy link
Member

@ryandberger ryandberger commented Oct 10, 2024

  • Conditionally use /users-keycloak/_self endpoint when users-keycloak interface is present.
  • Fulfills STCOR-835.

Copy link

github-actions bot commented Oct 10, 2024

Jest Unit Test Results

  1 files  ±0   56 suites  ±0   1m 34s ⏱️ ±0s
339 tests ±0  339 ✅ ±0  0 💤 ±0  0 ❌ ±0 
343 runs  ±0  343 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 2e9829d. ± Comparison against base commit cc8ef65.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Oct 10, 2024

Bigtest Unit Test Results

192 tests  ±0   187 ✅ ±0   7s ⏱️ -1s
  1 suites ±0     5 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 2e9829d. ± Comparison against base commit cc8ef65.

This pull request removes 5 and adds 3 tests. Note that renamed tests count towards both.
      equal to check email label in english translation
      equal to check email precautions label in english translation
      equal to sent email precautions label in english translation
Chrome_129_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status test check email status page tests should have the header with an appropriate text content
Chrome_129_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status test check email status page tests should have the paragraph with an appropriate text content
Chrome_129_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status test check email status page tests should have the header with an appropriate text content
      equal to check email label in english translation
Chrome_129_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status test check email status page tests should have the paragraph with an appropriate text content
      equal to check email precautions label in english translation
Chrome_129_0_0_0_(Linux_x86_64).Forgot username/password status test check email status page tests ‑ Forgot username/password status test check email status page tests should have the paragraph with an appropriate text content
      equal to sent email precautions label in english translation

♻️ This comment has been updated with latest results.

Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

I'm uncertain about this in light of the corresponding work @aidynoJ just did for UIU-3102 in PR 2752 to eliminate dependencies on the users-keycloak interface. I'm also confused about the directions given on this ticket. The conditional checks for the presence of roles but the endpoint (/users-keycloak/_self) comes from users-keycloak, so something seems amiss.

Finally, on the non-Eureka side, we should use /bl-users/_self instead of /perms/users/${id}/permissions since the former is universally accessible to any authenticated user but the latter requires specific permissions. If you don't want to do that clean up here I understand. Please file a new bug on UIU and have a chat with the ECS team who originally implemented this function since it was done incorrectly.

@ryandberger
Copy link
Member Author

I'm uncertain about this in light of the corresponding work @aidynoJ just did for UIU-3102 in PR 2752 to eliminate dependencies on the users-keycloak interface. I'm also confused about the directions given on this ticket. The conditional checks for the presence of roles but the endpoint (/users-keycloak/_self) comes from users-keycloak, so something seems amiss.

Finally, on the non-Eureka side, we should use /bl-users/_self instead of /perms/users/${id}/permissions since the former is universally accessible to any authenticated user but the latter requires specific permissions. If you don't want to do that clean up here I understand. Please file a new bug on UIU and have a chat with the ECS team who originally implemented this function since it was done incorrectly.

Seems like based on folio-org/ui-users#2752, both Eureka and non-Eureka can call /bl-users/_self. Is that correct?

@aidynoJ
Copy link
Contributor

aidynoJ commented Oct 11, 2024

I'm uncertain about this in light of the corresponding work @aidynoJ just did for UIU-3102 in PR 2752 to eliminate dependencies on the users-keycloak interface. I'm also confused about the directions given on this ticket. The conditional checks for the presence of roles but the endpoint (/users-keycloak/_self) comes from users-keycloak, so something seems amiss.

Finally, on the non-Eureka side, we should use /bl-users/_self instead of /perms/users/${id}/permissions since the former is universally accessible to any authenticated user but the latter requires specific permissions. If you don't want to do that clean up here I understand. Please file a new bug on UIU and have a chat with the ECS team who originally implemented this function since it was done incorrectly.

Seems like based on folio-org/ui-users#2752, both Eureka and non-Eureka can call /bl-users/_self. Is that correct?

That was the requirements for UIU-3192

@aidynoJ
Copy link
Contributor

aidynoJ commented Oct 11, 2024

I might be wrong. But based on https://folio-org.atlassian.net/browse/MODUSERSKC-30 and Bridging the gap between Users in the system and Users of the system we no longer need to create keycloak record directly. We can create them by assigning users to role and vice versa.

@ryandberger
Copy link
Member Author

I might be wrong. But based on https://folio-org.atlassian.net/browse/MODUSERSKC-30 and Bridging the gap between Users in the system and Users of the system we no longer need to create keycloak record directly. We can create them by assigning users to role and vice versa.

In this case, the endpoint is just being used to retrieve permissions a user has with tenants within a consortium. There is no create actions taking place here.

@aidynoJ
Copy link
Contributor

aidynoJ commented Oct 11, 2024

I might be wrong. But based on https://folio-org.atlassian.net/browse/MODUSERSKC-30 and Bridging the gap between Users in the system and Users of the system we no longer need to create keycloak record directly. We can create them by assigning users to role and vice versa.

In this case, the endpoint is just being used to retrieve permissions a user has with tenants within a consortium. There is no create actions taking place here.

I meant that probably it is related, now when we create user, the user is not created in keycloak and calling mod-roles-keycloak _self might cause an issue

Copy link

sonarcloud bot commented Oct 16, 2024

@ryandberger ryandberger merged commit 49bf361 into master Oct 16, 2024
26 checks passed
@ryandberger ryandberger deleted the STCOR-835 branch October 16, 2024 19:51
zburke pushed a commit that referenced this pull request Oct 31, 2024
…ead of permissions (#1543)

* Conditionally use /users-keycloak/_self endpoint when roles interface is present

* Update CHANGELOG

* Update URLs

* CHANGELOG and lint fix

* Match old API respsonse

* Clarify URL code

(cherry picked from commit 49bf361)
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.

3 participants