feat: allow overriding default ownership resolving

[drodil]

Hey, I just made a Pull Request!

This allows to modify the ownerships in built-in auth provider factories by overriding the necessary functionality in the context. For example if user wants to include parent groups also to the ownershipEntityRefs, it's not possible unless the built-in auth providers are forked and rewritten.

:heavy_check_mark: Checklist

• [x] A changeset describing the change and affected packages. (more info)
• [ ] Added or updated documentation
• [ ] Tests for new functionality and regression tests for bug fixes
• [ ] Screenshots attached (for UI changes)
• [x] All your commits have a Signed-off-by line in the message. (more info)

[backstage-goalie[bot]]

Changed Packages

Package Name	Package Path	Changeset Bump	Current Version
@backstage/plugin-auth-backend	plugins/auth-backend	patch	v0.22.4-next.1
@backstage/plugin-auth-node	plugins/auth-node	patch	v0.4.12-next.1

[github-actions[bot]]

Uffizzi Cluster pr-22765 was deleted.

[drodil]

I'm thinking it's better to fork and re-write the built-in sign-in resolver rather than forking and re-writing the resolver context? 😅

Thanks for this! That is an option but we are using 4 different resolvers and would have to basically fork 3 of them, while just overriding the context, we could get away with just replacing one method 🫠 If you think this is not going to get merged, I think we are out of options and have to go with the forks.

And what is the basic need behind is that we would like to have the parent groups of groups user belongs to in the ownershipEntityRefs. Another option would be to change the default resolution in context but I don't know if that has too many side effects to other adopters.

[awanlin]

Sorry, late to the conversation, but isn't this how you would work around this? https://backstage.io/docs/backend-system/building-backends/migrating#custom-resolver. Or is that what you were suggesting @Rugvip ?

[Rugvip]

Yep!

[drodil]

Sorry, late to the conversation, but isn't this how you would work around this? https://backstage.io/docs/backend-system/building-backends/migrating#custom-resolver. Or is that what you were suggesting @Rugvip ?

Yep, I'm aware of that 👍 It's just a bit too much just to modify the ownership resolution to fork all of the used providers 😆 But sure, if you think this is the only way to do this, you can close this PR and I will start forking like a monkey in a restaurant 🐒

[drodil]

@Rugvip @awanlin how 'bout this? Just to be able to override the ownership resolution, would that make sense?

[awanlin]

@drodil, I guess I'm mostly confused by what you mean by "forking", do you need to add more code then what's in the docs? There is a bit more then the current method of doing this in the auth.ts file but it's only a few lines.

[drodil]

@awanlin it might be a bit easier with the new backend system (which is not yet in production ready) with extension but with current it's basically a fork of each provider factory from upstream to replace the getDefaultOwnershipEntityRefs function.

[drodil]

Or am I missing something completely here? How can I use, let's say, github auth provider from the upstream and modify the ownership entity references resolving with old and/or the new backend? Feeling a bit dumb, I just couldn't figure it out 😅

[freben]

I think the mindset is that providers is something that you basically write once or twice per lifetime of a company, and they are so varying and so short that there's little use in trying to provide too much magic.

There are a couple of simple builtin resolvers yes, but I think that maybe we don't aim to have a super high threshold to having to write your own.

That being said, if there's more methods or options that we can provide in the builtin context features, for very common use cases - then maybe those can be considered in isolation. For example, could the ctx.signInWithCatalogUser method have a traverseOwnershipGroups?: 'parents' | 'children'? Not saying that that's the exact right answer, but just as a food for thought.

[drodil]

@freben yes the other way of handling this is to allow passing some properties to that function but again it requires changes to the existing providers either by config or some other way. It's also true that these providers are done once and usually forgotten, but this requirement rised in our internal portal and before starting to write own providers, I wanted to get some idea if this kind of change makes sense.

I know it can get complicated with lot of customization options, but building a platform for all needs is never a trivial task to do. I appreciate the work you put into reviewing these things and thinking the overall picture 🙏

I will wait for the final verdict for this and carry on with other way if this never gets merged 😄

[awanlin]

@drodil, sorry, my comments aren't intended to block anything, just trying to understand. Mostly because I was trying to do something similar recently and had issues and thought maybe you did as well.

But getting back to this, would this not be what you are trying to do with the current backend? https://backstage.io/docs/auth/identity-resolver#custom-ownership-resolution

[drodil]

@drodil, sorry, my comments aren't intended to block anything, just trying to understand. Mostly because I was trying to do something similar recently and had issues and thought maybe you did as well.

