> On March 18, 2022, 6:51 a.m., Madhan Neethiraj wrote:
> > security-admin/src/main/java/org/apache/ranger/db/XXAuthSessionDao.java
> > Lines 71 (patched)
> > <https://reviews.apache.org/r/73898/diff/2/?file=2266922#file2266922line71>
> >
> > This will result in 2 queries for each login attempt:
> > 1. to get the last successful login time
> > 2. to get number of failed login since last successful login
> >
> > Consider following to use only one query where possible:
> >
> > public boolean shouldLockUserLogin(String loginId, int seconds, int
> > maxFailedLoginsBeforeLock) {
> > boolean ret = false;
> > Date startTime = new Date(DateUtil.getUTCDate().getTime()
> > - seconds * 1000);
> > int numOfFailedLogins = getFailedLoginsCountForUserSince(loginId,
> > startTime);
> >
> > if (numOfFailedLogins >= maxFailedLoginsBeforeLock) {
> > Date lastSuccessLoginTime =
> > getLastSuccessfulLoginTimeSince(loginId, startTime);
> >
> > if (lastSuccessLoginTime != null) {
> > numOfFailedLogins = getFailedLoginsCountForUser(loginId,
> > lastSuccessLoginTime);
> >
> > ret = numOfFailedLogins >= maxFailedLoginsBeforeLock;
> > }
> > }
> >
> > return ret;
> > }
>
> Kirby Zhou wrote:
> ```
> return getEntityManager()
> .createQuery("SELECT count(1) FROM
> XXAuthSession obj" +
> " WHERE obj.loginId =
> :loginId" +
> " AND obj.createTime > (" +
> " SELECT
> max(obj2.createTime) FROM XXAuthSession obj2" +
> " WHERE obj2.loginId =
> :loginId )" +
> " AND
> obj2.authStatus = 1 )" +
> " AND obj.authStatus != 1",
> Long.class)
> .setParameter("authWindowStartTime",
> authWindowStartTime)
> .setParameter("loginId", loginId)
> .getSingleResult();
>
> ```
>
> How about ?
>
> Kirby Zhou wrote:
> Sorry for paste error.
>
> ```
> .createQuery("SELECT count(1) FROM
> XXAuthSession obj" +
> " WHERE obj.loginId =
> :loginId" +
> " AND obj.createTime >
> :authWindowStartTime" +
> " AND obj.createTime > (" +
> " SELECT
> max(obj2.createTime) FROM XXAuthSession obj2" +
> " WHERE obj2.loginId =
> :loginId " +
> " AND
> obj2.authStatus = 1 )" +
> " AND obj.authStatus != 1",
> Long.class)
> .setParameter("authWindowStartTime",
> authWindowStartTime)
> .setParameter("loginId", loginId)
> .getSingleResult();
> ```
>
> Madhan Neethiraj wrote:
> Does this work when the inner query returns null i.e. when there are now
> successful logins for the user?
>
> Kirby Zhou wrote:
> Good Point!
> ```
> .createQuery("SELECT count(1) FROM
> XXAuthSession obj" +
> " WHERE obj.loginId =
> :loginId" +
> " AND obj.createTime >
> :authWindowStartTime" +
> " AND obj.createTime >
> COALESCE(" +
> " (SELECT
> max(obj2.createTime) FROM XXAuthSession obj2" +
> " WHERE obj2.loginId
> = :loginId " +
> " AND
> obj2.authStatus = 1)," +
> " 1)" +
> " AND obj.authStatus != 1",
> Long.class)
>
> ```
That works!
Here is a slightly better version, as this restricts the query inside
COALESCE() to look for logins only after authWindowStartTime.
SELECT COUNT(1) FROM XXAuthSession obj
WHERE obj.loginId = :loginId
AND obj.authStatus = 1
AND obj.createTime > COALESCE((SELECT MAX(obj2.createTime) FROM XAuthSession
obj2
WHERE obj2.loginId = :loginId
AND obj2.authStatus = 1
AND obj2.createTime >
:authWindowStartTime),
:authWindowStartTime)
Also, please add this query to jpa_named_queries.xml and use the named query
here.
- Madhan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73898/#review224174
-----------------------------------------------------------
On March 18, 2022, 5:59 a.m., Kirby Zhou wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73898/
> -----------------------------------------------------------
>
> (Updated March 18, 2022, 5:59 a.m.)
>
>
> Review request for ranger, Bhavik Bavishi, Abhay Kulkarni, Madhan Neethiraj,
> and Pradeep Agrawal.
>
>
> Bugs: RANGER-2362
> https://issues.apache.org/jira/browse/RANGER-2362
>
>
> Repository: ranger
>
>
> Description
> -------
>
> RANGER-2362
>
>
> Here is a simple demo code for discussion.
>
> Hard-codeed:
> we limit 3 failures per 30 minutes. A successful login will reset the counter.
>
>
> BTW: I think the code of RangerAuthenticationProvider is a bit anti-pattern.
>
> 1. We new RangerAuthenticationProvider at each time user login. It is
> unreasonable, it should be a bean.
>
> see RangerKRBAuthenticationFilter.java and RangerSSOAuthenticationFilter.java
>
> 2. We new Jdbc/AD/Ldap/Pam authentication provider in
> RangerAuthenticationProvider at each time user login.
>
> 3. The member 'private LdapAuthenticator authenticator' seems useless
>
> 4. The RangerAuthenticationProvider seem should be replaced with
> ProviderManager or something like spring configuration.
>
>
> Diffs
> -----
>
> security-admin/src/main/java/org/apache/ranger/biz/SessionMgr.java
> 6b002cff994dd431a83ef46f10ee839fb83dafbb
> security-admin/src/main/java/org/apache/ranger/db/XXAuthSessionDao.java
> b0270e9d45aa5b5543735318eea4e22683cbfece
>
> security-admin/src/main/java/org/apache/ranger/security/handler/RangerAuthenticationProvider.java
> 8f7abbe7df3d0344c7b5b1af89f7322d82a0d238
>
> security-admin/src/main/java/org/apache/ranger/security/listener/SpringEventListener.java
> af5622a5f756db931a7173ad01d8c4162d5ee05a
>
>
> Diff: https://reviews.apache.org/r/73898/diff/2/
>
>
> Testing
> -------
>
> Self tested
>
>
> Thanks,
>
> Kirby Zhou
>
>
