Skip to content

Psresourcerepository file bug #661

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
May 26, 2022
Merged

Psresourcerepository file bug #661

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
840f53a
fix Uri/Url not being null checked for case where reading all repo's
anamnavi May 26, 2022
77f5b52
use Url as default name of attribute when read/remove/update/adding, …
anamnavi May 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
181 changes: 159 additions & 22 deletions src/code/RepositorySettings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public static PSRepositoryInfo Add(string repoName, Uri repoUri, int repoPriorit
XElement newElement = new XElement(
"Repository",
new XAttribute("Name", repoName),
new XAttribute("Uri", repoUri),
new XAttribute("Url", repoUri),
new XAttribute("Priority", repoPriority),
new XAttribute("Trusted", repoTrusted)
);
Expand Down Expand Up @@ -139,17 +139,60 @@ public static PSRepositoryInfo Update(string repoName, Uri repoUri, int repoPrio
throw new ArgumentException("Cannot find the repository because it does not exist. Try registering the repository using 'Register-PSResourceRepository'");
}

// Check that repository node we are attempting to update has all required attributes: Name, Url (or Uri), Priority, Trusted.
// Name attribute is already checked for in FindRepositoryElement()

if (node.Attribute("Priority") == null)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can simplify this code by first assigning all expected xml attributes to local variables, and not perform look ups multiple times for each attribute. Then we can check the local variables for null values.

{
throw new ArgumentException("Repository element does not contain neccessary 'Priority' attribute, in file located at path: {0}. Fix this in your file and run again.", FullRepositoryPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have a common message for any missing repository values that are required. This should be very unlikely and indicates the repository file got corrupted. I feel the only sensible thing to do in this case is to drop the corrupted entry and report the error to the user. We will need to re-write the repository with good data, and it is possible that the entire file is non-readable, in which case we should revert to the default state whatever it is (empty, or with PSGallery?).

Message suggestion, something like:

'The repository entry for [RepositoryName if available] is unreadable and will be dropped from the repository list.'

}

if (node.Attribute("Trusted") == null)
Copy link
Member

Choose a reason for hiding this comment

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

I think we should also add this null check for 'name' as well just in case we change something in the future or a user modifies their own xml

{
throw new ArgumentException("Repository element does not contain neccessary 'Trusted' attribute, in file located at path: {0}. Fix this in your file and run again.", FullRepositoryPath);
}

bool urlAttributeExists = node.Attribute("Url") != null;
bool uriAttributeExists = node.Attribute("Uri") != null;
Copy link
Member

Choose a reason for hiding this comment

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

I think we can get rid of the 'Uri' checks and just check for 'Url'. We can just let users know if they encounter the null object error that they can reset their xml (presumably this is a very small number of people anyway)

if (!urlAttributeExists && !uriAttributeExists)
{
throw new ArgumentException("Repository element does not contain neccessary 'Url' attribute (or alternatively 'Uri' attribute), in file located at path: {0}. Fix this in your file and run again.", FullRepositoryPath);
}

// Else, keep going
// Get root of XDocument (XElement)
var root = doc.Root;

// A null Uri value passed in signifies the Uri was not attempted to be set.
// A null Uri (or Url) value passed in signifies the Uri was not attempted to be set.
// So only set Uri attribute if non-null value passed in for repoUri

// determine if existing repository node (which we wish to update) had Url to Uri attribute
Uri thisUrl = null;
if (repoUri != null)
{
node.Attribute("Uri").Value = repoUri.AbsoluteUri;
if (urlAttributeExists)
{
node.Attribute("Url").Value = repoUri.AbsoluteUri;
// Create Uri from node Uri attribute to create PSRepositoryInfo item to return.
if (!Uri.TryCreate(node.Attribute("Url").Value, UriKind.Absolute, out thisUrl))
{
throw new PSInvalidOperationException(String.Format("Unable to read incorrectly formatted Url for repo {0}", repoName));
}
}
else
{
// Uri attribute exists
node.Attribute("Uri").Value = repoUri.AbsoluteUri;
if (!Uri.TryCreate(node.Attribute("Uri").Value, UriKind.Absolute, out thisUrl))
{
throw new PSInvalidOperationException(String.Format("Unable to read incorrectly formatted Uri for repo {0}", repoName));
}
}
}




// A negative Priority value passed in signifies the Priority value was not attempted to be set.
// So only set Priority attribute if non-null value passed in for repoPriority
if (repoPriority >= 0)
Expand Down Expand Up @@ -187,12 +230,6 @@ public static PSRepositoryInfo Update(string repoName, Uri repoUri, int repoPrio
}
}

// Create Uri from node Uri attribute to create PSRepositoryInfo item to return.
if (!Uri.TryCreate(node.Attribute("Uri").Value, UriKind.Absolute, out Uri thisUri))
{
throw new PSInvalidOperationException(String.Format("Unable to read incorrectly formatted Uri for repo {0}", repoName));
}

// Create CredentialInfo based on new values or whether it was empty to begin with
PSCredentialInfo thisCredentialInfo = null;
if (node.Attribute(PSCredentialInfo.VaultNameAttribute)?.Value != null &&
Expand All @@ -204,7 +241,7 @@ public static PSRepositoryInfo Update(string repoName, Uri repoUri, int repoPrio
}

updatedRepo = new PSRepositoryInfo(repoName,
thisUri,
thisUrl,
Int32.Parse(node.Attribute("Priority").Value),
Boolean.Parse(node.Attribute("Trusted").Value),
thisCredentialInfo);
Expand Down Expand Up @@ -257,9 +294,32 @@ public static List<PSRepositoryInfo> Remove(string[] repoNames, out string[] err
{
repoCredentialInfo = new PSCredentialInfo(node.Attribute("VaultName").Value, node.Attribute("SecretName").Value);
}

if (node.Attribute("Priority") == null)
Copy link
Member

Choose a reason for hiding this comment

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

Same comments here as above

{
tempErrorList.Add(String.Format("Repository element does not contain neccessary 'Priority' attribute, in file located at path: {0}. Fix this in your file and run again.", FullRepositoryPath));
continue;
}

if (node.Attribute("Trusted") == null)
{
tempErrorList.Add(String.Format("Repository element does not contain neccessary 'Trusted' attribute, in file located at path: {0}. Fix this in your file and run again.", FullRepositoryPath));
continue;
}

// determine if repo had Url or Uri (less likely) attribute
bool urlAttributeExists = node.Attribute("Url") != null;
bool uriAttributeExists = node.Attribute("Uri") != null;
if (!urlAttributeExists && !uriAttributeExists)
{
tempErrorList.Add(String.Format("Repository element does not contain neccessary 'Url' or equivalent 'Uri' attribute (it must contain one per Repository), in file located at path: {0}. Fix this in your file and run again.", FullRepositoryPath));
continue;
}

string attributeUrlUriName = urlAttributeExists ? "Url" : "Uri";
removedRepos.Add(
new PSRepositoryInfo(repo,
new Uri(node.Attribute("Uri").Value),
new Uri(node.Attribute(attributeUrlUriName).Value),
Int32.Parse(node.Attribute("Priority").Value),
Boolean.Parse(node.Attribute("Trusted").Value),
repoCredentialInfo));
Expand Down Expand Up @@ -290,17 +350,58 @@ public static List<PSRepositoryInfo> Read(string[] repoNames, out string[] error
throw new PSInvalidOperationException(String.Format("Loading repository store failed: {0}", e.Message));
}

if (repoNames == null || !repoNames.Any() || string.Equals(repoNames[0], "*") || repoNames[0] == null)
if (repoNames == null || repoNames.Length == 0 || string.Equals(repoNames[0], "*") || repoNames[0] == null)
{
// Name array or single value is null so we will list all repositories registered
// iterate through the doc
foreach (XElement repo in doc.Descendants("Repository"))
{
if (!Uri.TryCreate(repo.Attribute("Uri").Value, UriKind.Absolute, out Uri thisUri))
if (repo.Attribute("Name") == null)
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be a utility method that 'harvests' all expected repository attributes into a data structure, and also validates the required attributes. The returned repository data structure can then be returned and assumed to reflect a valid repository entry.

{
tempErrorList.Add(String.Format("Repository element does not contain neccessary 'Name' attribute, in file located at path: {0}. Fix this in your file and run again.", FullRepositoryPath));
continue;
}

if (repo.Attribute("Priority") == null)
{
tempErrorList.Add(String.Format("Unable to read incorrectly formatted Uri for repo {0}", repo.Attribute("Name").Value));
tempErrorList.Add(String.Format("Repository element does not contain neccessary 'Priority' attribute, in file located at path: {0}. Fix this in your file and run again.", FullRepositoryPath));
continue;
}

if (repo.Attribute("Trusted") == null)
{
tempErrorList.Add(String.Format("Repository element does not contain neccessary 'Trusted' attribute, in file located at path: {0}. Fix this in your file and run again.", FullRepositoryPath));
continue;
}

bool urlAttributeExists = repo.Attribute("Url") != null;
bool uriAttributeExists = repo.Attribute("Uri") != null;
// case: neither Url nor Uri attributes exist
if (!urlAttributeExists && !uriAttributeExists)
{
tempErrorList.Add(String.Format("Repository element does not contain neccessary 'Url' or equivalent 'Uri' attribute (it must contain one), in file located at path: {0}. Fix this in your file and run again.", FullRepositoryPath));
continue;
}

Uri thisUrl = null;
// case: either attribute Uri or Url exists
// TODO: do we only allow both to exist, across repositories? (i.e if a file has Uri attribute for one repo and Url attribute for another --> is that invalid?)
if (urlAttributeExists)
{
if (!Uri.TryCreate(repo.Attribute("Url").Value, UriKind.Absolute, out thisUrl))
{
tempErrorList.Add(String.Format("Unable to read incorrectly formatted Url for repo {0}", repo.Attribute("Name").Value));
continue;
}
}
else if (uriAttributeExists)
{
if (!Uri.TryCreate(repo.Attribute("Uri").Value, UriKind.Absolute, out thisUrl))
{
tempErrorList.Add(String.Format("Unable to read incorrectly formatted Uri for repo {0}", repo.Attribute("Name").Value));
continue;
}
}

PSCredentialInfo thisCredentialInfo;
string credentialInfoErrorMessage = $"Repository {repo.Attribute("Name").Value} has invalid CredentialInfo. {PSCredentialInfo.VaultNameAttribute} and {PSCredentialInfo.SecretNameAttribute} should both be present and non-empty";
Expand Down Expand Up @@ -335,7 +436,7 @@ public static List<PSRepositoryInfo> Read(string[] repoNames, out string[] error
}

PSRepositoryInfo currentRepoItem = new PSRepositoryInfo(repo.Attribute("Name").Value,
thisUri,
thisUrl,
Int32.Parse(repo.Attribute("Priority").Value),
Boolean.Parse(repo.Attribute("Trusted").Value),
thisCredentialInfo);
Expand All @@ -350,15 +451,50 @@ public static List<PSRepositoryInfo> Read(string[] repoNames, out string[] error
bool repoMatch = false;
WildcardPattern nameWildCardPattern = new WildcardPattern(repo, WildcardOptions.IgnoreCase);

foreach (var node in doc.Descendants("Repository").Where(e => nameWildCardPattern.IsMatch(e.Attribute("Name").Value)))
foreach (var node in doc.Descendants("Repository").Where(e => e.Attribute("Name") != null && nameWildCardPattern.IsMatch(e.Attribute("Name").Value)))
{
repoMatch = true;
if (!Uri.TryCreate(node.Attribute("Uri").Value, UriKind.Absolute, out Uri thisUri))
if (node.Attribute("Priority") == null)
{
//debug statement
tempErrorList.Add(String.Format("Unable to read incorrectly formatted Uri for repo {0}", node.Attribute("Name").Value));
tempErrorList.Add(String.Format("Repository element does not contain neccessary 'Priority' attribute, in file located at path: {0}. Fix this in your file and run again.", FullRepositoryPath));
continue;
}

if (node.Attribute("Trusted") == null)
{
tempErrorList.Add(String.Format("Repository element does not contain neccessary 'Trusted' attribute, in file located at path: {0}. Fix this in your file and run again.", FullRepositoryPath));
continue;
}

repoMatch = true;
bool urlAttributeExists = node.Attribute("Url") != null;
bool uriAttributeExists = node.Attribute("Uri") != null;

// case: neither Url nor Uri attributes exist
if (!urlAttributeExists && !uriAttributeExists)
{
tempErrorList.Add(String.Format("Repository element does not contain neccessary 'Url' or equivalent 'Uri' attribute (it must contain one), in file located at path: {0}. Fix this in your file and run again.", FullRepositoryPath));
continue;
}

Uri thisUrl = null;
// case: either attribute Uri or Url exists
// TODO: do we only allow both to exist, across repositories? (i.e if a file has Uri attribute for one repo and Url attribute for another --> is that invalid?)
if (urlAttributeExists)
{
if (!Uri.TryCreate(node.Attribute("Url").Value, UriKind.Absolute, out thisUrl))
{
tempErrorList.Add(String.Format("Unable to read incorrectly formatted Url for repo {0}", node.Attribute("Name").Value));
continue;
}
}
else if (uriAttributeExists)
{
if (!Uri.TryCreate(node.Attribute("Uri").Value, UriKind.Absolute, out thisUrl))
{
tempErrorList.Add(String.Format("Unable to read incorrectly formatted Uri for repo {0}", node.Attribute("Name").Value));
continue;
}
}

PSCredentialInfo thisCredentialInfo;
string credentialInfoErrorMessage = $"Repository {node.Attribute("Name").Value} has invalid CredentialInfo. {PSCredentialInfo.VaultNameAttribute} and {PSCredentialInfo.SecretNameAttribute} should both be present and non-empty";
Expand Down Expand Up @@ -393,7 +529,7 @@ public static List<PSRepositoryInfo> Read(string[] repoNames, out string[] error
}

PSRepositoryInfo currentRepoItem = new PSRepositoryInfo(node.Attribute("Name").Value,
thisUri,
thisUrl,
Int32.Parse(node.Attribute("Priority").Value),
Boolean.Parse(node.Attribute("Trusted").Value),
thisCredentialInfo);
Expand Down Expand Up @@ -421,7 +557,8 @@ public static List<PSRepositoryInfo> Read(string[] repoNames, out string[] error
private static XElement FindRepositoryElement(XDocument doc, string name)
{
return doc.Descendants("Repository").Where(
e => string.Equals(
e => e.Attribute("Name") != null &&
string.Equals(
e.Attribute("Name").Value,
name,
StringComparison.InvariantCultureIgnoreCase)).FirstOrDefault();
Expand Down