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

Email addresses should not be validated on login #6504

Closed
immanuel-h opened this issue Jun 27, 2023 · 27 comments
Closed

Email addresses should not be validated on login #6504

immanuel-h opened this issue Jun 27, 2023 · 27 comments
Assignees
Labels

Comments

@immanuel-h
Copy link

Describe the bug

Email addresses are validated on login, which means that previously valid logins become invalid when upgrading pgadmin.

To Reproduce

Steps to reproduce the behavior:

  1. Using pgadmin 6.x, create a user with a .local domain as the email address ([email protected])
  2. Try logging in with the user to verify it works -> works
  3. Upgrade to pgadmin 7.x
  4. Try logging in with the user again

Expected behavior

The user should be able to login

Error message

"Invalid username or password" is shown.

Why this is problematic?

  • It prevents existing users, who successfully logged in before, from logging in, based on arbitrarily added rules, and requiring the admin to research a solution. (in this case, adding a local config as proposed in Can't use accounts with .local emails #6222 ).
  • There is no indication to the failure reason in the application logs, leaving you to figure out if pgadmin has trouble maybe reading the old pgadmin4.db file with the users? Or maybe something in the nginx config does weird things? Etc.
  • The change is not communicated in the changelogs
@immanuel-h immanuel-h added the Bug label Jun 27, 2023
@GuillaumeRossolini
Copy link

GuillaumeRossolini commented Jun 29, 2023

From what I understand, this is expected.
A workaround is to use environment variables to control the behaviour, for example in the case of a Docker setup:

environment:
  PGADMIN_CONFIG_CHECK_EMAIL_DELIVERABILITY: "False"
  PGADMIN_CONFIG_SECURITY_EMAIL_VALIDATOR_ARGS: "{\"check_deliverability\": CHECK_EMAIL_DELIVERABILITY}"

Or using a config-local.py file:

CHECK_EMAIL_DELIVERABILITY = False
SECURITY_EMAIL_VALIDATOR_ARGS = \
    {"check_deliverability": CHECK_EMAIL_DELIVERABILITY}

[Edit] the syntax of the email address should still be valid even with this, but that's acceptable IMO.

@immanuel-h
Copy link
Author

I don't see this as expected behaviour. An upgrade should not invalidate previously working logins. Of course one can work around the issue, but that's not the point, really.

As you pointed out, emailvalidator seems to allow to validate deliverability separately from syntactic correctness (which might be an actually necessary check, to determine if it's a username or an email). As deliverability of an email is not a factor when logging in, it should not be put into consideration. Such validation should only happen on registration and possibly password recovery (and there it should produce a meaningful error message separate from "invalid email").

@adityatoshniwal
Copy link
Contributor

@GuillaumeRossolini check deliverability is False by default, below is the default setting:

CHECK_EMAIL_DELIVERABILITY = False
SECURITY_EMAIL_VALIDATOR_ARGS = \
    {"check_deliverability": CHECK_EMAIL_DELIVERABILITY}

@GuillaumeRossolini
Copy link

@adityatoshniwal I hadn't seen that, thanks for pointing it out. I had lots of trouble with v5.4 at the time, and I didn't see that 5.5 fixed it. Still, the hardening of the email syntax check isn't obvious, especially in a minor release (which is probably the issue here).

I mostly agree with @immanuel-h, that was a very confusing issue when it happened to me.

It was sort-of documented in the changelog for a minor release, under a seemingly unrelated name:
https://www.pgadmin.org/docs/pgadmin4/6.21/release_notes_5_4.html

Updated Flask-Security-Too to the latest v4.

@immanuel-h
Copy link
Author

@GuillaumeRossolini so this actually comes from flask-security-too? Should I raise a defect with them then?

@GuillaumeRossolini
Copy link

GuillaumeRossolini commented Jun 29, 2023

Yes that's where this change comes from
Flask-Middleware/flask-security@0abe7f4

I don't imagine the folks at flask-security-too would be interested, since their change happened over a major milestone (v4.0.0, I think?). You can expect backwards compatibility issues for major versions.

Perhaps simply, as you suggested, a message in pgadmin's logs?

There is in fact a helpful log message if you start the server with an invalid admin email address (in my case with the Docker image):

environment:
  PGADMIN_DEFAULT_EMAIL: pgadmin@localhost

'pgadmin@localhost' does not appear to be a valid email address. Please reset the PGADMIN_DEFAULT_EMAIL environment variable and try again.

If I rectify the PGADMIN_DEFAULT_EMAIL environment by appending a ".net" then the server starts successfully, but logging in without the ".net" suffix doesn't provide any help to the end user or the instance admin. At no point in the startup sequence did such a message appear either.

[Edit] Looks like the actual change comes from pypi in flask-security v4, at the time it was bumped for pgadmin v5.4
https://flask-security-too.readthedocs.io/en/stable/changelog.html#version-4-0-0rc2

(#394) Email input is now normalized prior to being stored in the DB. Previously, it was validated, but the raw input was stored. Normalization and validation rely on the email_validator package.

@immanuel-h
Copy link
Author

Seems like an unintentional side effect to me then. I.e. the change just is about storing, not validating on login. I'll make a ticket and see what comes of it.

@immanuel-h
Copy link
Author

Nice, flask-security fixed the issue and it should be part of their next release :)

@GuillaumeRossolini
Copy link

@immanuel-h I’m not sure what the schedule is to update this dependency, might have to open a new issue after upstream has tagged their release? No clue

@adityatoshniwal
Copy link
Contributor

adityatoshniwal commented Jul 10, 2023

Nice, flask-security fixed the issue and it should be part of their next release :)

They have only disabled deliverability check.

@thelfensdrfer
Copy link

You also cannot log in if pgadmin thinks it's an invalid email address, in my case it contains a +.

@adityatoshniwal
Copy link
Contributor

You also cannot log in if pgadmin thinks it's an invalid email address, in my case it contains a +.

A syntactically valid email is required.

@adityatoshniwal
Copy link
Contributor

Hi @immanuel-h,
The flask-security is upgraded in pgAdmin. Are we good to close this? Please try on v8.0.

@GuillaumeRossolini
Copy link

A syntactically valid email is required.

The + symbol in an email address is absolutely valid.

@adityatoshniwal
Copy link
Contributor

A syntactically valid email is required.

The + symbol in an email address is absolutely valid.

And it works fine, tested with v8.0

image Screenshot 2023-12-08 at 2 52 54 PM

@immanuel-h
Copy link
Author

Hi, nope looks like the problem persists.

Just to have it documented, my personal workaround is having this in my config_.local.py:

import email_validator
email_validator.SPECIAL_USE_DOMAIN_NAMES.remove("local")

guess I'll reopen that ticket over at flask_security ...

@immanuel-h
Copy link
Author

This is drawing bigger and bigger circles lol. After some browsing code it can be tracked down to
both a bug in flask-security (they updated the wrong setting), and also one in email-validator itself: JoshData/python-email-validator#122

@GuillaumeRossolini
Copy link

The only comprehensive way to validate an email address is to send a secret to that address and to ask for that secret in the UI.

Anything else will fail for one reason or another.

I'm not even sure that pgadmin needs to be able to send emails? I for one, am using it locally with no chance that anyone else can log in, and I have no need for the email part. Pgadmin wouldn't be able to send an email from my system anyway, so it's pointless.

@immanuel-h
Copy link
Author

@GuillaumeRossolini we are all on the same page here ... and so are the upstream packages that actually need to touch code as far as I know :)

