fix(scaffolder): remove waiting until all fields are filled in before requesting user credentials

[benjidotsh]

Hey, I just made a Pull Request!

When using RepoUrlPicker with its requestUserCredentials option, the dialog only appears after all fields have a value. This results in the dialog appearing while typing in the last value, which can lead to a frustrating experience.

This pull request removes this check, which shows the dialog as soon as the RepoUrlPicker is displayed.

✔️ Checklist

• A changeset describing the change and affected packages. (more info)
• All your commits have a Signed-off-by line in the message. (more info)

[backstage-goalie]

Changed Packages

Package Name	Package Path	Changeset Bump	Current Version
@backstage/plugin-scaffolder	plugins/scaffolder	patch	v1.21.0-next.3

[benjdlambert]

I'm not sure that we can do this, because when we request user credentials, they have the org and repoName already available so that we can request access to that URL with the org and repo already available.

Not sure if this could potentially break something, maybe not for GitHub but for other providers there could be a chance. What providers have you tested this with?

[benjdlambert]

I want to call out also, this is another reason why I want to look at doing something like this: #16996

[benjidotsh]

@benjdlambert ah, I see what you mean. I'm currently testing this with Bitbucket, where I didn't encounter any issues with it, but I can see the danger of it.

Maybe it's a better idea to make sure the dialog only pops up after the user leaves the last field for now? Or, maybe even better, add a button or when the button to the next step is pressed (if possible)?

[acierto]

Hey @benjidotsh!

It could be tricky to have a control of popup from the button, as the invocation of it resides in the picker.

[benjidotsh]

Hey @acierto, yeah, I know, but I think we could add a prop to the picker component that acts as a handler for controlling the dialog. But maybe using an on blur event is enough - then we don't have to escape the picker component.

[backstage-goalie]

Thanks for the contribution!
All commits need to be DCO signed before they are reviewed. Please refer to the the DCO section in CONTRIBUTING.md or the DCO status for more info.

[benjidotsh]

@benjdlambert @acierto I dug a little deeper and, unless I'm missing something, I noticed that we only use the host part of the URL anyways.

I changed the code to only pass and check for the host for authentication, which inherently fixes the goal of this PR.

[benjidotsh]

I think the test should call the scmAuthApi with the correct params if only a project is set is redundant, but I don't see the connection between the title and the code, so I left it in for now. Please let me know if my feeling is correct and I'll remove it.

[benjidotsh]

@benjdlambert can you maybe take a look at this PR again? 😇

[benjdlambert]

Hey 👋 thanks for this!

Just wanna get some additional feedback here, thinking that we might want to solve this a different way.

I'm thinking that the secret for the actual job could be handled with something like #16996 and that a token for your other PR #25132 could be generated without waiting for all fields to be filled out of course.

[benjdlambert]

Hmm, i'm not sure that we can make this change as it is now as it's a breaking change.

I wonder if we could perhaps support a union of either { url: string } | { host: string } instead.

The idea I guess is that there could be different credentials issued for different URLs, not just for VCS providers, but imagine a URL where you have a different token depending on path.

@Rugvip whats your thoughts here?

[benjidotsh]

Good point. I don't think it's absolutely necessary to even use host - I just refactored this because it looked like we only used the host part anyways. Even though I updated the usage of it everywhere in the code, I can imagine that an API change is not ideal for other plugins.

The idea I guess is that there could be different credentials issued for different URLs, not just for VCS providers, but imagine a URL where you have a different token depending on path.

I'm not sure what you mean with this part though. You're talking about potential use cases of ScmAuthTokenOptions that require URLs instead of hosts?

[benjdlambert]

No sorry , I'm talking about the getCredentials method. So lets say for GitHub for instance you have two different GitHub Apps configured in config. One of them is installed in the organization blob whilst the other is installed in the organization boop.

If I'm just requesting a token for github.com then the getCredentials method has no idea which app to select as it doesn't have context in the URL as to which org we're trying to reach right? I think thats the motivation behind keeping the URL as you can do url: 'https://github.com/blob/my-repo' to get a token for my-repo with the org blob.

That's why I think we can't remove it all at least.

[benjidotsh]

Yeah, I understand, but how it is implemented right now it completely ignores everything but the host anyways, right?

[benjdlambert]

Ah wait sorry, you're right, I'm getting confused here between frontend and backend. It's the GithubCredentialsProvider that does that work, and that's backend only.

Ok, maybe there's room for us to do this, but yeah, unsure about the breaking change.

[benjidotsh]

I think I can implement this without changing the API. I'll look into it!

[benjidotsh]

Yep, should be done!

It doesn't differ too much from the original code and simplifies the PR a lot since the only thing we do is not pass the workspace and repository that go unused anyways.

[benjidotsh]

@benjdlambert I agree #16996 would be the ideal solution, but I think that might be a huge undertaking. In the meantime it could be a good idea to just "fix" the existing implementation by making host the only required property since it's the only one that is being used anyways.

[benjdlambert]

Nice, thanks for the work here! Let's go with this! 🙏

[github-actions]

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