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

Add an optional ACCOUNT_EMAIL_CONFIRMATION_URL configuration #1213

Closed
wants to merge 2 commits into from

Conversation

ezegolub
Copy link

@ezegolub ezegolub commented Dec 2, 2015

Hi, i've been using django-allauth for a rest api, it works very good but i found that this use case has a problem: the email being sent contains an url to the api itself instead of the front end.
This was solvable via subclassing, of course, but i thought it would be nice to have it supported inside the module.

@stantond
Copy link
Contributor

Is the URL itself configurable here?

@ezegolub
Copy link
Author

Well, it was already generating a url using

        url = reverse(
            "account_confirm_email",
            args=[emailconfirmation.key])
        ret = build_absolute_uri(
            request,
            url,
            protocol=app_settings.DEFAULT_HTTP_PROTOCOL)
        return ret

This just overrides that url with a custom one you define in settings.py

@stantond
Copy link
Contributor

stantond commented Jun 24, 2016

Got it. I thought

The url should conform to http://example.com/confirm/{}, since the key will be added to this url

meant you'd be using [domain].com/confirm regardless rather than being able to set it to whatever is convenient.

Also, I agree that this would be very helpful, having the same issue!

@ezegolub
Copy link
Author

I think it's just a documentation issue at this point, what i meant to explain is that the URL string needs to have a {} somewhere, which will get replaced by the key.
Is that what you meant? I'll improve the docs if so.

@pennersr
Copy link
Owner

If the issue is that allauth is directing the user to reverse("account_confirm_email"...), whereas that URL got replaced in your project by an API, then I think allauth is not at fault here. I think this should be solved elsewhere ...

@stantond
Copy link
Contributor

stantond commented Jun 27, 2016

Please correct me if I'm wrong...

Using a Client + REST API, email verification link works like this:

Email Client ---GET link with confirmation key---> App Client
App Client ---POST with key---> REST API

As it's django-allauth that sends out the link for email verification, the cleanest way to implement the above is by giving django-allauth an out-of-the-box configurable email verification URL, keeping this piece encapsulated yet making it easy to use it to power an API.

I'm using django-rest-auth, which uses django-allauth for registration. It's very popular, which suggests this could benefit a lot of django-allauth users. There is an email verification endpoint that expects the key via POST.

@ezegolub What about:

ACCOUNT_EMAIL_CONFIRMATION_URL (=None)
The URL to use in the Email Confirmation email. Useful when building with a separate Client and API, as you'll need this GET URL to point to your Client App, and your Client App to make the POST call to your API endpoint for django-allauth email confirmation. The key will replace {}. Example: http://example.com/account/confirm_email/{} Set to None to use the default reverse("account_confirm_email"...).

@pennersr
Copy link
Owner

Email Client ---GET link with confirmation key---> App Client

I am not sure what you mean by "App Client". Are we talking about single page apps, mobile apps? Given that emails can be opened on any device, mobile or desktop, I would expect that the link inside the email is at any time a regular link to a normal web page, and not some (mobile) app link (if that is what you are referring to by "App Client"). So, following this logic that web page is either a server-side generated page, or some Angular/React/whatever client side SPA generated page, right? In case of the latter, you can still hook up your SPA router to the /account/confirm_email/{} endpoint.

Common practice in Django when you do not agree with the URL patterns of a reusable app is to provide your own urls.py pointing to the views of the reusable app, but providing your own patterns. I am not very fond of introducing additional settings for stuff that is supposedly to be handled in urls.py. You would probably want to alter password reset as well, and before you know it we have settings for each individual URL path.

@stantond
Copy link
Contributor

I meant with an SPA.

Is it possible to do this by just overriding account_confirm_email in urls.py? A SPA could be under a different domain. I'm not sure how to do this.

@pennersr
Copy link
Owner

Suppose your SPA is running on app.somewhere.com, and Django is running on backend.somewhere.com. The backend has urls for both allauth and rest-auth, but the allauth ones are apparently not going to be used in your project. So you are going to have to rewire the urls anyway, otherwise if somebody starts poking around on the backend domain he will find working but possibly unstyled/broken allauth views?

@stantond
Copy link
Contributor

The email that gets sent out needs to have a SPA URL in it for allauth to be used as a REST API. That's the only place this matters, as it'll go to an SPA. I don't understand why the urls.py needs significant work (perhaps this is already done in django-rest-auth).

@ezegolub how have you solved this previously? Does this issue appear for any other allauth functionality?

@pennersr
Copy link
Owner

If you simply include allauth urls as is, people will be able to view non-SPA urls for login, signup and whatever else allauth has to offer. So users will be able to visit an SPA based login, and the original non-SPA based login.

For now, I am closing this for reasons mentioned above...

@pennersr pennersr closed this Aug 17, 2016
@BenDevelopment
Copy link

BenDevelopment commented Oct 14, 2016

Sorry for commenting a closed issue, but I'm trying to override the confirmation email as mentionned here but I don't really know how to do that.
As you said, I would like to provide an custom validation email such:
http://www.my_frontend_url.com/verify-email/MTE:1bv2mi:axIjcoAzc03SnET7Ec5PTM8bmaM/
instead of:
http://www.my_backend_url.com/rest-auth/registration/account-confirm-email/MTE:1bv2mi:axIjcoAzc03SnET7Ec5PTM8bmaM/
Because I don't want the frontend url to be visible on the confirmation email.
How can I override get_email_confirmation_url method?
I would like to do something like this:

def get_email_confirmation_url(self, request, emailconfirmation):

    url = getattr(settings, "ACCOUNT_EMAIL_CONFIRMATION_URL", None)
    return url.format(emailconfirmation.key)

Exactly as suggested here: https://github.com/pennersr/django-allauth/pull/1213/files?diff=split&short_path=eead337&unchanged=expanded

Is it possible to override only this method in my project without affecting the allauth class.

Maybe it's more a django question than allauth. I'm sorry if it's the case (as said in other issues, I'm new in Django :-) ).

@pennersr
Copy link
Owner

pennersr commented Oct 14, 2016

You need to make sure that some URLs, including the email verification one, end up in your SPA. For that, you could simply alter your project urls.py in such a way that a url for /verify-email/ gets routed to say aclass SPAView(TemplateView)... which returns e.g. an index.html that boots up your SPA. If you name that url "account_confirm_email" you're done...

@stantond
Copy link
Contributor

That souds a lot more complex than sending out a link to the SPA in the email though

@pennersr
Copy link
Owner

I am probably not seeing the complete picture here. But, I keep hitting the same point as I mentioned in the first few comments... namely that you cannot do all of this properly without altering the url routing anyway.

If you use the allauth default routing as is, then you are giving your users access to two confirm email views: the allauth one over at /accounts/confirm-email/ and your project specific SPA one over at /verify-email. That is not the way things should be. You should have just one of those, so you have to tweak the url routing anyway. Now, we could indeed alter allauth so that you can plugin a custom email confirmation link, but that does not take away the complexity of having to revisit urls.py.

@pennersr
Copy link
Owner

Wondering: what API/XHR endpoint is the SPA confirm-email route hooked up to? allauth does not offer this yet, so that could be an improvement point indeed.

@BenDevelopment
Copy link

BenDevelopment commented Oct 14, 2016

The backend will only have one confirmation url as it is the case now. The frontend url is only a frontend view (in my case this is an Angular component), which call the backend confirmation url via a service. The frontend url is not concerned by the django url routing.
You say that this is giving the users access to two confirm email views. But concerning Django, There will be only one confirm email views.

I'm not sure if I understand your point of view (and I'm not sure if my point of view is well explained).

@ezegolub
Copy link
Author

Hey, sorry, IRL stuff prevented me from answering before. I'm on the same situation as @BenDevelopment. My setup was:
User <--> SPA (Angular hosted in s3) <--> API (Django Rest Framework)
(this is all from memory from April, and it was a POS POC, so be kind)
The problem i had was that i didn't wanted the user to be outside of the SPA per the client requirements, and having Django return the SPA to the user was not a good solution because SPA and API were tracked by different repos. In retrospect, i could have had the API do a 301 redirect to the SPA, but i thought it would be easier to just have the SPA itself be the url that got sent out from the API.
I don't know if this is well explained or not though. I'll expand it if needed.

@BenDevelopment
Copy link

For that matter, we can say that there are two access for each backend url because for each backend url there is a frontend service calling it. So you have the backend url (django) and the frontend "url" (which is the service call).
I don't know if you get what I try to explain. English is not my native language :/ .

@pennersr
Copy link
Owner

So, you have a django backend, hosted at be.project.com and an NG frontend, hosted at fe.project.com. Or, perhaps they are both hosted at simply one www.project.com, but then you need to make sure that some URLs are routed to the frontend, and some to the backend.

Either way, if you install vanilla allauth, you likely have the following URLs:

  1. be.project.com/xhr/verify-email/ -- the API endpoint for the SPA
  2. be.project.com/accounts/confirm-email/ -- the default allauth view
  3. fe.project.com/verify-email/ -- your SPA

My point is, that 2. should not be in there, neither should any of the regular allauth frontend views.

@pennersr
Copy link
Owner

FWIW, I would really like to cater for an SPA use case, so please all provide your input even while this ticket is closed. I closed it because I do not think making the email confirmation link in the mail pluggable is the solution, at least, not as long as the above has not been cleared up.

@BenDevelopment
Copy link

Ok that is more clear now.
In my concrete case, I have these urls:

  1. http://backend.com/rest-auth/registration/account-confirm-email/MTE:1bv2mi:axIjcoAzc03SnET7Ec5PTM8bmaM/
  2. http://backend.com/rest-auth/registration/verify-email/MTE:1bv2mi:axIjcoAzc03SnET7Ec5PTM8bmaM/
  3. http://frontend.com/verify-email/MTE:1bv2mi:axIjcoAzc03SnET7Ec5PTM8bmaM/

