Don't require app ID header in request for single-app deployment #6645
Conversation
…re is one and only one app if x-parse-application-id is provided on the header, it is required to match one of the app(s) same for clients always doing HTTP POST instead of proper HTTP calls _ApplicationId is no longer required on the post body if there is one and only one app if _ApplicationId is provided on the post body, it is required to match one of the app(s) probably also fixed a couple bugs of using req.body as JSON before parse it
Codecov Report
@@ Coverage Diff @@
## master #6645 +/- ##
==========================================
+ Coverage 93.85% 93.88% +0.02%
==========================================
Files 169 169
Lines 12206 12209 +3
==========================================
+ Hits 11456 11462 +6
+ Misses 750 747 -3
Continue to review full report at Codecov.
|
|
@sunshineo thanks for the PR. Could you please add some test cases? |
|
Is there any pending issues prevent merging this? Thanks |
|
Are we still collecting feedback from the community? |
|
I just resolved the merge conflicts. Hi @davimacedo @mman @acinader @TomWFox . Could anyone approve this? |
|
Could you please merge this ? @davimacedo @mman @acinader @TomWFox EDIT: If anyone need to bypass the header, this is working prety well // index.js
const app = express()
// Serve the Parse API on the /parse URL prefix
const mountPath = process.env.PARSE_MOUNT || '/parse'
app.use(`${mountPath}/functions`, (req, res, next) => {
if (!req.headers['x-parse-application-id']) {
req.headers['x-parse-application-id'] = config.appId
}
next()
}) |
|
This requires a thorough conceptual review. There has been some effort to remove the app ID in the past, but whether it is feasible or practical to remove it has not been evaluated conclusively. There is at least one current feature request which seem to require the app ID. Merging this PR seems like an uncoordinated step towards removing the app ID, just because it solves some unspecific problem. Therefore my suggestion is to look at the bigger picture before considering to make a step into a direction without examining the full path. |
|
This is not remove. This makes it optional if there is only one app. |
|
An optional requirement means it won't be available in certain scenarios instead of being mandatory as it currently is. I don't think it is clear yet what this PR implies, it removes a parameter that is otherwise mandatory throughout the platform. I suggest to change the first comment of this PR and use the new PR template, so we can further look into this PR. It is not clear what this PR does from the current comment:
Required but does not need to match, does not make sense to me. |
|
@mtrezza You do not understand the part because you did not read closely. It is the "_ApplicationId" not "ApplicationId" that you quoted. The dashboard always uses a POST instead of standard http methods to avoid preflight, so it need a "_ApplicationId" in the post body. This PR did not touch that |
|
Can you clarify what you mean by "standard http method / proper http calls"? Why is a POST request not a proper http request?
Would you mind clarifying your first comment in this PR and use the new PR template - maybe that helps. |
|
Dashboard use a special POST to retrieve information. It does not use GET because it want to avoid preflight. That special POST has an "_ApplicationId" in it. This PR did not touch that I'll switch to the new template when I find some time |
There was a problem hiding this comment.
"it does not need to match if there is only one app"
Not sure I fully understand the intention of this PR and the implications of the changes made. For example, "does not need to match" is unclear to me, does that mean "does not have to be provided"? Using the PR template with proper description of what should be achieved and how it is achieved could help.
There is also unhandled error catching, which looks suspicious.
mtrezza
changed the title
|
Danger run resulted in 1 warning; to find out more, see the checks page. Generated by 🚫 dangerJS |
|
if x-parse-application-id is provided on the header, it is required to match one of the app(s)
for clients always doing HTTP POST instead of proper HTTP calls, _ApplicationId is still required on the post body, it does not need to match if there is only one app, it is required to match one of the apps if there are multiple apps
probably also fixed a couple bugs of using req.body as JSON before parse it
#6590