Make API token optional for user api routes #159
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dev should probably override this setting, but keeping it as-is for now
costdev
reviewed
Feb 6, 2025
| $response = $this->getjson('/plugins/info/1.1?action=query_plugins'); | ||
| $response->assertStatus(200); | ||
| })->skip(fn() => config('app.aspirecloud.api_authentication_enable'), 'API authentication is enabled'); | ||
| // TODO: write tests for real and and bad auth tokens |
There was a problem hiding this comment.
Suggested change
| // TODO: write tests for real and and bad auth tokens | |
| // TODO: write tests for real and bad auth tokens |
Double "and". Possibly jittery from too much coffee 😉
There was a problem hiding this comment.
☕️☕️☕️ + 🌲🌲 = 😳
Anyway I already tested a bad token so I just need to generate and use a real one, and hopefully I'll have that knocked out before I merge this (it's already getting deployed to .io before then)
costdev
approved these changes
Feb 6, 2025
|
I decided to kick pint to the curb in CI, it's too slow to be worth it. If prettier can do PER-CS, I'll consider switching to it (I don't know about its php 8.4 support but we're not using any 8.4 syntax yet) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request
What changed?
Made the API token optional, but validated if present.
Removed the
app.aspirecloud.api_authentication_enableoption. It's always enabled and optional now, except for admin routes where it's always been mandatory.Unrelated change: sets the default error_reporting level to
E_ALL & ~E_DEPRECATED.Why did it change?
We should remove the speedbumps on adoption, plus it creates a privacy headache, being linked to registration email and all that. We can start throttling unauthenticated users if we need to.
We also don't need deprecation notices spamming up stderr on production.
Did you fix any specific issues?
Closes: #157
CERTIFICATION
By opening this pull request, I do agree to abide by the Code of Conduct and be bound by the terms of the Contribution Guidelines in effect on the date and time of my contribution as proven by the revision information in GitHub.