Skip to content

Use only username to login like it is used for all other authenticati…#110

Merged
rimas-kudelis merged 4 commits into
vexim:masterfrom
Udera:loginfield
Feb 24, 2016
Merged

Use only username to login like it is used for all other authenticati…#110
rimas-kudelis merged 4 commits into
vexim:masterfrom
Udera:loginfield

Conversation

@Udera

@Udera Udera commented Feb 21, 2016

Copy link
Copy Markdown
Collaborator

…on (IMAP/SMTP)

Follow up for this pull request: #106

With the current naming this is just your e-mailaddress (or siteadmin) like it is used for all other login processes (IMAP/SMTP).

@Udera Udera added this to the Version 2.3 milestone Feb 21, 2016
@rimas-kudelis

Copy link
Copy Markdown
Collaborator

I think username guessing should be configurable. While right now it may seem like this configuration step is an overhead, that would change as soon as we add support for custom usernames, which don't have to follow localpart@domainpart schema.
Furthermore, the admin might or might not want this magic in their setup, so it should be possible to not have it enabled, I think.
I'm also unsure if limiting guessed domain name to {current hostname with its leftmost part removed} is satisfactory. I think I'd rather either see some sort of mapping provided via a configuration option, or make it possible to provide the domain name by other means (e.g. via an environment variable). Perhaps a default could be in place, but there should be a way to override it. What do you think?

Comment thread vexim/login.php Outdated
# construct the correct sql statement based on who the user is
if ($_POST['localpart'] === 'siteadmin') {
$query = "SELECT crypt,localpart,user_id,domain,domains.domain_id,users.admin,users.type,domains.enabled AS domainenabled FROM users,domains
if ($_POST['username'] === 'siteadmin') {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the three cases can and should be merged into one.
Also, we should probably discover whether a user is site admin by checking the type field value, not by blindly mapping everyone whose localpart matches.

@Udera

Udera commented Feb 22, 2016

Copy link
Copy Markdown
Collaborator Author

There is only one SQL query left, there was no reason to use three different ones. Also added the option back to guess the hostname.

Comment thread vexim/login.php Outdated
if($domainguess === 1 && $_POST['username']!=='siteadmin') $_POST['username'].='@'.preg_replace ("/^mail\./", "", $_SERVER["SERVER_NAME"]);

# sql statement based on username
$query = "SELECT crypt,username,user_id,domain,domains.domain_id,users.admin,users.type,domains.enabled AS domainenabled, users.enabled AS userenabled

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please prefix all field names with table names?

@Udera

Udera commented Feb 23, 2016

Copy link
Copy Markdown
Collaborator Author

Put in all your ideas @rimas-kudelis

@rimas-kudelis

Copy link
Copy Markdown
Collaborator

Not all of them. You still left the else clause at the very end.

@Udera

Udera commented Feb 24, 2016

Copy link
Copy Markdown
Collaborator Author

Sorry, I thought you wanted to remove else with the including header() command. I updated the last commit.

rimas-kudelis added a commit that referenced this pull request Feb 24, 2016
Use only username to login like is used for other means of authentication.
@rimas-kudelis rimas-kudelis merged commit 0e382f6 into vexim:master Feb 24, 2016
@rimas-kudelis

Copy link
Copy Markdown
Collaborator

Thanks! I would have wanted a line-break just before that last statement, but didn't want to risk annoying you even more. :)

@Udera Udera deleted the loginfield branch February 24, 2016 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants