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

docs: Provide easy alternative to create App JWT token #2937

Merged
merged 5 commits into from
Jun 24, 2024

Conversation

rasmus
Copy link
Contributor

@rasmus rasmus commented Jun 20, 2024

This adds a simple code snippet for creating the JWT token required to authenticate GitHub Apps. All dependencies live in the System namespace.

  • System.IdentityModel.Tokens.Jwt
  • System.Security.Claims
  • System.Security.Cryptography

Before the change?

Developers would think that the only alternative was to import another dependency.

After the change?

Developers are able to create JWT tokens for their GitHub Apps using dependencies they likely already have imported.

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

  • Yes
  • No

@rasmus rasmus changed the title Provide easy alternative to create App JWT token [docs] Provide easy alternative to create App JWT token Jun 20, 2024
@rasmus rasmus changed the title [docs] Provide easy alternative to create App JWT token docs: Provide easy alternative to create App JWT token Jun 20, 2024
Copy link
Member

@kfcampbell kfcampbell left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, this is great! I would rather encourage people to use system libraries. I had a couple quick questions and a suggestion before shipping.

In order to create the token, you can create it manually using the following snippet.

``` csharp
var rsaPrivateKey = "..."; // RSA private key from the App configuration page
Copy link
Member

Choose a reason for hiding this comment

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

Is this the key itself or the path to the key? I think this could be made more clear.

var rsaPrivateKey = "..."; // RSA private key from the App configuration page
var appId = 1; // The GitHub App Id

using var rsa = RSA.Create();
Copy link
Member

Choose a reason for hiding this comment

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

What's the advantage of the using statement here?

Copy link
Contributor Author

@rasmus rasmus Jun 22, 2024

Choose a reason for hiding this comment

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

Since the RSA object implements the IDisposable interface, its usually required to invoke the .Dispose() when the object leaves scope and is no longer needed. There could be some resources not managed by the runtime that needs cleaning up. I haven't decompiled the code to see what's actually there. The syntax for declaring using without braces was made available in .NET 8.

docs/github-apps.md Outdated Show resolved Hide resolved
@rasmus
Copy link
Contributor Author

rasmus commented Jun 22, 2024

@kfcampbell do. I added a code comment regarding the required using statements as well and commented on the using statement

@nickfloyd nickfloyd merged commit 16cea25 into octokit:main Jun 24, 2024
5 checks passed
@colbylwilliams
Copy link
Collaborator

@rasmus first, thank you for this. This token is one of the most confusing parts of getting started with GitHub Apps.

I noticed an announcement on the GitHub blog directing GitHub Apps to starting using the clientId instead of the appId in the iss when minting these tokens.

From the announcement:

To date, GitHub Apps have had two different IDs to manage – the application ID and the client ID. The application ID was only used to mint a JWT, subsequently used to fetch an installation token. The client ID is used with the OAuth flow to sign in users and request installations. These two values equally identify the application and the question of which one to use where caused unnecessary developer friction. You can now use the client ID in the place of the application ID when minting JWTs.

It might be worth updating the sample to use the clientId.

cc: @kfcampbell @nickfloyd

@nickfloyd
Copy link
Contributor

It might be worth updating the sample to use the clientId.

Thanks for following up on this @colbylwilliams, you're correct about the shift. We've made this move in the newly generated SDKs as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants