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

Implement default jwt-secret paths #5979

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Zexuz
Copy link

@Zexuz Zexuz commented Aug 1, 2023

Resolves #5948

Changes

  • Remove default value for JsonRpc.JwtSecretFile
  • implement default jwt-secret paths

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

Testing this is hard since it's all IO methods, but since it's now a more complex logic flow, maybe its worth it?

Documentation

Requires documentation update

  • Yes
  • No

If yes, link the PR to the docs update or the issue with the details labeled docs. Remove if not applicable.

Requires explanation in Release Notes

  • Yes
  • No

If yes, fill in the details here. Remove if not applicable.

Remarks

Should we update the docs and/or the release notes with the new files paths?

@Zexuz
Copy link
Author

Zexuz commented Aug 2, 2023

@MarekM25, any feedback?

@LukaszRozmej LukaszRozmej requested review from rubo and deffrian August 3, 2023 08:24
@LukaszRozmej
Copy link
Member

@MarekM25, any feedback?

@MarekM25 is currently on leave, @rubo and @deffrian could you take a look?

Copy link
Member

@LukaszRozmej LukaszRozmej left a comment

Choose a reason for hiding this comment

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

Would be good to add some tests

string? homeDir = Environment.GetEnvironmentVariable("HOME");

if (string.IsNullOrEmpty(homeDir))
throw new Exception("HOME environment variable is not set");
Copy link
Member

Choose a reason for hiding this comment

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

We also need to find better exception type here

Copy link
Author

Choose a reason for hiding this comment

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

Create a new custom Exception? Maybe one of these?

  • NotFoundException
  • NotSetException
  • EnviormentVariableNotSet
  • EnviormentVariableNotFound

Copy link
Member

Choose a reason for hiding this comment

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

My candidates would be either InvalidOperationException or System.Configuration.ConfigurationException

@Zexuz
Copy link
Author

Zexuz commented Aug 3, 2023

Added some test, and broke out the the OS filePaths to a JwtSecretPathResolver to make testing easier. But the test cases for JwtAuthenticationTests are still huge. I don't see how to make them smaller since since it's all I/O stuff. Could refactor JwtAuthentication even more and break it up to two files, one for branch logic and on for the read/generate secrets. Since now the test cases need to setup the logic and test I/O.

@rubo
Copy link
Contributor

rubo commented Aug 3, 2023

Marking as draft for now until the spec finalizes. I'll back to this shortly.

@Zexuz Thanks for contributing.

@rubo rubo marked this pull request as draft August 3, 2023 19:00
@LukaszRozmej LukaszRozmej requested a review from MarekM25 August 9, 2023 12:47
@Zexuz
Copy link
Author

Zexuz commented Jan 11, 2024

Ping

@MarekM25 MarekM25 mentioned this pull request Nov 18, 2024
16 tasks
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.

Implement default jwt-secret paths
3 participants