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

Remove X-Parse-Application-Id header #6590

Closed
sunshineo opened this issue Apr 9, 2020 · 43 comments
Closed

Remove X-Parse-Application-Id header #6590

sunshineo opened this issue Apr 9, 2020 · 43 comments
Labels
type:feature New feature or improvement of existing feature

Comments

@sunshineo
Copy link
Contributor

NOTE: I'm willing to do the work and create a pull request if and after I get an OK from the maintainers

We should remove X-Parse-Application-Id header for the following reasons:

@stevestencil
Copy link
Contributor

You can accomplish what you're trying to do by just opening up an endpoint with express. Something like:


const api = new ParseServer({
// All of your server settings
});

const app = express();
app.use('/parse', api);

// Setup a POST endpoint for your web hook
app.post('/incoming_webhook', (req, res) => {
// Do whatever you want here without the X-Parse-Application-Id
});

@sunshineo
Copy link
Contributor Author

sunshineo commented Apr 10, 2020

@stevestencil Yes. This is the hack I'm talking about. You are basically writing an endpoint using nodejs express. Then this has nothing to do with Parse. It's wasting the Parse cloud function and the REST API functions

@acinader
Copy link
Contributor

acinader commented Apr 10, 2020

@sunshineo I don’t see an issue with your proposal.

@TomWFox as resident parliamentarian, what process for a change like this?

@dplewis
Copy link
Member

dplewis commented Apr 10, 2020

One concern I have is with the Files router that requires appId

/files/:appId/:filename

@sunshineo
Copy link
Contributor Author

@dplewis is there a reason that we cannot remove it?

@mman
Copy link
Contributor

mman commented Apr 10, 2020

I'm on parse for some good 8 years now and never had a use for it either, as @sunshineo says, it only complicates things, so +1 of my non-voting rights.

@mtrezza
Copy link
Member

mtrezza commented Apr 14, 2020

@dplewis Good point. Changing the URL of files can be a huge issue for CDN. Suddenly all CDN PoPs would refetch the files from the origin, causing a spike in traffic and costs.

Therefore, removing the app ID is not without consequences and potentially has a financial impact in some architectures.

@davimacedo
Copy link
Member

I think it is important we first clarify the idea. Are we removing all keys or only the app id? I'm not sure if removing all of them is a good idea in terms of security (even knowing it is relatively easy for someone to find the app id by reverse engineering the app code, it is still one more security layer). If we are not removing all keys, I don't see how only removing the app id will make things easier in the use cases that were mentioned in the thread. Another point to consider regarding this change is the effort it will take. Note that we are not only talking about changing the Parse Server but also all SDKs and docs.

@mtrezza
Copy link
Member

mtrezza commented Apr 14, 2020

This looks like a XY problem.
I think making the app ID optional in the server config would satisfy the actual issue.

@sunshineo
Copy link
Contributor Author

@davimacedo What do you mean by 'all keys'? Like x-parse-master-key ? I don't think we can remove those. I just want to remove x-parse-application-id. And @mtrezza , I prefer remove. My gut feeling is that make it optional will actually make things even more complicated

@mtrezza
Copy link
Member

mtrezza commented Apr 15, 2020

Why do you prefer remove? Again, I think this discussion is framed the wrong way.
The solution is already in the title, but what is the actual issue that should be solved?
Let's properly state the issue first, then evaluate options, then conclude, not the other way around.

@sunshineo
Copy link
Contributor Author

sunshineo commented Apr 15, 2020

@mtrezza In the post I listed 4 bullet points for 'what is the actual issue that should be solved' with links to other issues. I'm open to discuss each individually, and totally admit there are workarounds for some of them. But I aggregated them to show that x-parse-application-id is causing all sort of problems.

I prefer remove because my gut feeling is that make it optional will actually make things even more complicated. From this bug #6589 (comment) you can see that some code trying to get this id had a bug in it. Make it optional (again imho) is just more surface for bugs

@mtrezza
Copy link
Member

mtrezza commented Apr 15, 2020

In the post I listed 4 bullet points for 'what is the actual issue that should be solved'

It is providing no value.
It is a hassle ... to setup this header.
... /parse/health does not require that header but it does not include database status

I can't read any issues in your points, except for the one that you use a 3rd party service that does not allow you to send headers.

is there a reason that we cannot remove it?

You keep pushing for a solution parti pris without listening to the arguments here.

I prefer remove because my gut feeling is that make it optional will actually make things even more complicated.

I don't even know what that means. I stated above that there is potential financial impact for CDN when removing the app ID and your counter argument is your "gut feeling"?

I suggest you restate the issue properly and let's go from there.

@sunshineo
Copy link
Contributor Author

@mtrezza I sense a lot of hostility from you and I totally don't understand where that comes from. Why did you try to paint me as an evil asshole that is trying to push some secret agenda? All I said was I feel remove is better than make it optional from engineering perspective. And I provided a solid example of an existing bug that was cause by the complication to support this header. Please let's focus on discuss the problem.

I understand the impact for CDN and correct me if I'm wrong, the app id in the url did not provide any value. It is actually another example of it causing problems. You cannot change your app id freely because it will cost your CDN to refresh.

@mtrezza what if we have app id as 'optional' as you suggested but only on the file url? That would be backward compatible.

@mtrezza
Copy link
Member

mtrezza commented Apr 15, 2020

This is not the place for personal sensitivities. You want to remove a legacy parameter which requires a discussion where you have to explain your points. Each solution has implications and they have to be weighted against each other.

  1. Define issue
  2. Identify solutions
  3. Weight implications
  4. Agree on conclusion

It is actually another example of it causing problems. You cannot change your app id freely because it will cost your CDN to refresh.

That's an implication, not an issue. The app ID was never intended to be changed freely.

Issue:

  • Webhooks cannot be called by 3rd party tool that doesn't allow to set request headers.

Solution A: Remove app ID

  • con:
    • issue for CDN when file URL changes -> may be not an issue at least for parse S3 adapter which has a baseUrl parameter; do all parse file adapters use that parameter?
    • issue for other scenarios where the URL is cached (e.g. on client) as one did not expect the app ID to ever change
    • one (weak) security layer less
    • change code in parse server, dashboard, all parse SDKs -> are the changes necessary to communicate with parse server or can the changes be made at a later time?
    • ? (what else is app ID used for -> needs code investigation)
  • pro:
    • removing legacy code reduces maintenance effort
    • app ID makes 30-50% of file URL, removing app ID from file URL reduces data traffic cost

Solution B: Make app ID optional
Solution C: Change webhooks endpoint to accept app ID in body / url parameter
Solution D: ?

@mtrezza
Copy link
Member

mtrezza commented Apr 15, 2020

A quick look into the parse repos shows that app ID is heavily used.

Parse Dashboard:

  • App ID is used to identify apps internally, e.g. to save dashboard preferences per app
  • App ID is set in the local dashboard config, which becomes obsolete and confusing once there is no app ID anymore -> could be replaced with internal ID

Parse SDKs

  • App ID is set with SDK initialization, which becomes obsolete
  • App ID is part of local storage path -> probably requires a one-time migration to remove the app ID from storage. Why is the app ID part of the path anyway? Is there a scenario in which the local storage holds data for multiple app IDs?

For all repos:

  • Test cases have to be adapted that involve app ID.

General considerations:

  • Is there any functional relation between locally set app IDs in dashboard, SKDs and Parse Server, or are they only used for local functionality, once app ID is removed from parse server?
  • App ID is currently the only ID that identifies an app across all repos. That may not be relevant with the current feature set, but it could be an essential property for future features. It could already be relevant in custom architectures, for example where parse server and clients individually interact with a 3rd party system and rely on this common ID.