The 3. is routed (in my NG app) to a service which calls 2., so you are right, 1. is not used here.

@pennersr
Copy link
Owner

pennersr commented Oct 14, 2016

But, unless you tweaked your urls.py you have this one as well right?:

4 . http://backend.com/accounts/confirm-email/123/ (the allauth view)

@BenDevelopment
Copy link

Yes you're right. But I don't use it because there is the rest-auth one.

@BenDevelopment
Copy link

This methode get_email_confirmation_url from adapter.py returns the rest-auth confirmation url.

@pennersr
Copy link
Owner

While you are not using it, it is exposed to the outside world, which should not be the case. This was my original point. You will need to adjust your urls.py no matter what.

@BenDevelopment
Copy link

BenDevelopment commented Oct 14, 2016

I'm agree with you @pennersr but I think this problem doesn't concerns the fact that we can allow to customize the url sent in the registration email.
Without talking about this feature, the problem you are pointing exists in any case if you use rest-auth with all-auth.

@stantond
Copy link
Contributor

stantond commented Oct 14, 2016

My understanding of the situation:

  1. SPA: user actions trigger email activation or password reset
  2. SPA: calls API to send appropriate email to user
  3. API: sends appropriate email to user
  4. User clicks link in email
  5. Link takes user to SPA
  6. SPA: calls API for activation or password reset
  7. API: Does appropriate actions

Is that the correct, ideal flow?

If so, the problem:
Steps 3, 4 and 5 require that the API know the corresponding SPA URLs in order to send these URLs to the user via email. What's the best way for the API to know what these two SPA URLs are?

@BenDevelopment
Copy link

BenDevelopment commented Oct 14, 2016

Yes @stantond that's the correct workflow.
To make this workflow working on my project, I had to override the ConfirmEmailView:

class ConfirmEmailView(View):    
    def get(self, *args, **kwargs):
        return redirect(settings.VALIDATION_MAIL_FRONTEND_URL + kwargs['key']);

and to override the rest-auth url (because when you use allauth with rest-auth, the get_email_confirmation_url method from allauth returns http://site.com/rest-auth/registration/account-confirm-email/key/):

url(r'^rest-auth/registration/account-confirm-email/(?P<key>[-:\w]+)/$', ConfirmEmailView.as_view(), name="account_confirm_email"),

This workflow works well, but the problem is that the activation url in your email is a backend url.

@pennersr
Copy link
Owner

I understand, but I want to avoid putting multiple local tweaks into allauth in order to cater for the SPA use case, without taking the complete picture into perspective. Points:

  1. Most allauth views are already XHR aware: e.g. when an XHR request is made, form errors are already returned as a JSON blob. This could be used as your backend API. So, we have overlap with rest-auth here, and the question becomes would it be possible to drop rest-auth altogether and use allauth's XHR support ?
  2. What if there was a setting in allauth, e.g. XHR_ONLY that, when set, causes the XHR aware views to become XHR only. Or, perhaps when XHR_ONLY = True allauth should just move out of the way (as in, remove irrelevant urls from urls.py) and leave it up to other apps such as rest-auth to do their job.
  3. Regardless of the previous points, the issue with the URLs remain. Here, I am still interested
    to learn how your http://frontend.com/verify-email/MTE:1bv2mi:axIjcoAzc03SnET7Ec5PTM8bmaM/ gets served. Because, if there is a regular Django view behind this, one that responds with HTML that boots up your SPA, which routes to the SPA verify-email view, then you could simply name this Django URL route "account_confirm_email". Alternatively, you can override the def get_email_confirmation_url(self, request, emailconfirmation) adapter method. Either way, this should be sufficient for your use case.

@BenDevelopment
Copy link

BenDevelopment commented Oct 14, 2016

Alternatively, you can override the def get_email_confirmation_url(self, request, emailconfirmation) adapter method. Either way, this should be sufficient for your use case.

Yes, that's exactly what I need. But I don't know how. Can you give me a short sample?

@pennersr
Copy link
Owner

pennersr commented Oct 14, 2016

# settings.py
ACCOUNT_ADAPTER = 'foo.allauth.AccountAdapter'

# foo/allauth.py
from allauth.account.adapter import DefaultAccountAdapter

class AccountAdapter(DefaultAccountAdapter):

    def get_email_confirmation_url(self, request, emailconfirmation):
        return '/your/specific/path/{}'.format(emailconfirmation.key)

@pennersr
Copy link
Owner

Still, I would like to stress that the above is not sufficient, your urls.py needs attention as well to stop allauth views from being accessable by your end users.

@BenDevelopment
Copy link

So the idea would be to disable allauth urls if rest-auth is installed ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants