-
-
Notifications
You must be signed in to change notification settings - Fork 305
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
[16.0] [IMP] fastapi: Add endpoint public_url field #438
base: 16.0
Are you sure you want to change the base?
Conversation
Hi @lmignon, |
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.
a little comment but LGTM
e9ba274
to
1485354
Compare
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.
LGTM (code review)
I'm not convinced this is the right place to store such a thing. An API endpoint, by definition, does not have knowledge of all its clients, so this notion of public URL of an endpoint does not really make sense to me. |
Or maybe I am misunderstanding what this field means? Is it the URL of some public web site using the API (my initial impression, influenced by use cases I know - in which case this does not make sense to me)? Or is it the URL of the endpoint as exposed via a reverse proxy (in which case I understand, but this may be worth explaining in the help text)? |
In our use case a frontend site is based on an unique endpoint. In this context the public_url field is meant to represent the frontend site url, i.e.: https://oursupersite.shop/ Which is used in mail templating for instance on user registration (on the endpoint /auth/register) : And for impersonation where the flow goes like this : impersonate button in odoo -redirect-> impersonate endpoint -set cookie on api url + redirect-> public_url. -- I understand that an api endpoint can be used by several different frontend sites and a frontend site can use several endpoints which in the former case public_url does not make sense. I also acknowledge that we intentionally mix up the api domain and site domain in impersonation. -- Thing is, in case of a endpoint <-> site architecture, if we don't store this data on the endpoint, where should it be stored? I'd rather avoir relying on Also for the domain mix up I originally meant to add an api public url field too (the reverse proxy one) but we decided to wait for this need to arise. |
Yes, there are many scenarios where the backend needs to know some frontend URLs. But I'm not quite sure storing it on the endpoint is the way to go as it means one endpoint=one frontend and that may not be generic enough. Or at least it may not be something we want to "bless" in the base fastapi module. |
Fair enough, but the need remains. Maybe we need a |
Would it make sense for the frontend to communicate itss own URL through the API calls as needed? Or would that have security implications? |
Without thinking too much, I'd say yes: imagine forging a registration from a spoofing site, the mail sent to the user would be legit but pointing to the spoofing site. In this case we would need a list of authorized urls which in definitive would be similar to the public_url field. |
This introduces an optional
public_url
field in order to avoid hardcoding external site url in mails.(Also used to implement impersonation in fastapi_auth_partner)