The more I look into it, the more I am skeptical of solutions A and B.

@sunshineo What do you think about solution C?

@davimacedo
Copy link
Member

davimacedo commented Apr 15, 2020

In my opinion, solution C would make way more sense for solving this specific issue about third party integrations. BTW it is already implemented for any api call. You can just go with:

curl -X POST "http://localhost:1337/parse/functions/helloWorld" -d "{ \"_method\": \"POST\", \"_ApplicationId\": \"APPLICATION_ID\" }" -H "Content-Type: application/json"

Are there other issues that we want to fix here by removing the X-Parse-Application-Id?

@sunshineo
Copy link
Contributor Author

sunshineo commented Apr 15, 2020

I agree with @mtrezza and @davimacedo on that if we limit the issue to: make it easy for a 3rd party system to call Parse function as a webhook, option C would be enough. Even without option C, there are workarounds #2652 (comment).

My original motivation to create this issue ticket is to discuss this header in general: what value does it provide, what problems it caused, what if we remove it. I might have focused too much on the problems this header caused and too eager to remove it, but problems are all I've experienced with this header. Thanks again to everyone's input. I've learned a a lot more about this header:

Values:

  • Better security (uncertain, a little obfuscation at most)
  • Identify things on the client side (dashboard and sdk)

Complications if remove:

  • File url contains it and will invalidate CDN cache
  • Many sdks and docs still has it

I would like to propose a new option D:
Phase 1:

  1. Remove all code related to parsing x-parse-application-id from header or body. Remove all code require requests to specify this header. Remove all test code related to this header as well
  2. Make it optional to have app id in the server config. If it is configured, use it in the file url and no where else. Update the server documentation to reflect this.
  3. None of the SDK and their documentation need to change

Phase 2:
Gradually remove this header from each SDK and documentation.

Phase 3 (optional):
Help the community migrate the CDN cache to support the file url without app id. And eventually remove it from the server config and file url.

It is a big change. But I believe in progress forward and reduce legacy tech debt. I'm willing to do the grunt work if we can agree on the approach.

@davimacedo
Copy link
Member

Perhaps the X-Parse-Application-Id does not bring so much value (the main one in my point of view is the additional - weak - security layer) but I also don't see a clear benefit in removing it. I'm really afraid that this task would never be completely finished and the problems that we will have with outdated docs and sdks (not talking about old threads that will not be valid anymore) will cause way more problems than the benefits that it would bring. Given all the effort that it will take, I don't believe it should be a priority for the community.

@sunshineo
Copy link
Contributor Author

Hi, @davimacedo. I personally feel that there is a lot of value removing it and that is why I am offering my time to this work specifically. You could be totally right that this is not the highest priority task for the community. But I am not offering my time to do other tasks with higher priority. So the question is: if I do it following the option D I proposed, how much would it cost the community? Some code reviews to point out problems in my code maybe? Given it will be removing things, I'm pretty confident I can do a good job. Any other concerns?

@mtrezza
Copy link
Member

mtrezza commented Apr 15, 2020

I agree with @davimacedo.

I estimate this to be 100s of hours of work. It’s not only the code changes itself, it’s ensuring the repos stay healthy, the code reviews, the new bugs introduced, the additional questions that come up from users who are confused about the change. And then, imagine in 2 years from now we find out that we need a central app ID because someone wants to implement multi-tenancy or another feature.

I personally would rather see the effort invested to fix other open issues. There are enough bugs that need to be addressed or feature request that would bring greater benefit.

Sent with GitHawk

@davimacedo
Copy link
Member

davimacedo commented Apr 15, 2020

Any other concerns?

Yes. There are many adapters out there to be updated as well. In the users point of view, it would be a breaking change that would make everybody to update their client codes and publish a new version of their apps on the stores in order to run the new version of Parse Server. While their users are still updating their apps and part of them are sending app id and part of them not sending app id, we'd need to make sure that no bugs will happen neither in Parse Server nor in any of the adapters.

