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.

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

Ability to set user credentials from strings(s) #885

Merged
merged 9 commits into from
Apr 9, 2024
Merged

Ability to set user credentials from strings(s) #885

merged 9 commits into from
Apr 9, 2024

Conversation

scottf
Copy link
Collaborator

@scottf scottf commented Apr 8, 2024

No description provided.

@scottf scottf requested a review from mtmk April 8, 2024 21:58
Copy link
Contributor

@mtmk mtmk left a comment

Choose a reason for hiding this comment

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

LGTM

protected static string _loadUser(string text)
{
StringReader reader = null;
try
Copy link
Contributor

Choose a reason for hiding this comment

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

we could use using here but I see it's lifted from the original file, so better to leave as is.

Copy link
Collaborator

@sixlettervariables sixlettervariables left a comment

Choose a reason for hiding this comment

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

Minor nits; also double checked we wouldn't introduce any issues with the new base class.

Comment on lines 55 to 64
string text = secure.ToString();

// if it's a nk file, it only has the nkey
if (text.StartsWith("SU"))
{
return Nkeys.FromSeed(text);
}

// otherwise assume it's a creds file.
reader = new StringReader(text);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why the original code had the ToString

Suggested change
string text = secure.ToString();
// if it's a nk file, it only has the nkey
if (text.StartsWith("SU"))
{
return Nkeys.FromSeed(text);
}
// otherwise assume it's a creds file.
reader = new StringReader(text);
// if it's a nk file, it only has the nkey
if (secure.StartsWith("SU"))
{
return Nkeys.FromSeed(secure);
}
// otherwise assume it's a creds file.
reader = new StringReader(secure);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

leftover from when I was trying secure. Will fix.

@@ -22,7 +23,7 @@ namespace NATS.Client
/// not normally used directly, but is provided to extend or use for
/// utility methods to read a private seed or user JWT.
/// </summary>
public class DefaultUserJWTHandler
public class DefaultUserJWTHandler : BaseUserJWTHandler
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we had a breaking change earlier I went and double checked this would not introduce any issues, and I don't believe it will:

❓ REQUIRES JUDGMENT: Introducing a new base class

A type can be introduced into a hierarchy between two existing types if it doesn't introduce any new abstract members or change the semantics or behavior of existing types. For example, in .NET Framework 2.0, the DbConnection class became a new base class for SqlConnection, which had previously derived directly from Component.

https://learn.microsoft.com/en-us/dotnet/core/compatibility/library-change-rules#modifications-to-the-public-contract

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can change from base to a delegate

}
return user.ToString();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return user.ToString();
return user;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, that was left over from trying secure.

}
finally
{
Nkeys.Wipe(line);
Nkeys.Wipe(text);
Nkeys.Wipe(seed);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI, these were removed because the wipe tries to wipe a string which is not possible. The wipe method was recently changed to a no-op.

@scottf scottf merged commit 58c66f1 into main Apr 9, 2024
1 check passed
@scottf scottf deleted the issue-882 branch April 9, 2024 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants