fix: Send create user email for password resets where we have an email and person, but no user.

[ThisIsMissEm]

I think this fixes: #6458

I've tested manually by deleting my own auth_user database entry (and associated entries in person_historicalperson and person_historicalemail) to simulate a "missing user but known email and person" condition.

I'm not sure how the codebase is tested, since I'm not usually a python developer, however ietf/manage.py test --settings=settings_test seems to run fine. Though I do think it'd be good to have some sort of test coverage for exactly this case, since it seems like it's a pretty common issue for new editors/authors to encounter when collaborating with an existing IETF member on an I-D.

Sure, we could use a different email template to be a bit more specific about onboarding, but the overall goal I think here is to get the person setup with an account, and the create account flow does exactly that.

Whilst working on this, I did notice that from within the docker container setup, Site.objects.get_current().domain gives datatracker.ietf.org instead of localhost:8000 like I'd have expected, so in the logs the links appeared as:

https://datatracker.ietf.org/accounts/reset/confirm/eyJ1c2VybmFtZSI6ImVtZWxpYUBicmFuZGVkY29kZS5jb20i....YCR3KmkLDZw2ArA_lPg/

Instead of:

http://localhost:8000/accounts/reset/confirm/eyJ1c2VybmFtZSI6ImVtZWxpYUBicmFuZGVkY29kZS5jb20i....YCR3KmkLDZw2ArA_lPg/

I also happen to have Mailpit running in my development environment, on port 2025, however the emails were just logged to the console; I assume this is just a standard development thing in python, but it could be neat to say "actually, yes, send all emails to this test email server"

[ThisIsMissEm]

I just noticed the various tests from https://github.com/ietf-tools/datatracker/blob/main/ietf/ietfauth/tests.py#L397 (which I somehow missed before — I'm definitely not an experienced python developer)

I would've expected at least test_reset_password_address_handling or test_reset_password_without_username to fail, given this change, but they didn't, and I'm not sure why, unless PersonFactory() automatically creates a user record? But I don't think that's what those are doing?

[rjsparks]

Thanks for the PR!

Lots of questions above - we'll answer as best we can as the meeting allows.

