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

Manage api from Cachet Settings page (#235) #236

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

icecoldPHP
Copy link
Contributor

Add settings to enable and disable API
Add settings to force authentication for API
Closes: #235

Add settings to force authentication for API
@icecoldPHP icecoldPHP marked this pull request as draft February 9, 2025 07:00
@icecoldPHP icecoldPHP marked this pull request as ready for review February 9, 2025 07:09
Copy link
Member

@jbrooksuk jbrooksuk left a 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.

@icecoldPHP icecoldPHP marked this pull request as draft February 9, 2025 16:11
ApiEnabled & ApiPublicOrProtected which solves the early access to DB
Added GetOrDefault to AppSettings for ease of use and removal of duplicate code.
@icecoldPHP icecoldPHP requested a review from jbrooksuk February 11, 2025 16:06
@icecoldPHP icecoldPHP marked this pull request as ready for review February 11, 2025 16:07
Copy link
Member

@jbrooksuk jbrooksuk left a 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);
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

Copy link
Contributor Author

@icecoldPHP icecoldPHP Mar 7, 2025

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

@icecoldPHP icecoldPHP marked this pull request as draft February 13, 2025 08:55
@icecoldPHP icecoldPHP requested a review from jbrooksuk March 7, 2025 22:30
@icecoldPHP icecoldPHP marked this pull request as ready for review March 7, 2025 22:31
@icecoldPHP icecoldPHP changed the title Manage api from Cachet Settings page Manage api from Cachet Settings page (#235) Mar 8, 2025
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.

API GET Request with API Key
3 participants