-
Notifications
You must be signed in to change notification settings - Fork 19
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
Replace bunch of string concatenation with usage of StringBulder #55
Open
maxime-poulain
wants to merge
1
commit into
GFlisch:feature/Authentication
Choose a base branch
from
maxime-poulain:feature/stringBuilder-instead-of-concatenation
base: feature/Authentication
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
using System; | ||
using System.Diagnostics.CodeAnalysis; | ||
using System.Linq; | ||
using System.Text; | ||
using Arc4u.Configuration; | ||
using Arc4u.OAuth2.DataProtection; | ||
using Arc4u.OAuth2.Extensions; | ||
|
@@ -185,63 +186,72 @@ public static AuthenticationBuilder AddOidcAuthentication(this IServiceCollectio | |
|
||
var settings = section.Get<OidcAuthenticationSectionOptions>() ?? throw new NullReferenceException($"No section exists with name {authenticationSectionName} in the configuration providers for OpenId Connect authentication."); | ||
|
||
string? configErrors = null; | ||
var configErrorsStringBuilder = new StringBuilder(); | ||
if (string.IsNullOrWhiteSpace(settings.MetadataAddress)) | ||
{ | ||
configErrors += "MetadataAddress must be filled!" + System.Environment.NewLine; | ||
configErrorsStringBuilder.AppendLine("MetadataAddress must be filled!"); | ||
} | ||
|
||
if (string.IsNullOrWhiteSpace(settings.CookieName)) | ||
{ | ||
configErrors += "We need a cookie name defined specifically for your services." + System.Environment.NewLine; | ||
configErrorsStringBuilder.AppendLine("We need a cookie name defined specifically for your services."); | ||
} | ||
|
||
if (string.IsNullOrWhiteSpace(settings.OpenIdSettingsSectionPath)) | ||
{ | ||
configErrors += "We need a setting section to configure the OpenId Connect." + System.Environment.NewLine; | ||
configErrorsStringBuilder.AppendLine("We need a setting section to configure the OpenId Connect."); | ||
} | ||
|
||
if (string.IsNullOrWhiteSpace(settings.OAuth2SettingsSectionPath)) | ||
{ | ||
configErrors += "We need a setting section to configure OAuth2." + System.Environment.NewLine; | ||
configErrorsStringBuilder.AppendLine("We need a setting section to configure OAuth2."); | ||
} | ||
|
||
if (string.IsNullOrWhiteSpace(settings.CertificateSectionPath)) | ||
{ | ||
configErrors += "We need a setting section to specify the certificate to protect your sensitive information." + System.Environment.NewLine; | ||
configErrorsStringBuilder.AppendLine("We need a setting section to specify the certificate to protect your sensitive information."); | ||
} | ||
|
||
if (string.IsNullOrWhiteSpace(settings.DataProtectionSectionPath)) | ||
{ | ||
configErrors += "We need a setting section to configure the DataProtection cache store." + System.Environment.NewLine; | ||
configErrorsStringBuilder.AppendLine("We need a setting section to configure the DataProtection cache store."); | ||
} | ||
|
||
if (string.IsNullOrWhiteSpace(settings.JwtBearerEventsType)) | ||
{ | ||
configErrors += "The JwtBearerEventsType must be defined." + System.Environment.NewLine; | ||
configErrorsStringBuilder.AppendLine("The JwtBearerEventsType must be defined."); | ||
} | ||
|
||
if (string.IsNullOrWhiteSpace(settings.ClaimsIdentifierSectionPath)) | ||
{ | ||
configErrors += "We need a setting section to specify the claims used to identify a user." + System.Environment.NewLine; | ||
configErrorsStringBuilder.AppendLine("We need a setting section to specify the claims used to identify a user."); | ||
} | ||
|
||
if (configErrors is not null) | ||
if (string.IsNullOrWhiteSpace(settings.CookieAuthenticationEventsType)) | ||
{ | ||
throw new ConfigurationException(configErrors); | ||
configErrorsStringBuilder.AppendLine("The CookieAuthenticationEventsType must be defined."); | ||
} | ||
|
||
var jwtBearerEventsType = Type.GetType(settings.JwtBearerEventsType, false); | ||
if (string.IsNullOrWhiteSpace(settings.OpenIdConnectEventsType)) | ||
{ | ||
configErrorsStringBuilder.AppendLine("The OpenIdConnectEventsType must be defined."); | ||
} | ||
|
||
if (string.IsNullOrWhiteSpace(settings.CookieAuthenticationEventsType)) | ||
if (string.IsNullOrWhiteSpace(settings.ResponseType)) | ||
{ | ||
throw new MissingFieldException("The CookieAuthenticationEventsType must be defined."); | ||
configErrorsStringBuilder.AppendLine("A ResponseType is mandatory to define the OpenId Connect protocol."); | ||
} | ||
var cookieAuthenticationEventsType = Type.GetType(settings.CookieAuthenticationEventsType, true); | ||
|
||
if (string.IsNullOrWhiteSpace(settings.OpenIdConnectEventsType)) | ||
if (configErrorsStringBuilder.Length > 0) | ||
{ | ||
throw new MissingFieldException("The OpenIdConnectEventsType must be defined."); | ||
throw new ConfigurationException(configErrorsStringBuilder.ToString()); | ||
} | ||
var openIdConnectEventsType = Type.GetType(settings.OpenIdConnectEventsType, false); | ||
|
||
var jwtBearerEventsType = Type.GetType(settings.JwtBearerEventsType, false); | ||
var cookieAuthenticationEventsType = Type.GetType(settings.CookieAuthenticationEventsType, true); | ||
var openIdConnectEventsType = Type.GetType(settings.OpenIdConnectEventsType, false); | ||
var certSecurityKey = string.IsNullOrWhiteSpace(settings.CertSecurityKeyPath) ? null : new X509CertificateLoader(null).FindCertificate(configuration, settings.CertSecurityKeyPath) ?? throw new MissingFieldException($"No certificate was found based on the configuration section: {settings.CertSecurityKeyPath}."); | ||
|
||
var cert = new X509CertificateLoader(null).FindCertificate(configuration, settings.CertificateSectionPath) ?? throw new MissingFieldException($"No certificate was found based on the configuration section: {settings.CertificateSectionPath}."); | ||
|
||
var ticketStoreAction = CacheTicketStoreExtension.PrepareAction(configuration, settings.AuthenticationCacheTicketStorePath); | ||
|
||
Type? cookiesConfigureOptionsType; | ||
|
@@ -254,11 +264,6 @@ public static AuthenticationBuilder AddOidcAuthentication(this IServiceCollectio | |
cookiesConfigureOptionsType = Type.GetType(settings.CookiesConfigureOptionsType, true); | ||
} | ||
|
||
if (string.IsNullOrWhiteSpace(settings.ResponseType)) | ||
{ | ||
throw new MissingFieldException("A ResponseType is mandatory to define the OpenId Connect protocol."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of throwining the exception at the end if the setting is missing Change is that |
||
} | ||
|
||
void OidcAuthenticationFiller(OidcAuthenticationOptions options) | ||
{ | ||
options.DefaultAuthority = settings.DefaultAuthority; | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Instead of throwining the exception at the end if the setting is missing
I have moved up the check so the error is added to the error list.
Change is that MissingFieldException would not e thrown anymore.