re: Mailpit (or any other thing) - when in development mode (what you'd be in while running runserver, mail goes to localhost port 2025. You can change that in settings_local.py by changing:

>>> settings.EMAIL_HOST
'localhost'
>>> settings.EMAIL_PORT
2025

Note that it's localhost from inside the container's perspective - you can map ports out to your host machine to get to services running there.

If you're using the docker/run command, it starts a debuging python smtp server that just prints whatever it gets onto stdout. You can modify that in docker-init.sh (which is created from

datatracker/docker/scripts/app-init.sh

Line 104 in aa36f48

python -m smtpd -n -c DebuggingServer localhost:2025 &

PersonFactory does indeed create a User. The new tests can say PersonFactory(user=None). See

datatracker/ietf/person/factories.py

Line 70 in aa36f48

user = factory.SubFactory(UserFactory)

Again - busy week with meeting - we'll look into the proposed solution and suggest good ways to test it soon.

[ThisIsMissEm]

@rjsparks actually, I just tried PersonFactory(user=None) and PersonFactory isn't happy at all because it assumes the existence of a user, so you get several stacktraces like:

Traceback (most recent call last):
  File "/workspace/ietf/ietfauth/tests.py", line 534, in test_reset_password_without_user
    person = PersonFactory(user=None)
  File "/home/dev/.local/lib/python3.9/site-packages/factory/base.py", line 40, in __call__
    return cls.create(**kwargs)
  File "/home/dev/.local/lib/python3.9/site-packages/factory/base.py", line 528, in create
    return cls._generate(enums.CREATE_STRATEGY, kwargs)
  File "/home/dev/.local/lib/python3.9/site-packages/factory/django.py", line 121, in _generate
    return super()._generate(strategy, params)
  File "/home/dev/.local/lib/python3.9/site-packages/factory/base.py", line 465, in _generate
    return step.build()
  File "/home/dev/.local/lib/python3.9/site-packages/factory/builder.py", line 270, in build
    step.resolve(pre)
  File "/home/dev/.local/lib/python3.9/site-packages/factory/builder.py", line 211, in resolve
    self.attributes[field_name] = getattr(self.stub, field_name)
  File "/home/dev/.local/lib/python3.9/site-packages/factory/builder.py", line 356, in __getattr__
    value = value.evaluate_pre(
  File "/home/dev/.local/lib/python3.9/site-packages/factory/declarations.py", line 67, in evaluate_pre
    return self.evaluate(instance, step, context)
  File "/home/dev/.local/lib/python3.9/site-packages/factory/declarations.py", line 119, in evaluate
    return self.function(instance)
  File "/workspace/ietf/person/factories.py", line 71, in <lambda>
    name = factory.LazyAttribute(lambda p: normalize_name('%s %s'%(p.user.first_name, p.user.last_name)))
AttributeError: 'NoneType' object has no attribute 'first_name'

[rjsparks]

ok - then after you have something like
p = PersonFactory..., do a

p.user = None
p.save()

Other things may complain

[ThisIsMissEm]

Okay, finally managed to get the tests fully working; I had to use EmailFactory instead of PersonFactory, but that still creates a Person so, I don't know why PersonFactory errored out continously.

[rjsparks]

Unfortunately, I think we're going to need a different email round-trip sequence than reusing send_account_creation_email or we're going to need to carefully replumb that message and the handlers for the links it contains.

I walked through the rest of the process manually, and the result was very confusing (even if it mostly ended up with the right result).

The mail that gets emitted looks like this:

MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
From: IETF Secretariat <[email protected]>
To: <[email protected]>
Subject: Confirm registration at datatracker.ietf.org
X-Test-IDTracker: yes
X-IETF-IDTracker: 12.19.0-dev
Auto-Submitted: auto-generated
Precedence: bulk
Message-ID: <172168088514.42273.346891258396035602@a675050b3ff6>
Date: Mon, 22 Jul 2024 13:41:25 -0700
X-Peer: ::1


Hello,

We have received an account creation request for
[email protected] at datatracker.ietf.org.

If you do not already have datatracker account, please go to the following
link and follow the instructions there in order to set a new password for the
[email protected] account:

   https://datatracker.ietf.org/accounts/create/confirm/bunch-of-token-hash-bits-here/

This link will expire in 3 days.

If you already have a datatracker account, please go to the following link
instead, and add the new email [email protected] to your
existing account:

   https://datatracker.ietf.org/accounts/profile/


Best regards,

	The datatracker login manager service
	(for the IETF Secretariat)

Which is disconnected from reality in this case - the "account" exists as far as the end user is concerned, so following the account exists instructions results in the wrong thing happening. Following the "if you don't have an account" path almost results in the right thing, but the user gets a form where they have to provide their name and ascii-name and a password. The provided name and ascii-name don't get used afaict.

Also, we're got this send_account_creation_exists_email path that can get used, that is asking people to do something that won't work, even with this fix. That's going to need to be reconciled while straightening this out.

@ThisIsMissEm - please feel free to continue to push if you have the energy, but if not, we can pick this up and finish it in a bit. We'll definitely use what you've provided so far.

[ThisIsMissEm]

@rjsparks yeah, I totally agree that it's almost right but not quite — though, me being a new contributor + not really a python/django developer, I didn't think it'd be wise for me to setup a completely new flow, due to the security implications of that, hence reusing the existing flow to try to do a minimal "fix this so new comers who are on I-D's as co-authors don't get stuck" sense.

[rjsparks]

The push above takes the solution in a slightly different direction, but I think it's the better path.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.77%. Comparing base (c7f6bde) to head (08b2290).
Report is 23 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7729      +/-   ##
==========================================
- Coverage   88.78%   88.77%   -0.01%     
==========================================
  Files         296      296              
  Lines       41320    41350      +30     
==========================================
+ Hits        36687    36710      +23     
- Misses       4633     4640       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jennifer-richards]

As much a question as a change request - see inline.

Though I think we should remove the comment about "The form validation checks that a matching User exists" because that part of the comment is outright false after the last round of refactoring. (The form no longer does anything beyond email syntax validation.)