Skip to content
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

Add support for _auth and email field in upm credential #29

Closed
wants to merge 1 commit into from
Closed

Add support for _auth and email field in upm credential #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
52 changes: 33 additions & 19 deletions Editor/Halodi/PackageRegistry/Core/CredentialManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ public class NPMCredential
{
public string url;
public string token;
public string _auth;
Copy link
Contributor

Choose a reason for hiding this comment

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

any particular reason why this field is formatted differently from the others in this class?

Copy link
Author

Choose a reason for hiding this comment

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

All of the fields use the exact name that is used in the .upmconfig.toml file, which in this case is _auth (with the underscore). A nice side effect is that it is possible to use nameof(NPMCredential._auth) to reference the key.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't aware this was reading/writing from that file. Sounds good to me :) Thanks for helping me understand.

public string email;
public bool alwaysAuth;
}

Expand Down Expand Up @@ -68,8 +70,14 @@ public CredentialManager()
NPMCredential cred = new NPMCredential();
cred.url = registry.Key;
TomlTable value = (TomlTable)registry.Value;
cred.token = (string)value["token"];
cred.alwaysAuth = (bool)value["alwaysAuth"];
if (value.TryGetValue(nameof(NPMCredential.token), out var token))
cred.token = (string) token;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be much more comfortable with braces around all these conditionals

if (value.TryGetValue(nameof(NPMCredential._auth), out var _auth))
cred._auth = (string) _auth;
if (value.TryGetValue(nameof(NPMCredential.email), out var email))
cred.email = (string)email;
if (value.TryGetValue(nameof(NPMCredential.alwaysAuth), out var alwaysAuth))
cred.alwaysAuth = (bool)alwaysAuth;

credentials.Add(cred);
}
Expand All @@ -89,15 +97,17 @@ public void Write()
credential.token = "";
}

doc.Tables.Add(new TableSyntax(new KeySyntax("npmAuth", credential.url))
{
Items =
{
{"token", credential.token},
{"alwaysAuth", credential.alwaysAuth}
}
});
var table = new TableSyntax(new KeySyntax("npmAuth", credential.url));

if(!string.IsNullOrEmpty(credential.token))
table.Items.Add(nameof(NPMCredential.token), credential.token);
if(!string.IsNullOrEmpty(credential._auth))
table.Items.Add(nameof(NPMCredential._auth), credential._auth);
if(!string.IsNullOrEmpty(credential.email))
table.Items.Add(nameof(NPMCredential.email), credential.email);
table.Items.Add(nameof(NPMCredential.alwaysAuth), credential.alwaysAuth);

doc.Tables.Add(table);
}


Expand All @@ -114,21 +124,25 @@ public NPMCredential GetCredential(string url)
return credentials.FirstOrDefault(x => x.url?.Equals(url, StringComparison.Ordinal) ?? false);
}

public void SetCredential(string url, bool alwaysAuth, string token)
public void SetCredential(NPMCredential inputCred)
{
if (HasRegistry(url))
if (HasRegistry(inputCred.url))
{
var cred = GetCredential(url);
cred.url = url;
cred.alwaysAuth = alwaysAuth;
cred.token = token;
var cred = GetCredential(inputCred.url);
cred.url = inputCred.url;
cred.alwaysAuth = inputCred.alwaysAuth;
cred.token = inputCred.token;
cred.email = inputCred.email;
cred._auth = inputCred._auth;
}
else
{
NPMCredential newCred = new NPMCredential();
newCred.url = url;
newCred.alwaysAuth = alwaysAuth;
newCred.token = token;
newCred.url = inputCred.url;
newCred.alwaysAuth = inputCred.alwaysAuth;
newCred.token = inputCred.token;
newCred.email = inputCred.email;
newCred._auth = inputCred._auth;

credentials.Add(newCred);
}
Expand Down
6 changes: 4 additions & 2 deletions Editor/Halodi/PackageRegistry/Core/RegistryManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ private ScopedRegistry LoadRegistry(JObject Jregistry)
NPMCredential credential = credentialManager.GetCredential(registry.url);
registry.auth = credential.alwaysAuth;
registry.token = credential.token;
registry._auth = credential._auth;
registry.email = credential.email;
}

return registry;
Expand Down Expand Up @@ -129,9 +131,9 @@ public void Save(ScopedRegistry registry)

JToken manifestRegistry = GetOrCreateScopedRegistry(registry, manifestJSON);

