-
Notifications
You must be signed in to change notification settings - Fork 5
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
Eurofurence Identity Integration #43
Conversation
Looks good so far! I think it's fine to use an existing library if we can't get it working well easily with the default Microsoft libraries. Just one thing - I think we should rework the currently existing AuthenticationHandler or remove it if it is not needed anymore. There is also a "WhoAmI" endpoint, which I think won't work with the new authentication yet (correct me if I'm wrong). |
…y-provider # Conflicts: # src/Eurofurence.App.Server.Web/Startup.cs # src/Eurofurence.App.Server.Web/appsettings.sample.json
…y-provider # Conflicts: # src/Eurofurence.App.Server.Web/Eurofurence.App.Server.Web.csproj # src/Eurofurence.App.Server.Web/Startup.cs # src/Eurofurence.App.Tools.CliToolBox/Commands/CreateTokenCommand.cs # src/Eurofurence.App.Tools.CliToolBox/Commands/RegSysCommand.cs # test/Eurofurence.App.Server.Services.Tests/Eurofurence.App.Server.Services.Tests.csproj
I removed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could set the principal name using the property "NameClaimType" along with the RoleClaimType in the ConfigureOAuth2IntrospectionOptions class? I know that the TokenValidationOptions of the JwtBearerOptions from Microsoft support this at least. This way we can set the Name property of the ClaimsPrincipal, instead of using an extension method. If it is not supported by the authentication library, then maybe in a claims transformation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The NameClaimType
property exists, but it's default value is already name
, so we don't need to change it.
I changed the code to use the Name
property of the primary claims identity now.
…y-provider # Conflicts: # src/Eurofurence.App.Server.Web/Eurofurence.App.Server.Web.csproj # src/Eurofurence.App.Server.Web/Startup.cs # src/Eurofurence.App.Server.Web/appsettings.sample.json # test/Eurofurence.App.Server.Services.Tests/Eurofurence.App.Server.Services.Tests.csproj
I ended up using the
IdentityModel.AspNetCore.OAuth2Introspection
to implement the OAuth2 introspection based authentication. Not my favorite library, but it's the best that works for our use case. It even caches the introspection results, as long as the token is still valid.Client id and endpoints can be configured in the
Identity
section of theappsettings.json
It cannot query the user info endpoint though, so I added that feature manually and merge the claims from the user info endpoint into the
ClaimsIdentity
.Currently I configured the authorization policy so that every endpoint just requires a valid token, if not configured otherwise.Endpoints require aAuthorize
attribute like always.A few endpoints require cretin roles and they don't really map to anything right now, so calling these endpoints is currently impossible.
Closes #36