[GitHub] [syncope] ilgrosso commented on a change in pull request #189: SYNCOPE-1562: Manage tokens for WA GoogleAuth MFA

[GitBox <gi...@apache.org>]

ilgrosso commented on a change in pull request #189:
URL: https://github.com/apache/syncope/pull/189#discussion_r427355325



##########
File path: core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/dao/auth/GoogleMfaAuthTokenDAO.java
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.syncope.core.persistence.api.dao.auth;
+
+import org.apache.syncope.core.persistence.api.dao.DAO;
+import org.apache.syncope.core.persistence.api.entity.auth.GoogleMfaAuthToken;
+
+import java.util.Date;
+import java.util.List;
+
+public interface GoogleMfaAuthTokenDAO extends DAO<GoogleMfaAuthToken> {
+    GoogleMfaAuthToken find(String user, Integer otp);
+
+    GoogleMfaAuthToken find(String key);
+
+    GoogleMfaAuthToken save(GoogleMfaAuthToken token);
+
+    long count(String user);
+
+    long count();
+
+    void deleteAll();
+
+    boolean delete(Integer otp);
+
+    boolean delete(String user);
+
+    boolean delete(String user, Integer otp);
+
+    boolean delete(Date expirationDate);
+
+    List<GoogleMfaAuthToken> findTokensForUser(String user);

Review comment:
       I think this should be renamed as `findForOwner()`

##########
File path: wa/starter/src/test/java/org/apache/syncope/wa/starter/gauth/token/SyncopeWAGoogleMfaAuthTokenRepositoryTest.java
##########
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.syncope.wa.starter.gauth.token;
+
+import org.apereo.cas.gauth.token.GoogleAuthenticatorToken;
+import org.apereo.cas.otp.repository.token.OneTimeTokenRepository;
+
+import org.apache.syncope.wa.starter.AbstractTest;
+import org.junit.jupiter.api.Test;
+import org.springframework.beans.factory.annotation.Autowired;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+public class SyncopeWAGoogleMfaAuthTokenRepositoryTest extends AbstractTest {
+    @Autowired
+    private OneTimeTokenRepository tokenRepository;
+
+    @Test
+    public void verifyOps() {
+        tokenRepository.removeAll();
+        GoogleAuthenticatorToken token = new GoogleAuthenticatorToken(123456, "SyncopeWA");
+        tokenRepository.store(token);
+        assertEquals(1, tokenRepository.count(token.getUserId()));
+        assertEquals(1, tokenRepository.count());
+    }
+

Review comment:
       I know I am picky but... any reason for these two blank lines? :-)

##########
File path: core/am/rest-cxf/src/main/java/org/apache/syncope/core/rest/cxf/service/wa/GoogleMfaAuthTokenServiceImpl.java
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.syncope.core.rest.cxf.service.wa;
+
+import org.apache.syncope.common.lib.SyncopeClientException;
+import org.apache.syncope.common.lib.to.GoogleMfaAuthTokenTO;
+import org.apache.syncope.common.lib.types.ClientExceptionType;
+import org.apache.syncope.common.rest.api.RESTHeaders;
+import org.apache.syncope.common.rest.api.service.wa.GoogleMfaAuthTokenService;
+import org.apache.syncope.core.logic.GoogleMfaAuthTokenLogic;
+import org.apache.syncope.core.persistence.api.dao.NotFoundException;
+import org.apache.syncope.core.rest.cxf.service.AbstractServiceImpl;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.stereotype.Service;
+
+import javax.ws.rs.core.Response;
+
+import java.net.URI;
+import java.util.Date;
+import java.util.List;
+
+@Service
+public class GoogleMfaAuthTokenServiceImpl extends AbstractServiceImpl implements GoogleMfaAuthTokenService {
+    @Autowired
+    private GoogleMfaAuthTokenLogic logic;
+
+    @Override
+    public Response deleteTokensByDate(final Date expirationDate) {
+        boolean result = logic.delete(expirationDate);

Review comment:
       Normally, `delete()` methods should be returning `void` - see https://github.com/apache/syncope/blob/master/common/idm/rest-api/src/main/java/org/apache/syncope/common/rest/api/service/ResourceService.java#L216 for example.

##########
File path: common/am/lib/src/main/java/org/apache/syncope/common/lib/to/GoogleMfaAuthTokenTO.java
##########
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.syncope.common.lib.to;
+
+import org.apache.commons.lang3.builder.EqualsBuilder;
+import org.apache.commons.lang3.builder.HashCodeBuilder;
+import org.apache.syncope.common.lib.BaseBean;
+
+import javax.ws.rs.PathParam;
+import javax.xml.bind.annotation.XmlRootElement;
+import javax.xml.bind.annotation.XmlType;
+
+import java.util.Date;
+
+@XmlRootElement(name = "gauthToken")
+@XmlType
+public class GoogleMfaAuthTokenTO extends BaseBean implements EntityTO {
+
+    private static final long serialVersionUID = 1285073386484048953L;
+
+    private String key;
+
+    private Integer token;
+
+    private String user;
+
+    private Date issuedDate;
+
+    @Override
+    public String getKey() {
+        return key;
+    }
+
+    @PathParam("key")
+    @Override
+    public void setKey(final String key) {
+        this.key = key;
+    }
+
+    public Integer getToken() {
+        return token;
+    }
+
+    public void setToken(final Integer token) {
+        this.token = token;
+    }
+
+    public String getUser() {
+        return user;
+    }
+
+    public void setUser(final String user) {
+        this.user = user;
+    }
+
+    public Date getIssuedDate() {
+        return issuedDate;

Review comment:
       We normally use defensive copy for `Date` values - see https://github.com/apache/syncope/blob/master/common/idrepo/lib/src/main/java/org/apache/syncope/common/lib/to/AbstractStartEndBean.java#L49-L56

##########
File path: common/am/lib/src/main/java/org/apache/syncope/common/lib/to/GoogleMfaAuthTokenTO.java
##########
@@ -0,0 +1,142 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.syncope.common.lib.to;
+
+import org.apache.commons.lang3.builder.EqualsBuilder;
+import org.apache.commons.lang3.builder.HashCodeBuilder;
+import org.apache.syncope.common.lib.BaseBean;
+
+import javax.ws.rs.PathParam;
+import javax.xml.bind.annotation.XmlRootElement;
+import javax.xml.bind.annotation.XmlType;
+
+import java.util.Date;
+
+@XmlRootElement(name = "gauthToken")
+@XmlType
+public class GoogleMfaAuthTokenTO extends BaseBean implements EntityTO {
+
+    private static final long serialVersionUID = 1285073386484048953L;
+
+    private String key;
+
+    private Integer token;
+
+    private String user;

Review comment:
       Would it be possible to rename this field (and all related, including JPA) as `owner` instead? The wording `user` might sound like a Syncope user, which is not.

##########
File path: core/am/rest-cxf/src/main/java/org/apache/syncope/core/rest/cxf/service/wa/GoogleMfaAuthTokenServiceImpl.java
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.syncope.core.rest.cxf.service.wa;
+
+import org.apache.syncope.common.lib.SyncopeClientException;
+import org.apache.syncope.common.lib.to.GoogleMfaAuthTokenTO;
+import org.apache.syncope.common.lib.types.ClientExceptionType;
+import org.apache.syncope.common.rest.api.RESTHeaders;
+import org.apache.syncope.common.rest.api.service.wa.GoogleMfaAuthTokenService;
+import org.apache.syncope.core.logic.GoogleMfaAuthTokenLogic;
+import org.apache.syncope.core.persistence.api.dao.NotFoundException;
+import org.apache.syncope.core.rest.cxf.service.AbstractServiceImpl;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.stereotype.Service;
+
+import javax.ws.rs.core.Response;
+
+import java.net.URI;
+import java.util.Date;
+import java.util.List;
+
+@Service
+public class GoogleMfaAuthTokenServiceImpl extends AbstractServiceImpl implements GoogleMfaAuthTokenService {
+    @Autowired
+    private GoogleMfaAuthTokenLogic logic;
+
+    @Override
+    public Response deleteTokensByDate(final Date expirationDate) {
+        boolean result = logic.delete(expirationDate);
+        return result ? Response.ok().build() : Response.status(Response.Status.BAD_REQUEST).build();
+    }
+
+    @Override
+    public Response deleteToken(final String user, final Integer otp) {
+        boolean result = logic.delete(user, otp);
+        return result ? Response.ok().build() : Response.status(Response.Status.BAD_REQUEST).build();
+    }
+
+    @Override
+    public Response deleteTokensFor(final String user) {
+        boolean result = logic.delete(user);
+        return result ? Response.ok().build() : Response.status(Response.Status.BAD_REQUEST).build();
+    }
+
+    @Override
+    public Response deleteToken(final Integer otp) {
+        boolean result = logic.delete(otp);
+        return result ? Response.ok().build() : Response.status(Response.Status.BAD_REQUEST).build();
+    }
+
+    @Override
+    public Response deleteTokens() {
+        logic.deleteAll();
+        return Response.ok().build();

Review comment:
       this should be
   ```java
   return Response.noContent().build();
   ```
   
   no?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org