Add support for _auth and email field in upm credential
#29
Conversation
|
Hey, I think "Basic Auth" should be a new option in the "Login & get auth token" window and not in this window. |
|
This would be an additional feature. With this PR the idea is that the tool only supports adding the For now I assumed that the user already has a basic auth token, however, adding the option to retrieve the token via UI would also be nice (although, "retrieving" the basic auth token just means base64 encoding username + password). |
|
Not sure I get your point here - if you already have a token that's what the "Token" field is for. Did you maybe mean to only add the email field? |
|
Sorry, I missed adding the link to the docs. I guess ideally this would be handled by having different implementations for |
|
This seems like a useful feature. Might want to make the UI have a dropdown to select between token and basic auth, so it is easier to document. I'm sorry for the delay in answering, crazy busy time here. |
|
When applying this locally, I found that NPMPublish.cs also needed updating to check/pull for _auth if token was null, and if that was the case to use the Basic auth keyword instead of Bearer in the authorization header. Otherwise publish would fail as it tries to push the package using a null token value and wrong header. The affected area to change in addition was here: |
| @@ -14,6 +14,8 @@ public class NPMCredential | |||
| { | |||
| public string url; | |||
| public string token; | |||
| public string _auth; | |||
There was a problem hiding this comment.
any particular reason why this field is formatted differently from the others in this class?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I wasn't aware this was reading/writing from that file. Sounds good to me :) Thanks for helping me understand.
| cred.token = (string)value["token"]; | ||
| cred.alwaysAuth = (bool)value["alwaysAuth"]; | ||
| if (value.TryGetValue(nameof(NPMCredential.token), out var token)) | ||
| cred.token = (string) token; |
There was a problem hiding this comment.
I'd be much more comfortable with braces around all these conditionals
|
|
||
| public NPMCredential Credential => new NPMCredential() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Unfortunately, I probably won't find the time to implement the suggested changes, as I am no longer using this tool. Sorry. |
Quick fix for #28
I also added support for the email field. Works for me now, however, there might still be some edge cases where it breaks or some of the validation in the UI does not work correctly.
The UI simply shows all available fields:
