-
-
Notifications
You must be signed in to change notification settings - Fork 55
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
Manage api from Cachet Settings page (#235) #236
base: main
Are you sure you want to change the base?
Manage api from Cachet Settings page (#235) #236
Conversation
Add settings to force authentication for API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks pretty good to me. We need some tests to validate that the API can be enabled and disabled + that GET
requests can be protected.
ApiEnabled & ApiPublicOrProtected which solves the early access to DB Added GetOrDefault to AppSettings for ease of use and removal of duplicate code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of things we need to fix here.
{ | ||
$protected = AppSettings::getOrDefault('api_protected', false); | ||
if ($protected) { | ||
return parent::handle($request, $next, ...$guards); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is trying to redirect to a login
route which doesn't exist in Cachet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jbrooksuk This is something that is managed by Sanctum.
We could override this because this is currently a global issue.
So do you want me to override the sanctum routes within the Core?
or do you want me to fix this in the Cachet repo?
Or both
Because on my instance i changed this on the cachet repo by hand (sanctum config).
So it redirect to the correct login page
Fix add static types to method
Add settings to enable and disable API
Add settings to force authentication for API
Closes: #235