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

feature/domain_filtering_for_email_sources #3807

Merged
merged 24 commits into from Dec 18, 2018

Conversation

Projects
None yet
3 participants
@jrouzierinverse
Copy link
Member

jrouzierinverse commented Nov 21, 2018

Description

Allow to filter email domains that can be used for the email source

Impacts

EmailSource and the portal email source workflow

Checklist

  • Document the feature
  • Add unit tests
  • Add acceptance tests (TestLink)

NEWS file entries

Enhancements

  • Email Source can have banned and allowed email domains
@extrafu

This comment has been minimized.

Copy link
Member

extrafu commented Nov 21, 2018

@nqb Please test it.

@nqb

This comment has been minimized.

Copy link
Contributor

nqb commented Nov 22, 2018

@extrafu: I didn't see any calls to isEmailAllowed method except for unit tests, are you sure the PR is ready for tests ?

I try to add one domain to the allowed domains field on default email source, I got following error: https://gist.github.com/nqb/c6479592b3d48107cbae3887e7876a9a

Few remarks:

  • Feature has been requested to be use on sponsor sources
  • We should increase size box of the new fields:
    screenshot from 2018-11-22 13-32-07
@nqb

This comment has been minimized.

Copy link
Contributor

nqb commented Nov 22, 2018

I updated description of the PR to add the new checklist.

@jrouzierinverse: you should update the way you create PR based on https://github.com/inverse-inc/packetfence/blob/8e1d98af80a101a45a49528c6e465a85d55477da/.github/PULL_REQUEST_TEMPLATE.md#checklist

@nqb
Copy link
Contributor

nqb left a comment

Please add comments for isEmailAllowed, domains_regexes and make_regex methods.

@jrouzierinverse

This comment has been minimized.

Copy link
Member

jrouzierinverse commented Nov 22, 2018

Sorry did not push the commit

@jrouzierinverse

This comment has been minimized.

Copy link
Member

jrouzierinverse commented Nov 22, 2018

Done

@jrouzierinverse

This comment has been minimized.

Copy link
Member

jrouzierinverse commented Nov 22, 2018

Ready for testing.
Will update documentation

@jrouzierinverse jrouzierinverse force-pushed the jrouzierinverse:feature/domain_filtering_for_email_sources branch from 2faaa03 to e82eb1b Nov 22, 2018

@nqb

This comment has been minimized.

Copy link
Contributor

nqb commented Nov 22, 2018

If I put domains in fields, it works but when I get back to my source, domain fields looks like this: ARRAY(0x558e032f40c0) but seems fine in authentication.conf.

@@ -104,20 +107,85 @@ sub mandatoryFields {
sub authenticate {
my ( $self, $username, $password ) = @_;
my $logger = pf::log::get_logger;
if (!$self->isEmailAllowed($username)) {
$logger->warn("Failed to authenticate using EmailSource ($self->{id}) with PID '$username'");

This comment has been minimized.

@nqb

nqb Nov 22, 2018

Contributor

IMO, we should give a more precise reason in log, like you did before.
Is it possible to include content of "EMAIL_UNAUTHORIZED" ?

This comment has been minimized.

@jrouzierinverse

jrouzierinverse Nov 22, 2018

Member

Of course we can use the include the contents of 'EMAIL_UNAUTHORIZED'
What more precise information to you need?
I included the source id and email address in the log what else do you think we need?

This comment has been minimized.

@nqb

nqb Nov 23, 2018

Contributor

Just to say that the authentication failed due to "banned" domain

@nqb

This comment has been minimized.

Copy link
Contributor

nqb commented Nov 23, 2018

No more ARRAY(0x...) in fields.

tags => {
after_element => \&help,
help => 'A comma-separated list of domains that are banned for email registration. Banned domains are checked before allowed domains.'
},

This comment has been minimized.

@nqb

nqb Nov 26, 2018

Contributor

if banned domains are checked before allowed domains, I propose to reverse order of fields in the form.

This comment has been minimized.

@nqb

nqb Nov 26, 2018

Contributor

If we support regex in domains fields, we should update "help" statements.

}

return qr/@\Q$d\E$/;
}

This comment has been minimized.

@nqb

nqb Nov 26, 2018

Contributor

Could you describe what check the if and what return the function ?

);

