aspnet-contrib / AspNet.Security.OAuth.Providers

OAuth 2.0 social authentication providers for ASP.NET Core
Apache License 2.0
2.34k stars 533 forks source link

Add .NET SDK update workflow #828

Closed martincostello closed 4 months ago

martincostello commented 4 months ago

Add a GitHub Actions workflow that patches the .NET SDK and ASP.NET Core NuGet packages every month.

It can also be run manually targeting the dev-v9 branch for updating the .NET 9 SDK from month-to-month as new previews are published.

Note that because of the use of GITHUB_TOKEN, GitHub Actions CI will not be triggered when PRs are opened due to a GitHub Actions restriction; PRs need to be closed then opened by a maintainer to get it to run. A user PAT or a GitHub app is required to remove that limitation (example).

kevinchalet commented 4 months ago

Why not, if you think it can simplify the maintenance. That said, I have two concerns:

Instead of a scheduled action, maybe an on-demand-only workflow would be a more reasonable compromise?

kevinchalet commented 4 months ago

I just realized the action actually opens a PR (instead of just pushing a commit against the targeted branch), so it partially addresses my first concern 😄

martincostello commented 4 months ago

Updating the SDK - or the NuGet packages - is always a risky operation and it's not exactly clear what benefits moving immediately - and automatically - to the latest version has.

IMHO it's not risky because that's what the CI is for and why it's raised as a PR - if it passes, it works, we merge. If it doesn't, we don't.

Doing that will force our contributors to update their VS setup to be able to use the solution as soon as the new .NET SDK version ships.

They can always delete the file locally if they run into issues. It would be quite unfortunate timing for someone to try and contribute so close to an SDK update being merge that Windows Update hadn't updated their installation of VS anyway.

Instead of a scheduled action, maybe an on-demand workflow would be a more reasonable compromise?

It's already runnable on-demand, but kinda undercuts the value IMHO if you have to remember to run it in the first place. For my own repos, my automation has typically updated everything in <90 minutes from the release notes being updated to fully deployed (behold the matrix).

kevinchalet commented 4 months ago

Sold. Do you want me to create a fine-grained PAT to address the concern mentioned in your OP? If so, what permissions do you need for the action to work properly? Just pull requests R/W?

martincostello commented 4 months ago

That's up to you really - not using GITHUB_TOKEN is better, but using a PAT then makes it look like all the PRs are coming from you. A GitHub app is better as then it's a service account, which is what we set up in Polly, but of course that's then faff for you to set up (albeit, a one-time one).

In either case, access needed is:

If you do want to go down the PAT route, then also create the token with access to the OpenID repo, as I was planning on duplicating this workflow in that repo too if you approved this one.

kevinchalet commented 4 months ago

I created a GitHub app under aspnet-contrib named aspnet-contrib-service-account with the GitHub ID 161750942: https://api.github.com/users/aspnet-contrib-service-account[bot].

The client ID and the private key were added as SERVICE_ACCOUNT_ID and SERVICE_ACCOUNT_PRIVATE_KEY secrets.

Edit: assuming you didn't want to hardcode the git email/name in the workflow file, I also created two SERVICE_ACCOUNT_GIT_EMAIL and SERVICE_ACCOUNT_GIT_NAME variables as suggested in your doc.

Let me know if I did something wrong or if you need anything else 😄

kevinchalet commented 4 months ago

The client ID and the private key were added as SERVICE_ACCOUNT_ID and SERVICE_ACCOUNT_PRIVATE_KEY secrets.

Hum, actually, I added the "app ID" as the SERVICE_ACCOUNT_ID secret, but maybe it's the client ID that is required instead?

martincostello commented 4 months ago

Reminding myself of how I set this up elsewhere (assuming I've read it right), the Application ID is the numeric ID of the bot which isn't a secret, so in this case 161750942, and then the secret is the private key file content (the base64 stuff) you'll have generated when you set up the bot.

I'll push a change shortly to use those and merge this, then run it manually to see what happens. It should fail quite quickly if any of that is incorrectly configured as exchanging the key for a bearer token should fail before it tries to clone the repo.

martincostello commented 4 months ago

Wait, I think I've mixed up the bot/user ID and the app ID...

kevinchalet commented 4 months ago

Wait, I think I've mixed up the bot/user ID and the app ID...

Yeah, there are actually 3 identifiers: for this service account, the one called "app ID" starts with a 8 (I guess it's not secret, but just in case, I'll avoid disclosing it here 😄)

martincostello commented 4 months ago

So I checked my bot, and yeah I confused them.

There's the ID of the user/bot associated with the app, which is 161750942, then there's the ID of the installation for that app, which is another number. The latter is the one that goes in the config for getting then bearer token.

Hopefully it'll be clearer when I push the change in a few minutes.

martincostello commented 4 months ago

I've used the two Git vars too. I'd expect them to have these values:

kevinchalet commented 4 months ago

Yup:

image

kevinchalet commented 4 months ago

The latter is the one that goes in the config for getting then bearer token.

If it's an OAuth 2.0-inspired flow that is used to retrieve it, then maybe we'll need to store the "client ID" instead instead of the "app ID". I guess we'll know soon enough 🤣

martincostello commented 4 months ago

I don't think I've needed to use it before for this stuff, so shouldn't need it 🤞

martincostello commented 4 months ago

Failed - does the app have permissions to this repo (and OpenID)?

kevinchalet commented 4 months ago

Failed - does the app have permissions to this repo (and OpenID)?

Should be good now: I stupidly assumed that a private GitHub app created under an org' would be automatically installed but it's not the case. I just fixed that 🤣

martincostello commented 4 months ago

Ah yes, that's definitely never happened to me before ever either...

martincostello commented 4 months ago

Bonus points for giving the bot a profile image 😄

kevinchalet commented 4 months ago

Haha, sorry for being such a noob. I've created a lot of GitHub OAuth 2.0 demo apps for the various OpenIddict code flow/device flow samples in the past but it's the first time I'm creating a "GitHub app" 😅

martincostello commented 4 months ago

💥: #829

martincostello commented 4 months ago

No worries - it's not the easiest thing in the world to sort out!

kevinchalet commented 4 months ago

Bonus points for giving the bot a profile image 😄

Hehe. Too bad the background color you set is respected for the /app page but not for the rest of the GitHub UI... it looks horrible when using the black theme 😅

image

kevinchalet commented 4 months ago

💥: #829

I like it!

Is your action also able to update the SDK versions defined in a GitHub action?

OpenIddict needs multiple .NET runtime versions and used to rely on Arcade to install them (see https://github.com/openiddict/openiddict-core/blob/0a53dc9741851b7a3ae4984bb98e58a119f21aa0/global.json#L9-L16) but it wasn't 100% reliable, so I ended up tweaking the build action to simply install multiple SDK versions (which also install the corresponding runtime).

It can't be done via global.json since the setup action only supports setting one version, so it's done via yml: https://github.com/openiddict/openiddict-core/blob/0a53dc9741851b7a3ae4984bb98e58a119f21aa0/.github/workflows/build.yml#L45-L48

Asking for a friend 🤣

martincostello commented 4 months ago

Not at the moment, but I'm not adverse to the idea of adding support for it if it's something "standard".

I already added support for the tools key Arcade uses so the ASP.NET Core team could use it (workflow).

kevinchalet commented 4 months ago

Great, thanks! I'll keep an eye on your repo then 😄

martincostello commented 4 months ago

If you fire up an issue with exactly what behaviour you're looking fpr I'll take a look into it.