The "problem" is that many other users of flask_security do need to send emails, as well as that it absolutely does make sense to normalize email addresses before storing them (which is why they use email-validator).

@adityatoshniwal
Copy link
Contributor

This is not a pgAdmin bug. You can change the behaviour as per your need using config.
Closing this.

@adityatoshniwal adityatoshniwal closed this as not planned Won't fix, can't repro, duplicate, stale Dec 22, 2023
@AlexWinder
Copy link

AlexWinder commented Feb 12, 2024

I'm running into a similar issue with this and would suggest that this is not resolved, @adityatoshniwal

In my experience the validation should not be doing checking on a valid FQDN, as highlighted by @GuillaumeRossolini in #6504 (comment) - or if it does then there should be functionality to override this. The suggested fixes above of PGADMIN_CONFIG_CHECK_EMAIL_DELIVERABILITY and PGADMIN_CONFIG_SECURITY_EMAIL_VALIDATOR_ARGS appear to have no effect in 8.3.

I am working on a project which has a use case for a login which is completely local and isolated from the internet, but the login itself will only be a local login and so there is justification for a .local TLD, or better to just do away with the TLD or domain completely and have something as suggested above with, for example, admin@localhost, or better admin.

I am using PGAdmin 8.3 Docker container.

I would suggest that there is a bug in PGAdmin as there is a forced action to use a FQDN, and allowing use of .local is far safer than allow use of a domain which you may not have control over.

Here is a snippet from my docker-compose.yaml:

pgadmin:
        image: dpage/pgadmin4:8.3
        environment:
            PGADMIN_DEFAULT_EMAIL: [email protected]
            PGADMIN_DEFAULT_PASSWORD: test
            PGADMIN_DISABLE_POSTFIX: True
        depends_on:
            - database
  • [email protected] works fine, however this is dangerous because in my particular use case there is no domain which we own which can be reasonably used.
  • [email protected] builds the container with no warnings or errors but doesn't allow you to login at all.
  • admin@localhost does not start the container at all - 'admin@localhost' does not appear to be a valid email address. Please reset the PGADMIN_DEFAULT_EMAIL environment variable and try again.
  • admin does not start the container at all - 'admin' does not appear to be a valid email address. Please reset the PGADMIN_DEFAULT_EMAIL environment variable and try again.

It's also worth mentioning that the login page also says that you should enter an "Email Address / Username", however unless I'm mistaken there doesn't seem to be any way to just use a username and so this suggests a bug in the login page.

image

@AlexWinder
Copy link

It's also worth mentioning that the behaviour in PGAdmin currently appears to contradict the suggestions made in RFC 2606 and also by IANA.

@adityatoshniwal
Copy link
Contributor

@AlexWinder Please check my comment here - #6222 (comment)

@AlexWinder
Copy link

@adityatoshniwal Thanks for the speedy reply. I've tried the suggesting in your comment however I'm still having the same issues where .local is not accepted. Here are the snippets:

# docker-compose.yaml
pgadmin:
    image: dpage/pgadmin4:8.3
    environment:
        - [email protected]
        - PGADMIN_DEFAULT_PASSWORD=test
        - PGADMIN_DISABLE_POSTFIX=True
    volumes:
        - ./config/pgadmin/config_local.py:/pgadmin/config_local.py
    depends_on:
        - database
# config_local.py
import email_validator
email_validator.SPECIAL_USE_DOMAIN_NAMES.remove("local")

I've tried both mapping to /pgadmin/config_local.py as above, but also copying the file in as part of a Dockerfile and both have the same effort of not allowing login with [email protected].

Any ideas?

@AlexWinder
Copy link

@adityatoshniwal Ignore my above message. I've found what the issue was. I was mapping to the wrong directory. I can now use [email protected]. Thanks for the guidance.

It's worth me mentioning that the documentation of where to put config_local.py is incorrect. It says that for Linux it should be /etc/pgadmin/config_local.py however in the Docker environment it's /pgadmin4/config_local.py.

For the purpose of completeness my docker-compose.yaml looks like as following:

pgadmin:
    image: dpage/pgadmin4:8.3
    environment:
        - [email protected]
        - PGADMIN_DEFAULT_PASSWORD=test
        - PGADMIN_DISABLE_POSTFIX=True
    volumes:
        - ./config/pgadmin/config_local.py:/pgadmin4/config_local.py
    depends_on:
        - database

@adityatoshniwal
Copy link
Contributor

HI @AlexWinder,
We have separate doc page for container deployment here - https://www.pgadmin.org/docs/pgadmin4/8.3/container_deployment.html#mapped-files-and-directories

@adityatoshniwal
Copy link
Contributor

I think we should include this snippet in core code itself and provide one config var to allow special use domain names. That way, simply passing an env var should be enough.

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

No branches or pull requests

5 participants