Mov secr az mrg - Githubissues

Don’t agree. It’s same as configurationmanager.appsettings[]. Azure located secrets can be acquired by Names and setting name, configuration file located settings as you did before by parameter name. Thoughts it’s shorten time for new parameters in configuration but if you think different.

From: Maxim Shklar notifications@github.com Sent: Thursday, March 14, 2019 4:36 PM To: shklar/SubMinimizer SubMinimizer@noreply.github.com Cc: Evgeny Vitenberg eviten@microsoft.com; Author author@noreply.github.com Subject: Re: [shklar/SubMinimizer] Mov secr az mrg (#90)

@shklar requested changes on this pull request.

In Shared/Settings.cshttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F90%23discussion_r265588774&data=02%7C01%7Ceviten%40microsoft.com%7C50234a9a65144459fb6e08d6a88a6ea1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636881709854295560&sdata=uCIUtXumBZUtx8dCTY0vTTNH1UqMU3vrniMLVQFT79c%3D&reserved=0:

if (settings.ApiKey != null &&

settings.AppClientId != null &&
settings.AppPassword != null &&
settings.DataAccessConnectionString != null &&
settings.WebJobDashboardConnectionString != null &&
settings.WebJobStorageConnectionString != null)
{
return true;
}
else
{
return false;
}
}
public string GetSetting(string valueName)

I don't understand this pattern. Why would we need to access setting by an arbitrary string name. Please remove this and use the explicit parameters instead all throughout the project

In CogsMinimizer/Controllers/AccountController.cshttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F90%23discussion_r265589026&data=02%7C01%7Ceviten%40microsoft.com%7C50234a9a65144459fb6e08d6a88a6ea1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636881709854305555&sdata=WMocZfS6ojQJbsyKMYJ6rhtkqmNIkeA3Um9Dm%2Bb4xLA%3D&reserved=0:

@@ -16,7 +18,7 @@ public void SignIn(string directoryName = "common")

         if (!Request.IsAuthenticated)

         {

             // note configuration (keys, etc…) will not necessarily understand this authority.

HttpContext.GetOwinContext().Environment.Add("Authority", string.Format(ConfigurationManager.AppSettings["ida:Authority"] + "OAuth2/Authorize", directoryName));
HttpContext.GetOwinContext().Environment.Add("Authority", string.Format(Settings.Instance.GetSetting("ida:Authority") + "OAuth2/Authorize", directoryName));

Replace with the explicit setting.

In CogsMinimizer/Web.confighttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F90%23discussion_r265589474&data=02%7C01%7Ceviten%40microsoft.com%7C50234a9a65144459fb6e08d6a88a6ea1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636881709854305555&sdata=0EdNDdyaL6QnQj78h9REsx4GkO6yvLF%2FEpZ4ZHGhIJQ%3D&reserved=0:

 <add name="DefaultConnection" connectionString="Data Source=(LocalDb)\MSSQLLocalDB;AttachDbFilename=|DataDirectory|\CogsMinimizer.mdf;Initial Catalog=CogsMinimizer_db;Integrated Security=True" providerName="System.Data.SqlClient" />

Please merge with latest main changes

In OfflineSubscriptionManager/EmailUtils.cshttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F90%23discussion_r265590461&data=02%7C01%7Ceviten%40microsoft.com%7C50234a9a65144459fb6e08d6a88a6ea1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636881709854315552&sdata=f6nkn9hWKgDRQv0T6%2FtSK2Kj27N5WkU17D91X6TdGtw%3D&reserved=0:

@@ -35,7 +35,7 @@ Subminimizer report

                         </head>

                         <body>";

string serviceURL = ConfigurationManager.AppSettings["env:ServiceURL"]; ;
string serviceURL = Settings.Instance.GetSetting("env:ServiceURL");

same

In OfflineSubscriptionManager/EmailUtils.cshttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F90%23discussion_r265590568&data=02%7C01%7Ceviten%40microsoft.com%7C50234a9a65144459fb6e08d6a88a6ea1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636881709854315552&sdata=lzUuO89o%2FLY%2Bw%2FEs%2F0mD5s5OvxzYC3emLeC4J7SiVJw%3D&reserved=0:

@@ -144,10 +144,10 @@ public static async Task SendEmail(SubMinimizerEmail email, ITracer tracer)

     {

         Diagnostics.EnsureArgumentNotNull(() => email);

         Diagnostics.EnsureArgumentNotNull(() => tracer);

string apiKey = ConfigurationManager.AppSettings["API_KEY"];
string apiKey = Settings.Instance.ApiKey;

This is how it should look like

In OfflineSubscriptionManager/Functions.cshttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F90%23discussion_r265593456&data=02%7C01%7Ceviten%40microsoft.com%7C50234a9a65144459fb6e08d6a88a6ea1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636881709854325541&sdata=MUjxaqfEBmrsw%2Fh1MFGbo61mvIznDtcuzn9T7piDDZo%3D&reserved=0:

         if (sub.SendEmailToCoadmins)

         {

             cc.AddRange(analysisResult.Admins.Select(x=>new Email(x)));

         }

//Add BCC recepients - dev team, as configured in the app config
string devTeam = ConfigurationManager.AppSettings["env:DevTeam"];
if (devTeam != null)
// Add BCC recepients - dev team, as configured in the app config
string devTeam = Settings.Instance.GetSetting("env:DevTeam");
if (devTeam != null && !devTeam.Equals("Dummy", StringComparison.OrdinalIgnoreCase))

Rather than comparing to "Dummy" why not validate that the given string is a valid concatenation of email addresses?

In Shared/Utilities.cshttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F90%23discussion_r265598050&data=02%7C01%7Ceviten%40microsoft.com%7C50234a9a65144459fb6e08d6a88a6ea1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636881709854325541&sdata=u6Tz29pJeTCi7vBJbtrBUoUv%2BYedhO9PIAImi%2F2Wb%2FY%3D&reserved=0:

///
/// Create Azure provider from given encoded content
///
/// Content

/// Provider

 public static Provider CreateProvider(string content)

Please use a more descriptive name. Like CreateJsonObjectProvider.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F90%23pullrequestreview-214534900&data=02%7C01%7Ceviten%40microsoft.com%7C50234a9a65144459fb6e08d6a88a6ea1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636881709854335536&sdata=bTPWnbJCDfjtASkUz%2FYiwXkgGr%2BSofj4vuabWitQYWc%3D&reserved=0, or mute the threadhttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FARCVLOFM_rmeQ2KXwKtHp0kkU9Y7Gt6gks5vWl5mgaJpZM4bkEv4&data=02%7C01%7Ceviten%40microsoft.com%7C50234a9a65144459fb6e08d6a88a6ea1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636881709854335536&sdata=Vx77YLAIRFEo5cIskPnH0D5cy6XAGocrF0qq8GdOC1Q%3D&reserved=0.

shklar / SubMinimizer

Mov secr az mrg #90