Again, in my personal opinion, we are removing one benefit (even being small), adding no benefits, possibly creating problems, and putting a huge effort.

@acinader and @dplewis may have a different opinion.

@mtrezza
Copy link
Member

mtrezza commented Apr 15, 2020

Given it will be removing things, I'm pretty confident I can do a good job.

No, it is not just "removing things", which should be clear from the comments above.
And it's not just your personal time, but community time to review code and fix bugs.
You keep ignoring the concerns and - for whatever reason - want to push it your way.

This issue can be closed from my point of view.

@sunshineo
Copy link
Contributor Author

sunshineo commented Apr 15, 2020

@mtrezza 100s of hours is way way higher than what I estimated. Is there something I missed? Besides on the file url (which I won't change) all the code around the header is check if it matches. I think remove them is probably very safe and easy. For multi-tenancy, it's been 4 years since the project comes out of Parse.com . What is the likelihood it will come back within a couple of years. And are there any demand from existing users besides maybe back4app.com. I totally pointed out that it is not just my time but community time to review code. I don't think I will introduce bugs but if I did, of course I will fix them and not use community's time. Thank you @mtrezza for providing your opinions and concerns. I have been acknowledging them, but I want to to discuss details for those vague ones to see if they are real concerns. For example, the concern that this reduces security, is that real? Fear prevents progress. I don't want unfounded concerns preventing us from improving this amazing project. Lastly, I'm not pushing the particular change, but I want to push the discussion to address concerns, analyze cost benefits. Also I'm learning a lot from you guys. I'm not married to my idea, but I don't want it to be brushed off lightly with some vague "concerns" either.

@davimacedo Could you please elaborate why the adapters need to be updated and why client code need to change? I think old sdks and clients will continue to work without any problem. New client won't work with old server that still requires app id, but I don't think that is a problem.

@davimacedo
Copy link
Member

@sunshineo

You are probably underestimating the effort. Only to mention few places we would need to fix:

About the client code, it depends on how we would aim to handle this. Would we just ignore the app id if sent? In this case I agree that no changes would be required in the client code for old clients.

@sunshineo
Copy link
Contributor Author

sunshineo commented Apr 15, 2020

@sunshineo

You are probably underestimating the effort. Only to mention few places we would need to fix:

Thank you @davimacedo on pointing out the cache and adapters. I should adjust my estimation higher.

Brief look to me they are just used as the key to retrieve config from a map that will always just have one value.

Yes, these file adaptors need some work to continue to support the url with app id

This is just like all other SDKs that I can update one by one in Phase 2. I probably even could mark Phase 2 as optional

About the client code, it depends on how we would aim to handle this. Would we just ignore the app id if sent? In this case I agree that no changes would be required in the client code for old clients.

Any concerns on just ignore the app id?

@acinader
Copy link
Contributor

acinader commented Apr 15, 2020

In theory, I support Gordon's suggestion.

The app id is providing zero value and some level of overall, un-needed complexity.

I have bumped into this issue myself over the years and never had the gumption to question the value of the app id itself! So nice one Gordon.

If we could wave a magic wand and have it disappear, that would be the right call, in my opinion. Simplify whenever possible.

I also would not either make it optional nor would I worry about changing the image URLs (so long as the final solution wouldn't cause any 404's). If a user is worried about their CDN charges, they don't have to upgrade. I think this is a marginal issue.

Gordon, you indicated that you're up for the task. I'll certainly try to help you think about the full scope so you can reaffirm that you think it is worth the effort. I think Davi and Diamond have given you some good touchpoints to consider.

If after considering the scope, you still think it's worth biting off, I'll gladly help with the code reviews.

I suspect that if we think about it, we could find a higher value project for your time though ;).

@davimacedo
Copy link
Member

Any concerns on just ignore the app id?

