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

feat: support configuration to include transient group ownership during auth #2210

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: support configuration to include transient group ownership during auth #2210

wants to merge 1 commit into from
+621 −3

Conversation

JessicaJHee
Copy link
Member

@JessicaJHee JessicaJHee commented Jan 20, 2025 •
edited
Loading

Description

  • Adds InheritedGroupOwnershipResolver to provide the ability to add optionally transient parent groups into the resolved user group membership
  • Enabled by this config: includeInheritedGroupOwnership?: boolean
  • Adds unit tests for AuthProviderModule and InheritedGroupOwnershipResolver

With includeTransientGroupOwnership to true:
image

  • Includes maintainers group which is a parent of maintainers-plugins and maintainers-showcase

With includeTransientGroupOwnership to false (by default):
image

  • only direct group ownership is included

Which issue(s) does this PR fix

PR acceptance criteria

Please make sure that the following steps are complete:

  • GitHub Actions are completed and successful
  • Unit Tests are updated and passing
  • E2E Tests are updated and passing
  • Documentation is updated if necessary (requirement for new features)
  • Add a screenshot if the change is UX/UI related

@JessicaJHee JessicaJHee changed the title Support configuration to include transient group ownership during auth feat: support configuration to include transient group ownership during auth Jan 20, 2025
@JessicaJHee JessicaJHee force-pushed the extend-ownership-ref-res branch from a8f8f40 to e31bb50 Compare January 20, 2025 19:16
@github-actions GitHub Actions
Copy link
Contributor

The image is available at:

@JessicaJHee JessicaJHee marked this pull request as draft January 20, 2025 21:34
@JessicaJHee JessicaJHee force-pushed the extend-ownership-ref-res branch 3 times, most recently from 73c4e8d to 99b83e0 Compare January 22, 2025 21:25
@JessicaJHee JessicaJHee marked this pull request as ready for review January 22, 2025 21:49
@github-actions GitHub Actions
Copy link
Contributor

The image is available at:

PatAKnight
PatAKnight reviewed Jan 29, 2025
View reviewed changes
Copy link
Member

@PatAKnight PatAKnight left a comment

Choose a reason for hiding this comment

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

Overall, it looks fairly good to me. A couple of requests / questions from me.

  • Could you add some docs here. That way we have something for the docs team to reference.
  • I know there was an issue with large tokens, I believe that it had something to do with ownership refs, is that still a problem today?

Also, I did run into an infinite loop while testing that I think might be worth addressing. I tested with a user that had cyclic behavior in the group memberships, something like user -> group_a <=> group_b

packages/backend/src/transientGroupOwnershipResolver.ts Outdated Show resolved Hide resolved
packages/backend/src/transientGroupOwnershipResolver.ts Outdated Show resolved Hide resolved
@JessicaJHee JessicaJHee force-pushed the extend-ownership-ref-res branch from 99b83e0 to 0281d34 Compare February 3, 2025 16:20
@JessicaJHee
Copy link
Member Author

JessicaJHee commented Feb 3, 2025 •
edited
Loading

@PatAKnight Thank you very much for the review! I've addressed your feedback, PTAL :)

I know there was an issue with large tokens, I believe that it had something to do with ownership refs, is that still a problem today?

I'm not too familiar with this issue, the only thing I found was this and it looks like it's been resolved in 1.3 👍

@JessicaJHee JessicaJHee force-pushed the extend-ownership-ref-res branch from 0281d34 to 19fce4d Compare February 3, 2025 16:24
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 3, 2025

The image is available at:

@JessicaJHee JessicaJHee force-pushed the extend-ownership-ref-res branch from 19fce4d to 5cf91f7 Compare February 5, 2025 15:32
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 5, 2025

The image is available at:

PatAKnight
PatAKnight approved these changes Feb 6, 2025
View reviewed changes
Copy link
Member

@PatAKnight PatAKnight 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.

Tested and it worked for me.
/lgtm

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Feb 6, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: PatAKnight

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved label Feb 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PatAKnight PatAKnight PatAKnight approved these changes

@dzemanov dzemanov Awaiting requested review from dzemanov

@its-mitesh-kumar its-mitesh-kumar Awaiting requested review from its-mitesh-kumar

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

Successfully merging this pull request may close these issues.
2 participants
@JessicaJHee @PatAKnight