Issue 298310043: Add support for (whitelisted) dynamic assertion consumer urls (Closed)
|
Created:
7 years, 11 months ago by sfruhwald Modified:
7 years, 1 month ago CC:
connector-cr_google.com Visibility:
Public. |
DescriptionAdd support for (whitelisted) dynamic assertion consumer urls
Patch Set 1 #
Total comments: 16
Patch Set 2 : Add support for (whitelisted) dynamic assertion consumer urls #Patch Set 3 : Add support for (whitelisted) dynamic assertion consumer urls #
Total comments: 2
Patch Set 4 : Add support for (whitelisted) dynamic assertion consumer urls #Patch Set 5 : Add support for (whitelisted) dynamic assertion consumer urls #
Total comments: 15
Patch Set 6 : Add support for (whitelisted) dynamic assertion consumer urls #
Total comments: 9
Patch Set 7 : Add support for (whitelisted) dynamic assertion consumer urls #
Total comments: 1
Patch Set 8 : Add support for (whitelisted) dynamic assertion consumer urls #
Total comments: 1
Patch Set 9 : Add support for (whitelisted) dynamic assertion consumer urls #
MessagesTotal messages: 31
Hi Szabi, Please have either Jeff Ling, Ondra, Marc, or John Lacey for this review. Thank you, PJ - technology's compounding interest - On Wed, May 25, 2016 at 5:03 PM, <reply@codereview-hr.appspotmail.com> wrote: > Reviewers: pjo, > > Description: > Add support for (whitelisted) dynamic assertion consumer urls > > Please review this at http://codereview.appspot.com/298310043/ > > Affected files (+194, -158 lines): > M SAML Bridge.sln > M gsa-simulator/App_Code/Common.cs > M gsa-simulator/Web.config > M saml-bridge/App_Code/AuthImpl.cs > M saml-bridge/App_Code/AuthenticationPage.cs > M saml-bridge/App_Code/Common.cs > M saml-bridge/App_Code/Global.asax.cs > M saml-bridge/App_Code/ImpSM.cs > M saml-bridge/App_Code/SamlArtifactCacheEntry.cs > A saml-bridge/App_Code/SamlAssertionConsumerResolver.cs > M saml-bridge/App_Code/SsoAuthImpl.cs > M saml-bridge/Login.aspx.cs > M saml-bridge/Post.aspx.cs > M saml-bridge/Resolve.aspx.cs > M saml-bridge/Web.config > > >
Sign in to reply to this message.
-PJ +Ondra It would have been nice to split out the VS upgrade. At least mention that in the description/commit log, and also, can you explain ConfigurationSettings to ConfigurationManager change? Presumably it was related to the VS upgrade along with the config file changes and removal of unused variables, and added new modifiers? I didn't look at the logic, just the boring stuff. John L https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Authenti... File saml-bridge/App_Code/AuthenticationPage.cs (right): https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Authenti... saml-bridge/App_Code/AuthenticationPage.cs:23: private string _id, _issuer, _assertionConsumerServiceURL; String seems to be much preferred, and there are some cases here where you changed String to string. https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.cs File saml-bridge/App_Code/Common.cs (right): https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.c... saml-bridge/App_Code/Common.cs:124: if(allowedAssertionConsumers == null) Add space after "if" and other keywords. https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.c... saml-bridge/App_Code/Common.cs:126: allowedAssertionConsumers = new List<String>(); Why not make it an empty list in the first place if you're going to create one here anyway? https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... File saml-bridge/App_Code/Global.asax.cs (right): https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... saml-bridge/App_Code/Global.asax.cs:88: if(val != null && val.Length > 0) How can val be null here? If we need such a check, it seems like it would have come before the call to Split. https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... saml-bridge/App_Code/Global.asax.cs:155: if (store.Certificates[i].FriendlyName.Equals(ConfigurationManager.AppSettings["certificate_friendly_name"])) Added or edited lines should wrap at 100 cols. https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SamlAsse... File saml-bridge/App_Code/SamlAssertionConsumerResolver.cs (right): https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SamlAsse... saml-bridge/App_Code/SamlAssertionConsumerResolver.cs:2: * Copyright (C) 2006-2016 Google Inc. * Copyright 2016 Google Inc. All Rights Reserved. This has morphed over the years, but the current format is only the year the file was added, with "All Rights Reserved". https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SsoAuthI... File saml-bridge/App_Code/SsoAuthImpl.cs (right): https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SsoAuthI... saml-bridge/App_Code/SsoAuthImpl.cs:36: IMP_METHOD = ConfigurationManager.AppSettings["impersonation_method"]; Delete extra space before = while you're here.
Sign in to reply to this message.
Couple of notes - did you test this with POST binding? - it seems you have changed line endings in those files, so the patch does not apply cleanly - can you revert that. On Thu, May 26, 2016 at 3:36 AM, <jlacey@google.com> wrote: > -PJ > +Ondra > > It would have been nice to split out the VS upgrade. At least mention > that in the description/commit log, and also, can you explain > ConfigurationSettings to ConfigurationManager change? Presumably it was > related to the VS upgrade along with the config file changes and removal > of unused variables, and added new modifiers? > > I didn't look at the logic, just the boring stuff. > > John L > > > > https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Authenti... > File saml-bridge/App_Code/AuthenticationPage.cs (right): > > > https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Authenti... > saml-bridge/App_Code/AuthenticationPage.cs:23: private string _id, > _issuer, _assertionConsumerServiceURL; > String seems to be much preferred, and there are some cases here where > you changed String to string. > > > https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.cs > File saml-bridge/App_Code/Common.cs (right): > > > https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.c... > saml-bridge/App_Code/Common.cs:124: if(allowedAssertionConsumers == > null) > Add space after "if" and other keywords. > > > https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.c... > saml-bridge/App_Code/Common.cs:126: allowedAssertionConsumers = new > List<String>(); > Why not make it an empty list in the first place if you're going to > create one here anyway? > > > https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... > File saml-bridge/App_Code/Global.asax.cs (right): > > > https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... > saml-bridge/App_Code/Global.asax.cs:88: if(val != null && val.Length > > 0) > How can val be null here? If we need such a check, it seems like it > would have come before the call to Split. > > > https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... > saml-bridge/App_Code/Global.asax.cs:155: if > > (store.Certificates[i].FriendlyName.Equals(ConfigurationManager.AppSettings["certificate_friendly_name"])) > Added or edited lines should wrap at 100 cols. > > > https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SamlAsse... > File saml-bridge/App_Code/SamlAssertionConsumerResolver.cs (right): > > > https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SamlAsse... > saml-bridge/App_Code/SamlAssertionConsumerResolver.cs:2: * Copyright (C) > 2006-2016 Google Inc. > * Copyright 2016 Google Inc. All Rights Reserved. > > This has morphed over the years, but the current format is only the year > the file was added, with "All Rights Reserved". > > > https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SsoAuthI... > File saml-bridge/App_Code/SsoAuthImpl.cs (right): > > > https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SsoAuthI... > saml-bridge/App_Code/SsoAuthImpl.cs:36: IMP_METHOD = > ConfigurationManager.AppSettings["impersonation_method"]; > Delete extra space before = while you're here. > > https://codereview.appspot.com/298310043/ >
Sign in to reply to this message.
- Yes, I tested POST binding too and it works well - The line endings: it is tricky as made the changes on windows, I had to copy over the cloned git repo, make the changes and copy back to Goobuntu as the Windows Server instance has firewalled internet connection.. But yes, I realized and I will figure out something to change those back On Thu, May 26, 2016 at 5:29 AM, Ondrej Novak <ondrejnovak@google.com> wrote: > Couple of notes > - did you test this with POST binding? > - it seems you have changed line endings in those files, so the patch does > not apply cleanly - can you revert that. > > On Thu, May 26, 2016 at 3:36 AM, <jlacey@google.com> wrote: > >> -PJ >> +Ondra >> >> It would have been nice to split out the VS upgrade. At least mention >> that in the description/commit log, and also, can you explain >> ConfigurationSettings to ConfigurationManager change? Presumably it was >> related to the VS upgrade along with the config file changes and removal >> of unused variables, and added new modifiers? >> >> I didn't look at the logic, just the boring stuff. >> >> John L >> >> >> >> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Authenti... >> File saml-bridge/App_Code/AuthenticationPage.cs (right): >> >> >> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Authenti... >> saml-bridge/App_Code/AuthenticationPage.cs:23: private string _id, >> _issuer, _assertionConsumerServiceURL; >> String seems to be much preferred, and there are some cases here where >> you changed String to string. >> >> >> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.cs >> File saml-bridge/App_Code/Common.cs (right): >> >> >> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.c... >> saml-bridge/App_Code/Common.cs:124: if(allowedAssertionConsumers == >> null) >> Add space after "if" and other keywords. >> >> >> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.c... >> saml-bridge/App_Code/Common.cs:126: allowedAssertionConsumers = new >> List<String>(); >> Why not make it an empty list in the first place if you're going to >> create one here anyway? >> >> >> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... >> File saml-bridge/App_Code/Global.asax.cs (right): >> >> >> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... >> saml-bridge/App_Code/Global.asax.cs:88: if(val != null && val.Length > >> 0) >> How can val be null here? If we need such a check, it seems like it >> would have come before the call to Split. >> >> >> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... >> saml-bridge/App_Code/Global.asax.cs:155: if >> >> (store.Certificates[i].FriendlyName.Equals(ConfigurationManager.AppSettings["certificate_friendly_name"])) >> Added or edited lines should wrap at 100 cols. >> >> >> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SamlAsse... >> File saml-bridge/App_Code/SamlAssertionConsumerResolver.cs (right): >> >> >> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SamlAsse... >> saml-bridge/App_Code/SamlAssertionConsumerResolver.cs:2: * Copyright (C) >> 2006-2016 Google Inc. >> * Copyright 2016 Google Inc. All Rights Reserved. >> >> This has morphed over the years, but the current format is only the year >> the file was added, with "All Rights Reserved". >> >> >> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SsoAuthI... >> File saml-bridge/App_Code/SsoAuthImpl.cs (right): >> >> >> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SsoAuthI... >> saml-bridge/App_Code/SsoAuthImpl.cs:36: IMP_METHOD = >> ConfigurationManager.AppSettings["impersonation_method"]; >> Delete extra space before = while you're here. >> >> https://codereview.appspot.com/298310043/ >> > >
Sign in to reply to this message.
Okay, so just zip up the git repository and send it to me :). I tried to apply the patch from codereview and failed. On Thu, May 26, 2016 at 6:49 PM, Szabolcs Fruhwald <sfruhwald@google.com> wrote: > - Yes, I tested POST binding too and it works well > - The line endings: it is tricky as made the changes on windows, I had to > copy over the cloned git repo, make the changes and copy back to Goobuntu > as the Windows Server instance has firewalled internet connection.. But > yes, I realized and I will figure out something to change those back > > On Thu, May 26, 2016 at 5:29 AM, Ondrej Novak <ondrejnovak@google.com> > wrote: > >> Couple of notes >> - did you test this with POST binding? >> - it seems you have changed line endings in those files, so the patch >> does not apply cleanly - can you revert that. >> >> On Thu, May 26, 2016 at 3:36 AM, <jlacey@google.com> wrote: >> >>> -PJ >>> +Ondra >>> >>> It would have been nice to split out the VS upgrade. At least mention >>> that in the description/commit log, and also, can you explain >>> ConfigurationSettings to ConfigurationManager change? Presumably it was >>> related to the VS upgrade along with the config file changes and removal >>> of unused variables, and added new modifiers? >>> >>> I didn't look at the logic, just the boring stuff. >>> >>> John L >>> >>> >>> >>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Authenti... >>> File saml-bridge/App_Code/AuthenticationPage.cs (right): >>> >>> >>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Authenti... >>> saml-bridge/App_Code/AuthenticationPage.cs:23: private string _id, >>> _issuer, _assertionConsumerServiceURL; >>> String seems to be much preferred, and there are some cases here where >>> you changed String to string. >>> >>> >>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.cs >>> File saml-bridge/App_Code/Common.cs (right): >>> >>> >>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.c... >>> saml-bridge/App_Code/Common.cs:124: if(allowedAssertionConsumers == >>> null) >>> Add space after "if" and other keywords. >>> >>> >>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.c... >>> saml-bridge/App_Code/Common.cs:126: allowedAssertionConsumers = new >>> List<String>(); >>> Why not make it an empty list in the first place if you're going to >>> create one here anyway? >>> >>> >>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... >>> File saml-bridge/App_Code/Global.asax.cs (right): >>> >>> >>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... >>> saml-bridge/App_Code/Global.asax.cs:88: if(val != null && val.Length > >>> 0) >>> How can val be null here? If we need such a check, it seems like it >>> would have come before the call to Split. >>> >>> >>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... >>> saml-bridge/App_Code/Global.asax.cs:155: if >>> >>> (store.Certificates[i].FriendlyName.Equals(ConfigurationManager.AppSettings["certificate_friendly_name"])) >>> Added or edited lines should wrap at 100 cols. >>> >>> >>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SamlAsse... >>> File saml-bridge/App_Code/SamlAssertionConsumerResolver.cs (right): >>> >>> >>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SamlAsse... >>> saml-bridge/App_Code/SamlAssertionConsumerResolver.cs:2: * Copyright (C) >>> 2006-2016 Google Inc. >>> * Copyright 2016 Google Inc. All Rights Reserved. >>> >>> This has morphed over the years, but the current format is only the year >>> the file was added, with "All Rights Reserved". >>> >>> >>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SsoAuthI... >>> File saml-bridge/App_Code/SsoAuthImpl.cs (right): >>> >>> >>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SsoAuthI... >>> saml-bridge/App_Code/SsoAuthImpl.cs:36: IMP_METHOD = >>> ConfigurationManager.AppSettings["impersonation_method"]; >>> Delete extra space before = while you're here. >>> >>> https://codereview.appspot.com/298310043/ >>> >> >> >
Sign in to reply to this message.
git repo: http://sfruhwald0.mtv.corp.google.com/pub/samlbridge.zip built binaries: http://sfruhwald0.mtv.corp.google.com/pub/samlbridge-binaries.zip On Thu, May 26, 2016 at 9:50 AM, Ondrej Novak <ondrejnovak@google.com> wrote: > Okay, so just zip up the git repository and send it to me :). > > I tried to apply the patch from codereview and failed. > > On Thu, May 26, 2016 at 6:49 PM, Szabolcs Fruhwald <sfruhwald@google.com> > wrote: > >> - Yes, I tested POST binding too and it works well >> - The line endings: it is tricky as made the changes on windows, I had to >> copy over the cloned git repo, make the changes and copy back to Goobuntu >> as the Windows Server instance has firewalled internet connection.. But >> yes, I realized and I will figure out something to change those back >> >> On Thu, May 26, 2016 at 5:29 AM, Ondrej Novak <ondrejnovak@google.com> >> wrote: >> >>> Couple of notes >>> - did you test this with POST binding? >>> - it seems you have changed line endings in those files, so the patch >>> does not apply cleanly - can you revert that. >>> >>> On Thu, May 26, 2016 at 3:36 AM, <jlacey@google.com> wrote: >>> >>>> -PJ >>>> +Ondra >>>> >>>> It would have been nice to split out the VS upgrade. At least mention >>>> that in the description/commit log, and also, can you explain >>>> ConfigurationSettings to ConfigurationManager change? Presumably it was >>>> related to the VS upgrade along with the config file changes and removal >>>> of unused variables, and added new modifiers? >>>> >>>> I didn't look at the logic, just the boring stuff. >>>> >>>> John L >>>> >>>> >>>> >>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Authenti... >>>> File saml-bridge/App_Code/AuthenticationPage.cs (right): >>>> >>>> >>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Authenti... >>>> saml-bridge/App_Code/AuthenticationPage.cs:23: private string _id, >>>> _issuer, _assertionConsumerServiceURL; >>>> String seems to be much preferred, and there are some cases here where >>>> you changed String to string. >>>> >>>> >>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.cs >>>> File saml-bridge/App_Code/Common.cs (right): >>>> >>>> >>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.c... >>>> saml-bridge/App_Code/Common.cs:124: if(allowedAssertionConsumers == >>>> null) >>>> Add space after "if" and other keywords. >>>> >>>> >>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.c... >>>> saml-bridge/App_Code/Common.cs:126: allowedAssertionConsumers = new >>>> List<String>(); >>>> Why not make it an empty list in the first place if you're going to >>>> create one here anyway? >>>> >>>> >>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... >>>> File saml-bridge/App_Code/Global.asax.cs (right): >>>> >>>> >>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... >>>> saml-bridge/App_Code/Global.asax.cs:88: if(val != null && val.Length > >>>> 0) >>>> How can val be null here? If we need such a check, it seems like it >>>> would have come before the call to Split. >>>> >>>> >>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... >>>> saml-bridge/App_Code/Global.asax.cs:155: if >>>> >>>> (store.Certificates[i].FriendlyName.Equals(ConfigurationManager.AppSettings["certificate_friendly_name"])) >>>> Added or edited lines should wrap at 100 cols. >>>> >>>> >>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SamlAsse... >>>> File saml-bridge/App_Code/SamlAssertionConsumerResolver.cs (right): >>>> >>>> >>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SamlAsse... >>>> saml-bridge/App_Code/SamlAssertionConsumerResolver.cs:2: * Copyright (C) >>>> 2006-2016 Google Inc. >>>> * Copyright 2016 Google Inc. All Rights Reserved. >>>> >>>> This has morphed over the years, but the current format is only the year >>>> the file was added, with "All Rights Reserved". >>>> >>>> >>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SsoAuthI... >>>> File saml-bridge/App_Code/SsoAuthImpl.cs (right): >>>> >>>> >>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SsoAuthI... >>>> saml-bridge/App_Code/SsoAuthImpl.cs:36: IMP_METHOD = >>>> ConfigurationManager.AppSettings["impersonation_method"]; >>>> Delete extra space before = while you're here. >>>> >>>> https://codereview.appspot.com/298310043/ >>>> >>> >>> >> >
Sign in to reply to this message.
Thanks, will upload the updates files soon. https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Authenti... File saml-bridge/App_Code/AuthenticationPage.cs (right): https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Authenti... saml-bridge/App_Code/AuthenticationPage.cs:23: private string _id, _issuer, _assertionConsumerServiceURL; On 2016/05/26 01:36:59, JohnL wrote: > String seems to be much preferred, and there are some cases here where you > changed String to string. VS 2015 suggested to change it to string, there is no functional difference btw. It seems 'string' is the c# convention over String, this also saves an import line, but it is not important.. So I can roll these back if you think so to keep consistency. https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.cs File saml-bridge/App_Code/Common.cs (right): https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.c... saml-bridge/App_Code/Common.cs:124: if(allowedAssertionConsumers == null) On 2016/05/26 01:36:59, JohnL wrote: > Add space after "if" and other keywords. Acknowledged. https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.c... saml-bridge/App_Code/Common.cs:126: allowedAssertionConsumers = new List<String>(); On 2016/05/26 01:36:59, JohnL wrote: > Why not make it an empty list in the first place if you're going to create one > here anyway? Yes, but then the field needs to be set to private (they should all be private here anyway..) to make sure no one can overwrite it with a null. Done. https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... File saml-bridge/App_Code/Global.asax.cs (right): https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... saml-bridge/App_Code/Global.asax.cs:88: if(val != null && val.Length > 0) On 2016/05/26 01:36:59, JohnL wrote: > How can val be null here? If we need such a check, it seems like it would have > come before the call to Split. You are right, it can't be, so just a bad habit :) Removed. https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... saml-bridge/App_Code/Global.asax.cs:155: if (store.Certificates[i].FriendlyName.Equals(ConfigurationManager.AppSettings["certificate_friendly_name"])) On 2016/05/26 01:36:59, JohnL wrote: > Added or edited lines should wrap at 100 cols. Done. https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SamlAsse... File saml-bridge/App_Code/SamlAssertionConsumerResolver.cs (right): https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SamlAsse... saml-bridge/App_Code/SamlAssertionConsumerResolver.cs:2: * Copyright (C) 2006-2016 Google Inc. On 2016/05/26 01:37:00, JohnL wrote: > * Copyright 2016 Google Inc. All Rights Reserved. > > This has morphed over the years, but the current format is only the year the > file was added, with "All Rights Reserved". Done. https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SsoAuthI... File saml-bridge/App_Code/SsoAuthImpl.cs (right): https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SsoAuthI... saml-bridge/App_Code/SsoAuthImpl.cs:36: IMP_METHOD = ConfigurationManager.AppSettings["impersonation_method"]; On 2016/05/26 01:37:00, JohnL wrote: > Delete extra space before = while you're here. Done.
Sign in to reply to this message.
Should we (in a separate CL) change the file line endings in Git to CRLF? I think we did that for Search Box and/or Connector Services in Subversion, but those settings were lost in the transition to Git. John L On Thu, May 26, 2016 at 10:02 AM Szabolcs Fruhwald <sfruhwald@google.com> wrote: > git repo: http://sfruhwald0.mtv.corp.google.com/pub/samlbridge.zip > built binaries: > http://sfruhwald0.mtv.corp.google.com/pub/samlbridge-binaries.zip > > On Thu, May 26, 2016 at 9:50 AM, Ondrej Novak <ondrejnovak@google.com> > wrote: > >> Okay, so just zip up the git repository and send it to me :). >> >> I tried to apply the patch from codereview and failed. >> >> On Thu, May 26, 2016 at 6:49 PM, Szabolcs Fruhwald <sfruhwald@google.com> >> wrote: >> >>> - Yes, I tested POST binding too and it works well >>> - The line endings: it is tricky as made the changes on windows, I had >>> to copy over the cloned git repo, make the changes and copy back to >>> Goobuntu as the Windows Server instance has firewalled internet >>> connection.. But yes, I realized and I will figure out something to change >>> those back >>> >>> On Thu, May 26, 2016 at 5:29 AM, Ondrej Novak <ondrejnovak@google.com> >>> wrote: >>> >>>> Couple of notes >>>> - did you test this with POST binding? >>>> - it seems you have changed line endings in those files, so the patch >>>> does not apply cleanly - can you revert that. >>>> >>>> On Thu, May 26, 2016 at 3:36 AM, <jlacey@google.com> wrote: >>>> >>>>> -PJ >>>>> +Ondra >>>>> >>>>> It would have been nice to split out the VS upgrade. At least mention >>>>> that in the description/commit log, and also, can you explain >>>>> ConfigurationSettings to ConfigurationManager change? Presumably it was >>>>> related to the VS upgrade along with the config file changes and >>>>> removal >>>>> of unused variables, and added new modifiers? >>>>> >>>>> I didn't look at the logic, just the boring stuff. >>>>> >>>>> John L >>>>> >>>>> >>>>> >>>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Authenti... >>>>> File saml-bridge/App_Code/AuthenticationPage.cs (right): >>>>> >>>>> >>>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Authenti... >>>>> saml-bridge/App_Code/AuthenticationPage.cs:23: private string _id, >>>>> _issuer, _assertionConsumerServiceURL; >>>>> String seems to be much preferred, and there are some cases here where >>>>> you changed String to string. >>>>> >>>>> >>>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.cs >>>>> File saml-bridge/App_Code/Common.cs (right): >>>>> >>>>> >>>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.c... >>>>> saml-bridge/App_Code/Common.cs:124: if(allowedAssertionConsumers == >>>>> null) >>>>> Add space after "if" and other keywords. >>>>> >>>>> >>>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Common.c... >>>>> saml-bridge/App_Code/Common.cs:126: allowedAssertionConsumers = new >>>>> List<String>(); >>>>> Why not make it an empty list in the first place if you're going to >>>>> create one here anyway? >>>>> >>>>> >>>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... >>>>> File saml-bridge/App_Code/Global.asax.cs (right): >>>>> >>>>> >>>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... >>>>> saml-bridge/App_Code/Global.asax.cs:88: if(val != null && val.Length > >>>>> 0) >>>>> How can val be null here? If we need such a check, it seems like it >>>>> would have come before the call to Split. >>>>> >>>>> >>>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Global.a... >>>>> saml-bridge/App_Code/Global.asax.cs:155: if >>>>> >>>>> (store.Certificates[i].FriendlyName.Equals(ConfigurationManager.AppSettings["certificate_friendly_name"])) >>>>> Added or edited lines should wrap at 100 cols. >>>>> >>>>> >>>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SamlAsse... >>>>> File saml-bridge/App_Code/SamlAssertionConsumerResolver.cs (right): >>>>> >>>>> >>>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SamlAsse... >>>>> saml-bridge/App_Code/SamlAssertionConsumerResolver.cs:2: * Copyright >>>>> (C) >>>>> 2006-2016 Google Inc. >>>>> * Copyright 2016 Google Inc. All Rights Reserved. >>>>> >>>>> This has morphed over the years, but the current format is only the >>>>> year >>>>> the file was added, with "All Rights Reserved". >>>>> >>>>> >>>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SsoAuthI... >>>>> File saml-bridge/App_Code/SsoAuthImpl.cs (right): >>>>> >>>>> >>>>> https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SsoAuthI... >>>>> saml-bridge/App_Code/SsoAuthImpl.cs:36: IMP_METHOD = >>>>> ConfigurationManager.AppSettings["impersonation_method"]; >>>>> Delete extra space before = while you're here. >>>>> >>>>> https://codereview.appspot.com/298310043/ >>>>> >>>> >>>> >>> >> >
Sign in to reply to this message.
https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Authenti... File saml-bridge/App_Code/AuthenticationPage.cs (right): https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Authenti... saml-bridge/App_Code/AuthenticationPage.cs:23: private string _id, _issuer, _assertionConsumerServiceURL; On 2016/05/26 17:32:15, sfruhwald wrote: > On 2016/05/26 01:36:59, JohnL wrote: > > String seems to be much preferred, and there are some cases here where you > > changed String to string. > > VS 2015 suggested to change it to string, there is no functional difference btw. > It seems 'string' is the c# convention over String, this also saves an import > line, but it is not important.. So I can roll these back if you think so to keep > consistency. I looked at this closer, and Microsoft seems to recommend the string alias for objects. I'm happy to make that shift in new code, but I'd prefer not to change individual, otherwise unmodified lines. We could make a sweep in a separate CL if it pleases VS (i.e., silences warnings).
Sign in to reply to this message.
+1 for John's comments to seperate this CL into parts. There are several things going on in this CL in addition to support of multivalued element. The language updates, editor updates, newlines. Seperated CLs are easier to review, revert as units, are easier to understand when viewing history. Thank you, PJ - technology's compounding interest - On Thu, May 26, 2016 at 10:48 AM, <jlacey@google.com> wrote: > > > https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Authenti... > File saml-bridge/App_Code/AuthenticationPage.cs (right): > > > https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/Authenti... > saml-bridge/App_Code/AuthenticationPage.cs:23: private string _id, > _issuer, _assertionConsumerServiceURL; > On 2016/05/26 17:32:15, sfruhwald wrote: > >> On 2016/05/26 01:36:59, JohnL wrote: >> > String seems to be much preferred, and there are some cases here >> > where you > >> > changed String to string. >> > > VS 2015 suggested to change it to string, there is no functional >> > difference btw. > >> It seems 'string' is the c# convention over String, this also saves an >> > import > >> line, but it is not important.. So I can roll these back if you think >> > so to keep > >> consistency. >> > > I looked at this closer, and Microsoft seems to recommend the string > alias for objects. I'm happy to make that shift in new code, but I'd > prefer not to change individual, otherwise unmodified lines. We could > make a sweep in a separate CL if it pleases VS (i.e., silences > warnings). > > https://codereview.appspot.com/298310043/ > > -- > >
Sign in to reply to this message.
My changes since first patch set. Please ignore patch set #2. The .sln file versuin update has been extracted to a separate commit with comment: Update solution file to VS2010+ format.
Sign in to reply to this message.
https://codereview.appspot.com/298310043/diff/40001/saml-bridge/App_Code/Saml... File saml-bridge/App_Code/SamlAssertionConsumerResolver.cs (right): https://codereview.appspot.com/298310043/diff/40001/saml-bridge/App_Code/Saml... saml-bridge/App_Code/SamlAssertionConsumerResolver.cs:34: Common.error("Not allowed AssertionConsumerServiceURL value in SAML Request. Please check assertion_consumer value in Web.config."); Can you log the rejected assertionconsumerservice URL?
Sign in to reply to this message.
Added the request's assertionConsumerURL to the log; After discussing with PJ, I moved the Web.config obsolete node changes (not related to this feature) to the commit of updated solution file. I create a separate review for that.
Sign in to reply to this message.
https://codereview.appspot.com/298310043/diff/40001/saml-bridge/App_Code/Saml... File saml-bridge/App_Code/SamlAssertionConsumerResolver.cs (right): https://codereview.appspot.com/298310043/diff/40001/saml-bridge/App_Code/Saml... saml-bridge/App_Code/SamlAssertionConsumerResolver.cs:34: Common.error("Not allowed AssertionConsumerServiceURL value in SAML Request. Please check assertion_consumer value in Web.config."); On 2016/06/01 12:44:51, ondrejnovak wrote: > Can you log the rejected assertionconsumerservice URL? Done.
Sign in to reply to this message.
This is languishing and Ondra's in another time zone. Tanmay, can you please take a look? John L https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SamlAsse... File saml-bridge/App_Code/SamlAssertionConsumerResolver.cs (right): https://codereview.appspot.com/298310043/diff/1/saml-bridge/App_Code/SamlAsse... saml-bridge/App_Code/SamlAssertionConsumerResolver.cs:2: * Copyright (C) 2006-2016 Google Inc. On 2016/05/26 17:32:15, sfruhwald wrote: > On 2016/05/26 01:37:00, JohnL wrote: > > * Copyright 2016 Google Inc. All Rights Reserved. > > > > This has morphed over the years, but the current format is only the year the > > file was added, with "All Rights Reserved". > > Done. Not done.
Sign in to reply to this message.
Fixed copyright line in SamlAssertionConsumerResolver.cs
Sign in to reply to this message.
On 2016/06/02 23:57:55, sfruhwald wrote: > Fixed copyright line in SamlAssertionConsumerResolver.cs Ping
Sign in to reply to this message.
Just trivial comments, except the version number. If no one else is willing to look at it I'll OK the logic. John L https://codereview.appspot.com/298310043/diff/80001/saml-bridge/App_Code/Glob... File saml-bridge/App_Code/Global.asax.cs (right): https://codereview.appspot.com/298310043/diff/80001/saml-bridge/App_Code/Glob... saml-bridge/App_Code/Global.asax.cs:85: foreach(String s in ConfigurationManager.AppSettings["assertion_consumer"].Split(',')) Add space after "foreach". Looks like that might put it over 100 cols. https://codereview.appspot.com/298310043/diff/80001/saml-bridge/App_Code/Glob... saml-bridge/App_Code/Global.asax.cs:88: if(val.Length > 0) Add space after "if". https://codereview.appspot.com/298310043/diff/80001/saml-bridge/App_Code/Saml... File saml-bridge/App_Code/SamlArtifactCacheEntry.cs (right): https://codereview.appspot.com/298310043/diff/80001/saml-bridge/App_Code/Saml... saml-bridge/App_Code/SamlArtifactCacheEntry.cs:34: public SamlArtifactCacheEntry(string subject, string authNRequestId, string samlAssertionConsumerURL) 100 cols https://codereview.appspot.com/298310043/diff/80001/saml-bridge/App_Code/Saml... File saml-bridge/App_Code/SamlAssertionConsumerResolver.cs (right): https://codereview.appspot.com/298310043/diff/80001/saml-bridge/App_Code/Saml... saml-bridge/App_Code/SamlAssertionConsumerResolver.cs:25: foreach(string allowedUrl in Common.AllowedAssertionConsumers) Add space after "foreach". https://codereview.appspot.com/298310043/diff/80001/saml-bridge/App_Code/Saml... saml-bridge/App_Code/SamlAssertionConsumerResolver.cs:34: Common.error("Not allowed AssertionConsumerServiceURL value in SAML Request: " s/Not allowed/Disallowed/ in both strings. https://codereview.appspot.com/298310043/diff/80001/saml-bridge/Login.aspx.cs File saml-bridge/Login.aspx.cs (right): https://codereview.appspot.com/298310043/diff/80001/saml-bridge/Login.aspx.cs... saml-bridge/Login.aspx.cs:96: gsa = SamlAssertionConsumerValidator.GetValidURL(authNRequest) + Move + to the beginning of the next line. https://codereview.appspot.com/298310043/diff/80001/saml-bridge/Web.config File saml-bridge/Web.config (right): https://codereview.appspot.com/298310043/diff/80001/saml-bridge/Web.config#ne... saml-bridge/Web.config:95: <add key="log_level" value="error"/> <!-- debug, error, with debug being the most detailed level --> Maybe undo this change. The comment on the line after (which drives me nuts) seems consistent here. https://codereview.appspot.com/298310043/diff/80001/saml-bridge/Web.config#ne... saml-bridge/Web.config:96: <!-- Since v3.1.0 SAML Bridge takes this URL from the SAML Request's AssertionConsumerServiceURL attribute in order to support multi-host GSA setup. Is "3.1.0" intended to be the next version of SAML Bridge? If so, I think 4.1.1 is the correct version number. https://codereview.appspot.com/298310043/diff/80001/saml-bridge/Web.config#ne... saml-bridge/Web.config:96: <!-- Since v3.1.0 SAML Bridge takes this URL from the SAML Request's AssertionConsumerServiceURL attribute in order to support multi-host GSA setup. 100 cols throughout for the comments at least, and for the XML if the line breaks are OK.
Sign in to reply to this message.
Thank you, I also added an entry to update the RELEASE_NOTES.txt to 4.1.1, as JohnL requested (to match other connectors' version). https://codereview.appspot.com/298310043/diff/80001/saml-bridge/App_Code/Glob... File saml-bridge/App_Code/Global.asax.cs (right): https://codereview.appspot.com/298310043/diff/80001/saml-bridge/App_Code/Glob... saml-bridge/App_Code/Global.asax.cs:85: foreach(String s in ConfigurationManager.AppSettings["assertion_consumer"].Split(',')) On 2016/07/22 10:40:35, JohnL wrote: > Add space after "foreach". Looks like that might put it over 100 cols. Done. https://codereview.appspot.com/298310043/diff/80001/saml-bridge/App_Code/Glob... saml-bridge/App_Code/Global.asax.cs:88: if(val.Length > 0) On 2016/07/22 10:40:36, JohnL wrote: > Add space after "if". Done. https://codereview.appspot.com/298310043/diff/80001/saml-bridge/App_Code/Saml... File saml-bridge/App_Code/SamlArtifactCacheEntry.cs (right): https://codereview.appspot.com/298310043/diff/80001/saml-bridge/App_Code/Saml... saml-bridge/App_Code/SamlArtifactCacheEntry.cs:34: public SamlArtifactCacheEntry(string subject, string authNRequestId, string samlAssertionConsumerURL) On 2016/07/22 10:40:36, JohnL wrote: > 100 cols Done. https://codereview.appspot.com/298310043/diff/80001/saml-bridge/App_Code/Saml... File saml-bridge/App_Code/SamlAssertionConsumerResolver.cs (right): https://codereview.appspot.com/298310043/diff/80001/saml-bridge/App_Code/Saml... saml-bridge/App_Code/SamlAssertionConsumerResolver.cs:25: foreach(string allowedUrl in Common.AllowedAssertionConsumers) On 2016/07/22 10:40:36, JohnL wrote: > Add space after "foreach". Done. https://codereview.appspot.com/298310043/diff/80001/saml-bridge/App_Code/Saml... saml-bridge/App_Code/SamlAssertionConsumerResolver.cs:34: Common.error("Not allowed AssertionConsumerServiceURL value in SAML Request: " On 2016/07/22 10:40:36, JohnL wrote: > s/Not allowed/Disallowed/ in both strings. Done. https://codereview.appspot.com/298310043/diff/80001/saml-bridge/Login.aspx.cs File saml-bridge/Login.aspx.cs (right): https://codereview.appspot.com/298310043/diff/80001/saml-bridge/Login.aspx.cs... saml-bridge/Login.aspx.cs:96: gsa = SamlAssertionConsumerValidator.GetValidURL(authNRequest) + On 2016/07/22 10:40:36, JohnL wrote: > Move + to the beginning of the next line. Done.
Sign in to reply to this message.
Thank you JohnL, - I fixed your previous comments, updated version to 4.1.1, - added relnotes entry and - added Lavanya to review as well (as she has better C# readability)
Sign in to reply to this message.
A few suggestions. John L https://codereview.appspot.com/298310043/diff/100001/RELEASE_NOTE.txt File RELEASE_NOTE.txt (right): https://codereview.appspot.com/298310043/diff/100001/RELEASE_NOTE.txt#newcode11 RELEASE_NOTE.txt:11: Release 4.1.1 December 8, 2016 I would generally put the release notes in a separate CL. https://codereview.appspot.com/298310043/diff/100001/saml-bridge/App_Code/Glo... File saml-bridge/App_Code/Global.asax.cs (right): https://codereview.appspot.com/298310043/diff/100001/saml-bridge/App_Code/Glo... saml-bridge/App_Code/Global.asax.cs:85: foreach (String s in ConfigurationManager.AppSettings["assertion_consumer"].Split(',')) Do we need to check for a missing assertion_consumer property? Did null used to "work" in the sense of not throwing exceptions? https://codereview.appspot.com/298310043/diff/100001/saml-bridge/Web.config File saml-bridge/Web.config (right): https://codereview.appspot.com/298310043/diff/100001/saml-bridge/Web.config#n... saml-bridge/Web.config:98: <!-- Since v4.1.1 SAML Bridge takes this URL from the SAML Request's AssertionConsumerServiceURL This reads a little funny, and suggests that the default value is taken from the SAML request. Maybe just s/takes this URL from/uses/? https://codereview.appspot.com/298310043/diff/100001/saml-bridge/Web.config#n... saml-bridge/Web.config:103: Maybe restore (in modified form) the note about replacing yourgsa1 and yourgsa2 with the actual GSA IP addresses or host names?
Sign in to reply to this message.
https://codereview.appspot.com/298310043/diff/100001/saml-bridge/App_Code/Glo... File saml-bridge/App_Code/Global.asax.cs (right): https://codereview.appspot.com/298310043/diff/100001/saml-bridge/App_Code/Glo... saml-bridge/App_Code/Global.asax.cs:85: foreach (String s in ConfigurationManager.AppSettings["assertion_consumer"].Split(',')) +1 to John. In case of null, assertion_consumer split function might throw exception. Also can it possible to assign assertion_consumer property to an object and use it in for loop?
Sign in to reply to this message.
Thank you, just uploaded a new version of the CL. https://codereview.appspot.com/298310043/diff/100001/RELEASE_NOTE.txt File RELEASE_NOTE.txt (right): https://codereview.appspot.com/298310043/diff/100001/RELEASE_NOTE.txt#newcode11 RELEASE_NOTE.txt:11: Release 4.1.1 December 8, 2016 On 2016/12/06 01:10:59, JohnL wrote: > I would generally put the release notes in a separate CL. OK, I will move it to a new one.. https://codereview.appspot.com/298310043/diff/100001/saml-bridge/App_Code/Glo... File saml-bridge/App_Code/Global.asax.cs (right): https://codereview.appspot.com/298310043/diff/100001/saml-bridge/App_Code/Glo... saml-bridge/App_Code/Global.asax.cs:85: foreach (String s in ConfigurationManager.AppSettings["assertion_consumer"].Split(',')) On 2016/12/09 18:27:55, lchandramouli wrote: > +1 to John. > > In case of null, assertion_consumer split function might throw exception. > Also can it possible to assign assertion_consumer property to an object and use > it in for loop? Done. https://codereview.appspot.com/298310043/diff/100001/saml-bridge/Web.config File saml-bridge/Web.config (right): https://codereview.appspot.com/298310043/diff/100001/saml-bridge/Web.config#n... saml-bridge/Web.config:98: <!-- Since v4.1.1 SAML Bridge takes this URL from the SAML Request's AssertionConsumerServiceURL On 2016/12/06 01:10:59, JohnL wrote: > This reads a little funny, and suggests that the default value is taken from the > SAML request. Maybe just s/takes this URL from/uses/? Done. https://codereview.appspot.com/298310043/diff/100001/saml-bridge/Web.config#n... saml-bridge/Web.config:103: On 2016/12/06 01:10:59, JohnL wrote: > Maybe restore (in modified form) the note about replacing yourgsa1 and yourgsa2 > with the actual GSA IP addresses or host names? Done.
Sign in to reply to this message.
LGTM Thanks for picking this up again. John L
Sign in to reply to this message.
One last nit. John L https://codereview.appspot.com/298310043/diff/120001/saml-bridge/Web.config File saml-bridge/Web.config (right): https://codereview.appspot.com/298310043/diff/120001/saml-bridge/Web.config#n... saml-bridge/Web.config:98: <!-- Since v4.1.1 SAML Bridge uses the SAML Request's AssertionConsumerServiceURL There's no need to point out the version number or the change here (that's what release notes are for). Let's just say: "To support multi-host GSA setup, this setting is a comma-separated list ..."
Sign in to reply to this message.
On 2017/03/22 20:03:49, JohnL wrote: > One last nit. > > John L > > https://codereview.appspot.com/298310043/diff/120001/saml-bridge/Web.config > File saml-bridge/Web.config (right): > > https://codereview.appspot.com/298310043/diff/120001/saml-bridge/Web.config#n... > saml-bridge/Web.config:98: <!-- Since v4.1.1 SAML Bridge uses the SAML Request's > AssertionConsumerServiceURL > There's no need to point out the version number or the change here (that's what > release notes are for). Let's just say: > > "To support multi-host GSA setup, this setting is a comma-separated list ..." Done
Sign in to reply to this message.
LGTM, with a followup nit. I don't think we're waiting for anyone else on this. John L https://codereview.appspot.com/298310043/diff/140001/saml-bridge/Web.config File saml-bridge/Web.config (right): https://codereview.appspot.com/298310043/diff/140001/saml-bridge/Web.config#n... saml-bridge/Web.config:98: <!-- To support multi-host GSA setup, this setting is now a comma separate list Since you've managed to delete the "d", I will mention that I really was suggesting deleting "now" and hyphenating "comma-separated".
Sign in to reply to this message.
Indeed.. Better ? :) On Wed, Mar 22, 2017 at 5:14 PM, <jlacey@google.com> wrote: > LGTM, with a followup nit. > > I don't think we're waiting for anyone else on this. > > John L > > > https://codereview.appspot.com/298310043/diff/140001/saml- > bridge/Web.config > File saml-bridge/Web.config (right): > > https://codereview.appspot.com/298310043/diff/140001/saml- > bridge/Web.config#newcode98 > saml-bridge/Web.config:98: <!-- To support multi-host GSA setup, this > setting is now a comma separate list > Since you've managed to delete the "d", I will mention that I really was > suggesting deleting "now" and hyphenating "comma-separated". > > https://codereview.appspot.com/298310043/ > > -- > >
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