No concerns after all sdks and docs updated. In the meantime it would be weird to have a sdk pointing out an app id to be sent but the server accepting anything you send.

@sunshineo
Copy link
Contributor Author

@davimacedo very valid concern. I imagine I need to update the sdk docs to state the app id is required before a specific server version and optional after. I can get those doc changes ready early and start to push them out once the server side change is released to make the doc not weird for as little time as possible. How was it done before? Is there an official procedure?

@TomWFox
Copy link
Contributor

TomWFox commented Apr 15, 2020

Ooo my bit, how exciting, I’ve been waiting for my time to shine 😅😂

@sunshineo you can submit PRs to the docs repo whenever, I (and others when needed) review and then I label them as “waiting for release” and merge once a release has been made for Parse Server (or other repos).

@sunshineo
Copy link
Contributor Author

sunshineo commented Apr 19, 2020

I dived in and spent about 20 hours. It seems to me that:

  1. It's very hard to make app Id optional
  2. It is less hard than 1 to completely remove it
  3. It is less hard than 2 to give it a default value (not the same as optional) and keep using it everywhere
  4. It is less hard than 3 to keep app Id and keep using it everywhere except just remove the requirement on passing x-parse-application-id in the header. Need to add the assumption that there is one and only one app running, which I think we can all agree on. Let me know if not.

I have 4 done. It was hard. A lot of changes especially on tests. Test often create many servers unnecessarily and sometimes are just plain wrong. I'll be releasing small PRs one by one to inch towards 4. The first one would be #6629

@davimacedo
Copy link
Member

4 is very weird to me. We will continue having an app id that can be setup in the server as before but sending it from the client as before will not make any effect anymore. It would be little bit less weird if we add a new option in Parse Server like verifyAppId. I also didn't understand the https://github.com/parse-community/parse-server/pull/6629/files , could you please explain what you want to achieve with this first PR?

@sunshineo
Copy link
Contributor Author

@davimacedo 4 is the first step towards get rid of app id. Maybe I should not have used numbers and made it felt like different options excluding each other. app id is used in many places on many things as discussed above. One of them is all request must have the x-parse-application-id header. From the discussions above this header does not provide much value except maybe a little obfuscation as security. And this header is causing some pain (a lot pain to me personally). So that's why it is the first thing I want to remove.

I totally understand that the user have to setup something but not immediately obvious how to use it might feels weird. But I think the solution would be continue to work towards 3 after 4 is done. 3 will avoid the CDN caching problem brought up above. Existing users still providing the appId will have the path not changed. verifyAppId option is going to be hard to implement and error prone, it's similar to 1, and I played with it and gave it a lot thoughts, so trust me.

So now let me explain the first PR. I have 4 already finished but there are a lot of changes and hard to review. I'm trying to break it down to smaller pieces and merge one by one. To not verify x-parse-application-id, we need to assume that there is one and only one parse server instance running. That should be the case of all Parse users as discussed in tickets listed above multi-tenet is never supported after Parse become open source. Please let me know if there is only one parse server instance is not something we can assume. If not this whole thing won't work.

A few tests does not use the parse server instance created in beforeEach of the test. Maybe they were written before the beforeEach was introduced. So they broke after I stop pick app based on appId from the x-parse-application-id header. So in the first PR, I'm changing one of them to use the instance from beforeEach. And it won't break afterwards.

@davimacedo
Copy link
Member

