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

feat: Replace Basic Authentication with JWT Tokens, Added Login Page #2995

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

Conversation

Hazer
Copy link
Member

@Hazer Hazer commented Aug 9, 2024

Description

Overtaking of #2252, as I can't force push my rebased changes there.

  • Fix conflicts
  • Apply theme options
  • Handle Login redirects, instead of always returning to path /
  • Improve autocomplete
  • Move auth logic to a class(?) this protocol will allow for alternative auth implementations, and testing
  • Avoid logging out after applying settings (how to do it safely?)

Screenshot

Original at: #2252
TODO

Issues Fixed or Closed

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Dependency update (updates to dependencies)
  • Documentation update (changes to documentation)
  • Repository update (changes to repository files, e.g. .github/...)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

newPath = newPath + redirect;
}
}
document.location.href = newPath;

Check warning

Code scanning / CodeQL

Client-side URL redirect Medium

Untrusted URL redirection depends on a
user-provided value
.
Copy link
Member Author

Choose a reason for hiding this comment

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

This value comes from the backend, usually, but in case someone tried to lure a user, just above it there's some code to cleanup the redirect value before using it.

@Hazer
Copy link
Member Author

Hazer commented Aug 9, 2024

FYI @ReenigneArcher @TheElixZammuto @cgutman let's continue discussion here.

@TheElixZammuto I invited you to my fork, so you can push something yourself if needed. This way we can iterate fast and I can get your amazing work merged.

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

Attention: Patch coverage is 0.97087% with 102 lines in your changes missing coverage. Please review.

Project coverage is 9.52%. Comparing base (0e153cf) to head (07967ec).

Files Patch % Lines
src/confighttp.cpp 0.97% 42 Missing and 60 partials ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           master   #2995      +/-   ##
=========================================
- Coverage    9.57%   9.52%   -0.06%     
=========================================
  Files          73      73              
  Lines       13616   13703      +87     
  Branches     6263    6332      +69     
=========================================
+ Hits         1304    1305       +1     
- Misses       9737    9770      +33     
- Partials     2575    2628      +53     
Flag Coverage Δ
Windows 5.07% <0.00%> (-0.04%) ⬇️
macOS-12 10.27% <1.06%> (-0.09%) ⬇️
macOS-13 10.18% <0.00%> (-0.09%) ⬇️
macOS-14 10.48% <0.00%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/confighttp.cpp 1.16% <0.97%> (-0.01%) ⬇️

@ns6089
Copy link
Contributor

ns6089 commented Aug 9, 2024

Please correct me if I'm wrong, but won't this require new login prompt each time sunshine settings are "applied"?

@Hazer
Copy link
Member Author

Hazer commented Aug 9, 2024

Please correct me if I'm wrong, but won't this require new login prompt each time sunshine settings are "applied"?

@ns6089 You're completely right, just tested it myself and had to login again.

This value comes from the backend, but in case someone may try to lure some user somehow, we add a layer of protection
@ns6089
Copy link
Contributor

ns6089 commented Aug 9, 2024

So on the pro side we have better UI and possibly better compatibility with browser password managers.
On the con, more complicated api and this minor annoyance with repeating login prompt.

@TheElixZammuto (or anyone who knows), you mentioned "it's more compatible with password managers" in the original PR. Did you run into problems with basic auth in a particular browser?

@Hazer Hazer force-pushed the hazer/feat/new-session-system branch from 18145e7 to 448d83c Compare August 9, 2024 08:54
@Hazer
Copy link
Member Author

Hazer commented Aug 9, 2024

@ns6089 that apply thing can be fixed. What do you mean by more complicated API? JWT Auth is kinda almost the standard nowadays, and we can further provide customization in the future like due date expiring sessions, or maybe allow for custom providers. What will you miss from Basic Auth that is useful to you right now?

Basic Auth won't work with autocomplete in many browser's password manager APIs, or works but won't allow third-party password managers to fill in, only the built-in browser one, that many people don't use. It is an annoyance I want to fix too.

@ns6089
Copy link
Contributor

ns6089 commented Aug 9, 2024

@Hazer I don't personally use web api for anything, just wanted list all possible pros and cons. My only concern that we may be fixing the problem for 1% of users (custom password managers) by introducing a new problem to 99% of users (that recurring login prompt). If it can be fixed, I'm all for it.

@ns6089
Copy link
Contributor

ns6089 commented Aug 10, 2024

Also a random thought.
Maybe we should consider dropping off username from the authentication scheme leaving only the password.
It was required as part of http basic auth, but JWT gives us more freedom.

@TheElixZammuto
Copy link
Contributor

So on the pro side we have better UI and possibly better compatibility with browser password managers. On the con, more complicated api and this minor annoyance with repeating login prompt.

@TheElixZammuto (or anyone who knows), you mentioned "it's more compatible with password managers" in the original PR. Did you run into problems with basic auth in a particular browser?

Not personally since I don't use my password manager for Sunshine, but this feature was asked over Discord and GitHub a couple of times

@TheElixZammuto
Copy link
Contributor

As for keeping the auth safely, the only way is to store the jwt_key in the state file instead of just in memory, in a default config only admin users should be able to access it. The problem lies in the fact that the we cannot revoke a session when a session gets compromised. Maybe we can add an action to reset the key in the troubleshooting section?

@TheElixZammuto
Copy link
Contributor

Also a random thought. Maybe we should consider dropping off username from the authentication scheme leaving only the password. It was required as part of http basic auth, but JWT gives us more freedom.

I would prefer to keep it. Users are already aware of the combo and they are used to that, also we can use the username in the future if we want to add multiple users/multiple access levels

@ns6089
Copy link
Contributor

ns6089 commented Aug 10, 2024

As for keeping the auth safely, the only way is to store the jwt_key in the state file instead of just in memory, in a default config only admin users should be able to access it. The problem lies in the fact that the we cannot revoke a session when a session gets compromised. Maybe we can add an action to reset the key in the troubleshooting section?

Can't we just use symmetrical keys so leaking this key doesn't compromise security without knowing client's secret?

@ns6089
Copy link
Contributor

ns6089 commented Aug 10, 2024

Sorry, assymetrical.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@Nonary

This comment was marked as off-topic.

@ReenigneArcher

This comment was marked as off-topic.

@Nonary

This comment was marked as off-topic.

@ClassicOldSong

This comment was marked as off-topic.

@Nonary

This comment was marked as off-topic.

@Nonary

This comment was marked as off-topic.

@ClassicOldSong

This comment was marked as off-topic.

@Nonary

This comment was marked as off-topic.

@ClassicOldSong

This comment was marked as off-topic.

@Nonary

This comment was marked as off-topic.

@ClassicOldSong

This comment was marked as off-topic.

@Nonary

This comment was marked as off-topic.

@ClassicOldSong

This comment was marked as off-topic.

@Nonary

This comment was marked as off-topic.

@ReenigneArcher
Copy link
Member

Please move this noise to discord or somewhere else. This space is for code reviews.

@Nonary

This comment was marked as off-topic.

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.

6 participants