if(!string.IsNullOrEmpty(registry.token))
if(registry.isValidCredential())
{
credentialManager.SetCredential(registry.url, registry.auth, registry.token);
credentialManager.SetCredential(registry.Credential);
}
else
{
Expand Down
13 changes: 12 additions & 1 deletion Editor/Halodi/PackageRegistry/Core/ScopedRegistry.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,18 @@ public class ScopedRegistry
public bool auth;

public string token;
public string _auth;
public string email;

public NPMCredential Credential => new NPMCredential()
Copy link
Contributor

@StephenHodgson StephenHodgson Apr 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This creates a new instance each time the property is accessed, it might be better to keep a non-serialized backing field that is initialized when the object is deserialized, or when one of the fields are updated. Although in reality none of the fields should be public, and only have property accessors to begin with, that would actually help when determining when the new credential should be updated.

It seems a bit redundant to have the Credential property when the fields are already exposed for this class.

Copy link
Author

Choose a reason for hiding this comment

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

It has been so long that I am not sure anymore why I added this one. Probably because I thought it was easier to bundle credentials in a class/struct and pass that instead of each filed separately (like in credentialManager.SetCredential). I guess I only used this property for convenience to quickly construct a NPMCredential with a values from the ScopedRegistry. I agree that it could be refactored to be less redundant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Really the feedback is more focused on encapsulation. I think providing the property is great, but maybe we might want to properly encapsulate the fields to prevent abusive manipulations of the underlying data.

{
url = url,
token = token,
_auth = _auth,
email = email,
alwaysAuth = auth
};

public override string ToString()
{
return JsonUtility.ToJson(this, true);
Expand All @@ -31,7 +42,7 @@ public bool isValidCredential()

if(auth)
{
if(string.IsNullOrEmpty(token))
if(string.IsNullOrEmpty(token) && string.IsNullOrEmpty(_auth))
{
return false;
}
Expand Down
12 changes: 8 additions & 4 deletions Editor/Halodi/PackageRegistry/UI/CredentialEditorView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ public void Edit(NPMCredential credential, CredentialManager credentialManager)
this.registry.url = credential.url;
this.registry.auth = credential.alwaysAuth;
this.registry.token = credential.token;
this.registry._auth = credential._auth;
this.registry.email = credential.email;

this.createNew = false;
this.initialized = true;
Expand Down Expand Up @@ -73,18 +75,20 @@ void OnGUI()

registry.auth = EditorGUILayout.Toggle("Always auth", registry.auth);
registry.token = EditorGUILayout.TextField("Token", registry.token);
registry._auth = EditorGUILayout.TextField("Basic Auth", registry._auth);
registry.email = EditorGUILayout.TextField("Email", registry.email);

EditorGUILayout.Space();

EditorGUI.BeginDisabledGroup(string.IsNullOrEmpty(registry.url));
tokenMethod = GetTokenView.CreateGUI(tokenMethod, registry);

if (!string.IsNullOrEmpty(registry.url) && string.IsNullOrEmpty(registry.token))
if (!string.IsNullOrEmpty(registry.url) && string.IsNullOrEmpty(registry.token) && string.IsNullOrEmpty(registry._auth))
{
EditorGUILayout.HelpBox("Select an authentication method and click on \"Get token\"", MessageType.Warning);
}

EditorGUI.BeginDisabledGroup(string.IsNullOrEmpty(registry.token));
EditorGUI.BeginDisabledGroup(string.IsNullOrEmpty(registry.token) && string.IsNullOrEmpty(registry._auth));

EditorGUILayout.Space();
EditorGUILayout.BeginVertical(GUILayout.ExpandHeight(true));
Expand Down Expand Up @@ -121,9 +125,9 @@ void OnGUI()

private void Save()
{
if (registry.isValidCredential() && !string.IsNullOrEmpty(registry.token))
if (registry.isValidCredential())
{
credentialManager.SetCredential(registry.url, registry.auth, registry.token);
credentialManager.SetCredential(registry.Credential);
credentialManager.Write();

// TODO figure out in which cases/Editor versions a restart is actually required,
Expand Down
2 changes: 1 addition & 1 deletion Editor/Halodi/PackageRegistry/UI/RegistryManagerView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ internal static ReorderableList GetRegistryList(RegistryManager registryManager)
{
var registry = registryList.list[index] as ScopedRegistry;
if (registry == null) return;
bool registryHasAuth = !string.IsNullOrEmpty(registry.token) && registry.isValidCredential();
bool registryHasAuth = registry.isValidCredential();

var rect2 = rect;
rect.width -= 60;
Expand Down
2 changes: 2 additions & 0 deletions Editor/Halodi/PackageRegistry/UI/ScopedRegistryEditorView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ void OnGUI()

registry.auth = EditorGUILayout.Toggle("Always auth", registry.auth);
registry.token = EditorGUILayout.TextField("Token", registry.token);
registry._auth = EditorGUILayout.TextField("Basic Auth", registry._auth);
registry.email = EditorGUILayout.TextField("Email", registry.email);

EditorGUILayout.Space();

Expand Down