Regarding the sole parse server instance running in the process, I believe it is ok for most of the cases but it is for a sure a breaking change that will affect some users (myself included but I'd be able to overcome it in my specific use case). So I think it is something that would be great to also hear from the others and/or maybe open a discussion in the Community Forum.

Regarding the small PRs, I (in my personal opinion) would prefer to see the whole PR in place. We can even open a branch so you can do separate commits or separate PRs to that branch, and, once completed, we can send a single PR from that branch to the master. @acinader @dplewis any thoughts?

@mman
Copy link
Contributor

mman commented Apr 21, 2020

I do not claim to know everything, but I am on parse since the very commercial beginnings. I believe the app id was initially developed do allow hosting of multiple parse apps within the same node process and split traffic between them correctly. I believe it might have been useful for original parse.com deployment. I do not think it is a good practice anyway.

Nowadays when we host parse in custom (or officially built) docker images scaling horizontally in Kubernetes or the likes and being installed behind a reverse proxy or ingress router, the app id is becoming useless because reverse proxy or ingress router can do this work for you easier. Splitting the traffic based on host, path, headers, whatever you wish.

That being said, it is a very intensive and potentially risky change and parse is opensource so time is limited. We have to be very wise here.

I think I am still +1 on doing it bit by bit.

If anybody sees a use case for having the app id present in every request, please speak up.

Thanks,
Martin

@sunshineo
Copy link
Contributor Author

sunshineo commented Apr 21, 2020

@davimacedo I would love to hear more details on how you use multiple instances of Parse server. I created a thread on Community Forum asking for feedback https://community.parseplatform.org/t/advance-users-rfc-do-you-create-multiple-parse-server-instances-in-one-thread/188 . And lastly the PR is simply a test improvement stands on its own and should be merged regardless

@davimacedo
Copy link
Member

I use it to scale multiple apps that have the same cloud code but are connecting to different databases. I'm pretty sure there are other guys with similar cases out there. I don't believe it is a big issue though (in my case I can easily get rid of it) and we can maybe assume there is only one instance of parse server per process. I just don't want any of us to make a unilateral decision about it since it is a breaking change.

Yes. This first PR should be merged regardless the others.

@sunshineo
Copy link
Contributor Author

sunshineo commented Apr 21, 2020

Thank you @mman . I agree that docker and K8 and the upcoming service mesh stuff are much better at handle routing and also align very well with the single thread nature of nodejs. I will continue work on fix test cases that create multiple Parse server instances unnecessarily while waiting for more feedback. Thank you @davimacedo for merging the previous change

@davimacedo
Copy link
Member

I agree that's right for 99% of the use cases. But, if you have a use case with a big number of apps, all of them with the same cloud code, it is easier and more efficient to scale up horizontally many instances of the same process that is able to handle requests for all apps. I know that's a corner case and I'd never ask to include a new feature or change Parse Server to support this corner case. But, since it is the current state, it will break the application of some people (including myself) and it is worth to hear from the community before taking the decision. We also have to consider that X-Parse-Application-Id (the subject of this thread) can be removed regardless of this assumption.

@sunshineo
Copy link
Contributor Author

@davimacedo, do you also need to mount different parse instances to different route? Let me rethink about verifyAppId option. Maybe it is closer to 3 instead of 1 and could work on remove the header without the single instance assumption

@davimacedo
Copy link
Member

In my case, I mount all in the same path but I remember to see questions of other people in the community in which they were mounting app1 in http://host/parse1, app2 in http://host/parse2, and so on.

@sunshineo
Copy link
Contributor Author

@davimacedo @acinader @TomWFox , have a look at my pull request above when you have a chance

@stale
Copy link

stale bot commented Jun 10, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jun 10, 2020
@stale stale bot closed this as completed Jun 17, 2020
@mtrezza mtrezza added enhancement and removed stale labels Jul 25, 2020
sunshineo added a commit to sunshineo/parse-server that referenced this issue Jul 26, 2020
sunshineo added a commit to sunshineo/parse-server that referenced this issue Jul 26, 2020
sunshineo added a commit to sunshineo/parse-server that referenced this issue Jul 26, 2020
sunshineo added a commit to sunshineo/parse-server that referenced this issue Jul 26, 2020
@mtrezza mtrezza added type:feature New feature or improvement of existing feature and removed type:improvement labels Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature or improvement of existing feature
Projects
None yet
Development

No branches or pull requests

8 participants