But getting back to this, would this not be what you are trying to do with the current backend? https://backstage.io/docs/auth/identity-resolver#custom-ownership-resolution

Thanks for this and no worries. I think I tried this already without success but will get back to you with actual code samples. If this can be resolved like in the docs, I have to apologize for wasting time 😄 But I will return this tomorrow!

[drodil]

Ah, I had to check this. So, let's take 2 providers, microsoft and awsalb.

The Microsoft provider resolver has the following signature:

 resolver: SignInResolver<OAuthResult>;

while awsalb has:

resolver: SignInResolver<AwsAlbResult>;

While I think it's possible to rewrite the resolvers instead of the default ones, it still requires two different resolvers to be implemented. Not to count the other 2 providers we have that use the createAuthProviderIntegration where the resolvers can be any type:

TResolvers extends {
    [name in string]: (...args: any[]) => SignInResolver<any>;
}

This seems to be somewhat extra effort instead of just writing one class that handles all of these cases where the signInWithCatalogUser is called:

export class MyAuthOwnershipResolver implements AuthOwnershipResolver {
  private readonly catalogClient: CatalogApi;
  constructor(options: {
    discovery: PluginEndpointDiscovery;
    tokenManager: TokenManager;
    catalogClient?: CatalogApi;
  }) {
    this.catalogClient =
      catalogClient || new CatalogClient({ discoveryApi: discovery });
  }

  private async resolveParentGroups(groupRefs: string[]) {
    const groups = this.catalogClient.getEntitiesByRefs(groupRefs);
    // ... get parents, call iterative
   return groups.map(stringifyEntityRef);
  }

  async getOwnershipEntityRefs(entity: Entity): Promise<string[]> {
    const membershipRefs =
      entity.relations
        ?.filter(
          r =>
            r.type === RELATION_MEMBER_OF && r.targetRef.startsWith('group:'),
        )
        .map(r => r.targetRef) ?? [];

    return Array.from(new Set([stringifyEntityRef(entity), ...resolveParentGroups(membershipRefs)]));
  }
}

And then pass an instance of that to the createRouter.

As said, if you think it's not the proper way to handle this and I should go and write the different resolvers for each provider, I will go that way 👍

[Rugvip]

@drodil I get the need and I think we can definitely add something in this space. Actually been looking at ways to make ownership resolution more accurate out of the box too.

Could you perhaps elaborate on the exact kind of logic that you're looking to introduce? I'd like to compare that to others' and our own internal one to see if any patterns emerge. Our own internal logic is that any group of a specific type and any type of group within such groups can be owners.

[drodil]

@Rugvip sure! So what I would like to achieve is that:

• a group called company-developers has a child group called team1-developers
• user john.doe is a member of the group team1-developers
• john.does ownership entity refs after the desired resolution consist of: user:john.doe, group:team1-developers, and group:company-developers
• if the company-developers group has also parent group(s), those should also be in the array of ownership entity refs

Hope this makes sense 😄 To resolve all the groups, we need access to the CatalogApi which might be a bit hard to add to the custom resolvers pattern.

[drodil]

Any chance to probably get this as part of the next release? We have some internal requirements that require solving the ownerships as described in my earlier comment. Thanks for your consideration!

[Rugvip]

@drodil hmm, would like a bit more time to think about this tbh. Are you unblocked for now by duplicating the sign-in resolvers?

[drodil]

@Rugvip ok, yes we can do it with duplicating the resolvers but have to wait for the @backstage/plugin-auth-backend-module-aws-alb-provider to be released as it has some types that can be reused instead duplicating everything

[github-actions[bot]]

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

[drodil]

Not stale

[drodil]

So did the resolving for one provider now and it requires a lot of changes already for one provider. Any chance to make this go forward in some direction? We can manage for now with only one provider but it's a bit pain in the ass to test.

[github-actions[bot]]

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

[drodil]

Not stale

[github-actions[bot]]

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

[drodil]

Not stale

[github-actions[bot]]

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

[drodil]

Not stale, needs discussion

[github-actions[bot]]

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

[drodil]

Not stale

[drodil]

Thanks for the comments, will get back to you with the fixes later this week 👌

[github-actions[bot]]

This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution!

[drodil]

Not stale

[drodil]

Ready for another round 🏑

[github-actions[bot]]

Thank you for contributing to Backstage! The changes in this pull request will be part of the 1.27.0 release, scheduled for Tue, 14 May 2024.