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

EZP-26794 : unable to login in backend #1873

Merged
merged 1 commit into from Jan 11, 2017

Conversation

@nicolas-bastien
Copy link
Contributor

commented Jan 4, 2017

Jira : https://jira.ez.no/browse/EZP-26794

Description :

Clear unauthorized user authentication if they don't have acces to remove siteaccess unwanted exception

Test manually

Trying to login using a user whose user group has no assigned role

  • Create a user group - Group1
  • Under Group1, create a user - user1 (don't assign any role to Group1)
  • Try to login in admin interface using the user1
  • We are unable to do so, because of a "401 - unauthorized" - ok once again
  • Right after, try to login as admin

-> i should work

QA

composer require ezsystems/ezpublish-kernel:dev-EZP-26794-unable-to-login
@nicolas-bastien nicolas-bastien requested review from bdunogier and dpobel Jan 4, 2017
@@ -92,8 +92,10 @@ public function createSessionAction(Request $request)
// Already logged in with another user, this will be converted to HTTP status 409
return new Values\Conflict();
} catch (AuthenticationException $e) {
$this->authenticator->logout($request);

This comment has been minimized.

Copy link
@bdunogier

bdunogier Jan 4, 2017

Member

Is the user authenticated when there is an authentication error ? I understand that the later case, where the user doesn't have enough permissions, leaves the user logged in, but I don't think it does, or should, when auth failed.

This comment has been minimized.

Copy link
@nicolas-bastien

nicolas-bastien Jan 5, 2017

Author Contributor

@bdunogier you're right, done

@nicolas-bastien nicolas-bastien force-pushed the EZP-26794-unable-to-login branch from c42c86c to 795f969 Jan 5, 2017
@andrerom

This comment has been minimized.

Copy link
Member

commented Jan 5, 2017

Is this fixing two issues? I don't fully see the connection with the logout() call to making sure ez route does not throw a UnauthorizedSiteAccessException.

@bdunogier

This comment has been minimized.

Copy link
Member

commented Jan 6, 2017

The idea is that if the account doesn't have the right authorizations, then the user shouldn't be logged in, as it will be prevent him from trying to login again.

But in retrospect, I am not sure what the route check on /ez or not.

@andrerom

This comment has been minimized.

Copy link
Member

commented Jan 6, 2017

and by "not right authorizations", we are talking about not access to user/login right? or?

@bdunogier

This comment has been minimized.

Copy link
Member

commented Jan 6, 2017

we are talking about not access to user/login right? or?

Yes sir.

@andrerom

This comment has been minimized.

Copy link
Member

commented Jan 6, 2017

Then I'm +1
But what are your concern regarding the ez route check?

Clear unauthorized user authentication if they don't have acces to remove siteaccess unwanted exception
@nicolas-bastien nicolas-bastien force-pushed the EZP-26794-unable-to-login branch from 795f969 to 6dacc48 Jan 6, 2017
@nicolas-bastien

This comment has been minimized.

Copy link
Contributor Author

commented Jan 6, 2017

@andrerom you're right the ez route check was not needed

So here is my PR updated a single line !

/cc @bdunogier

@nicolas-bastien nicolas-bastien requested review from bdunogier and andrerom Jan 6, 2017
@bdunogier

This comment has been minimized.

Copy link
Member

commented Jan 11, 2017

@nicolas-bastien you can send this to QA now (and then merge 😄 )

@nicolas-bastien

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2017

I just did it and I'll wait this time ;-)

@nicolas-bastien nicolas-bastien merged commit 61fa420 into 6.7 Jan 11, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
ezrobot Code review by ezrobot
Details
@nicolas-bastien nicolas-bastien deleted the EZP-26794-unable-to-login branch Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.