Readonly our $COMMUNICATION_ERROR_MSG => 'Unable to validate credentials at the moment';
Readonly our $AUTH_FAIL_MSG => 'Invalid login or password';
Readonly our $AUTH_SUCCESS_MSG => 'Authentication successful.';
Readonly our $INVALID_EMAIL_MSG => 'Invalid e-mail address';
Readonly our $LOCALDOMAIN_EMAIL_UNAUTHORIZED => "You can't register as a guest with this corporate email address. Please register as a regular user using your email address instead.";
Readonly our $EMAIL_UNAUTHORIZED => "Cannot register with this email address.";

This comment has been minimized.

@nqb

nqb Nov 26, 2018

Contributor
Suggested change Beta
Readonly our $EMAIL_UNAUTHORIZED => "Cannot register with this email address.";
Readonly our $EMAIL_UNAUTHORIZED => "Cannot register with this email address: unallowed domain.";
@jrouzierinverse

This comment has been minimized.

Copy link
Member

jrouzierinverse commented Nov 26, 2018

Done re-review

@nqb

This comment has been minimized.

Copy link
Contributor

nqb commented Dec 3, 2018

Thanks a lot for your changes.

My new remarks:

  • register using local domain in uppercase work, not for allowed/banned domains. We need to be consistent: it should not work in any cases.
  • as said before, feature has been requested to be use on sponsor sources. We need to port it.
@nqb

This comment has been minimized.

Copy link
Contributor

nqb commented Dec 3, 2018

I created acceptance tests.

@jrouzierinverse

This comment has been minimized.

Copy link
Member

jrouzierinverse commented Dec 3, 2018

Did the issue come up when the allowed/banned domains are uppercase in the config or during the authentication?

@nqb

This comment has been minimized.

Copy link
Contributor

nqb commented Dec 4, 2018

I'm able to reproduce the issue with following config:

  • allow local domain: checked
  • banned domains: empty
  • allowed domains: one uppercase domain (FREE.fr)

My local domain is: centos-devel.example.lan, I'm able to register with nqb@CENTOS-devel.example.lan. No changes if I put my allowed domain in lowercase.

@nqb

This comment has been minimized.

Copy link
Contributor

nqb commented Dec 10, 2018

register using local domain in uppercase work, not for allowed/banned domains. We need to be consistent: it should not work in any cases.

Still presents.

I got a new issue. If I edit the default email and sponsor sources and update only the Comma-separated list of Allowed Domains field with one domain, I got this error in logs.

If I filled the Comma-separated list of Banned Domains field, error disappears.

@jrouzierinverse

This comment has been minimized.

Copy link
Member

jrouzierinverse commented Dec 11, 2018

Done

@nqb

This comment has been minimized.

Copy link
Contributor

nqb commented Dec 13, 2018

@jrouzierinverse : is this code can be apply as is on a 8.1 installation ?

@nqb

This comment has been minimized.

Copy link
Contributor

nqb commented Dec 13, 2018

Issue in webadmin fix, I don't see the fix for register using local domain in uppercase

@jrouzierinverse

This comment has been minimized.

Copy link
Member

jrouzierinverse commented Dec 13, 2018

Here is the fix dfcc31c and test 2e1b457.

@nqb

This comment has been minimized.

Copy link
Contributor

nqb commented Dec 17, 2018

Now, I'm able to register with domain in uppercase or lowercase.

@nqb

nqb approved these changes Dec 17, 2018

@extrafu

This comment has been minimized.

Copy link
Member

extrafu commented Dec 17, 2018

@nqb If all is fine, please merge and update the NEWS file

@nqb nqb merged commit cd721bb into inverse-inc:devel Dec 18, 2018

1 check passed

license/cla Contributor License Agreement is signed.
Details

nqb added a commit that referenced this pull request Dec 18, 2018

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