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

[Cms] Restricting access to your site: Code examples throw exception #1481

Closed
tpmatthes opened this issue Feb 14, 2019 · 3 comments
Closed
Labels
type: enhancement ✨ Suggests an enhancement; improves Kirby
Milestone

Comments

@tpmatthes
Copy link

First of all, thanks for your awesome work on Kirby 3! It’s a solid update. 👍

Describe the bug
I’m using Kirby to manage an internal website for students, who participate in various classes about web development. They need to log in with given account data to see infos about the class and corresponding tasks. I’m currently updating my site from Kirby 2 and encountered two issues.

I’ve implemented the authentication logic in a custom controller. It roughly follows this example: https://getkirby.com/docs/cookbook/security/access-restriction

If students log in with the correct email but wrong password, the code throws an exception:

Kirby \ Exception \ PermissionException (error.access.login)
Invalid login

If students log in with a wrong email, the login() function simply returns false. In Kirby 2 it returned false in both cases, which is more convenient. You’ve discussed this to some extent in #1027, but the issue is closed, so I’ve opened a new one.

Additionally, you can’t login with a username anymore. You’ve discussed this in #763. However, I have a strong argument in favor of usernames: In my case, I don’t want students to see my email adresses. Also, usernames are generally shorter. Would it be possible to extend the login() function to allow logging in via username? You could differentiate usernames from email by searching for an @ symbol.

To Reproduce
Steps to reproduce the behavior:

  1. Install starterkit
  2. Add admin account by visiting yoursite.dev/panel
  3. Add code from https://getkirby.com/docs/cookbook/security/access-restriction
  4. Visit login page
  5. Login with correct email, but wrong password
  6. Look at error message

Expected behavior
The login() function should return false, if used in a controller. I understand that there are arguments for throwing an exception. However, your code examples (see link above) suggest a different behavior.

Kirby Version
3.0.1

@distantnative distantnative changed the title Restricting access to your site: Code examples throw exception [Cms] Restricting access to your site: Code examples throw exception Mar 6, 2019
@distantnative distantnative self-assigned this Mar 6, 2019
@distantnative distantnative added the type: enhancement ✨ Suggests an enhancement; improves Kirby label Mar 17, 2019
@distantnative distantnative added this to the 3.1.2 milestone Mar 17, 2019
@distantnative
Copy link
Member

distantnative commented Mar 17, 2019

Additionally, you can’t login with a username anymore. You’ve discussed this in #763. However, I have a strong argument in favor of usernames: In my case, I don’t want students to see my email adresses. Also, usernames are generally shorter. Would it be possible to extend the login() function to allow logging in via username? You could differentiate usernames from email by searching for an @ symbol.

We have discussed this in length. I don't think this will change anytime soon.

Regarding the rest, my thoughts:

  • wrong email should also throw the same exception, I think it is best practice not to reveal which part was wrong
  • the code examples need to be updated to include try {} catch() {}

@distantnative distantnative removed their assignment Apr 14, 2019
@bastianallgeier
Copy link
Member

I created a ticket for our site. We need to fix the code examples: getkirby/getkirby.com#495

You can also still create a login via username if you search for the user by name:

if ($user = $users->findBy('name', $someName)) {
  // do the login logic here. 
}

You could also create an additional username field for your users, if you prefer a separate username field instead of the name.

@texnixe
Copy link
Member

texnixe commented Apr 16, 2019

@tpmatthes For login via name rather than email, you would have to make sure that names are unique when creating/updating new users (using hooks), otherwise it won't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement ✨ Suggests an enhancement; improves Kirby
Projects
None yet
Development

No branches or pull requests

4 participants