Skip to content

Commit a4d624a

Browse files
georgweissgeorgweiss
committed
Updates based on peer feedback
1 parent ac8c88b commit a4d624a

File tree

7 files changed

+57
-81
lines changed

7 files changed

+57
-81
lines changed

‎app/credentials-management/src/main/java/org/phoebus/applications/credentialsmanagement/CredentialsManagementController.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ private void configureCellFactory() {
162162
Callback<TableColumn<ServiceItem, ServiceItem>, TableCell<ServiceItem, ServiceItem>> actionColumnCellFactory = new Callback<>() {
163163
@Override
164164
public TableCell<ServiceItem, ServiceItem> call(final TableColumn<ServiceItem, ServiceItem> param) {
165-
final TableCell<ServiceItem, ServiceItem> cell = new TableCell<>() {
165+
return new TableCell<>() {
166166

167167
private final Button btn = new Button(Messages.LogoutButtonText);
168168

@@ -194,7 +194,6 @@ public void updateItem(ServiceItem serviceItem, boolean empty) {
194194
}
195195
}
196196
};
197-
return cell;
198197
}
199198
};
200199
actionButtonColumn.setCellFactory(actionColumnCellFactory);
@@ -381,7 +380,7 @@ public AuthenticationStatus login() {
381380
username.get(),
382381
password.get()));
383382
} catch (Exception e) {
384-
throw new RuntimeException(e);
383+
LOGGER.log(Level.WARNING, "Failed to store user credentials");
385384
}
386385
}
387386
this.authenticationStatus.set(authenticationStatus);

‎app/logbook/olog/client-es/src/main/java/org/phoebus/applications/logbook/authentication/OlogServiceAuthenticationProvider.java‎

Lines changed: 1 addition & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -20,42 +20,20 @@
2020

2121
import org.phoebus.olog.es.api.OlogHttpClient;
2222
import org.phoebus.security.authorization.AuthenticationStatus;
23-
import org.phoebus.security.authorization.ServiceAuthenticationException;
2423
import org.phoebus.security.authorization.ServiceAuthenticationProvider;
2524
import org.phoebus.security.tokens.AuthenticationScope;
2625

27-
import java.net.ConnectException;
28-
import java.util.logging.Level;
29-
import java.util.logging.Logger;
30-
3126
public class OlogServiceAuthenticationProvider implements ServiceAuthenticationProvider {
3227

3328
private final AuthenticationScope ologAuthenticationScope;
3429

35-
private final Logger logger = Logger.getLogger(OlogServiceAuthenticationProvider.class.getName());
36-
3730
public OlogServiceAuthenticationProvider() {
3831
ologAuthenticationScope = new OlogAuthenticationScope();
3932
}
4033

4134
@Override
4235
public AuthenticationStatus authenticate(String username, String password) {
43-
try {
44-
OlogHttpClient.builder().build().authenticate(username, password);
45-
return AuthenticationStatus.AUTHENTICATED;
46-
} catch (ConnectException e) {
47-
logger.log(Level.WARNING, "Unable to connect to logbook service");
48-
return AuthenticationStatus.SERVICE_OFFLINE;
49-
} catch (ServiceAuthenticationException e){
50-
logger.log(Level.WARNING, "User " + username + " not authenticated");
51-
return AuthenticationStatus.BAD_CREDENTIALS;
52-
}
53-
catch (Exception e) {
54-
// NOTE!!! Exception message and/or stack trace could contain request URL and consequently
55-
// user's password, so do not log or propagate it.
56-
logger.log(Level.WARNING, "Failed to authenticate user " + username + " with logbook service, reason unknown");
57-
return AuthenticationStatus.UNKNOWN_ERROR;
58-
}
36+
return OlogHttpClient.builder().build().authenticate(username, password);
5937
}
6038

6139
@Override

‎app/logbook/olog/client-es/src/main/java/org/phoebus/olog/es/api/OlogHttpClient.java‎

Lines changed: 25 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,8 @@
2626
import org.phoebus.olog.es.api.model.OlogObjectMappers;
2727
import org.phoebus.olog.es.api.model.OlogSearchResult;
2828
import org.phoebus.olog.es.authentication.LoginCredentials;
29-
import org.phoebus.security.authorization.ServiceAuthenticationException;
29+
import org.phoebus.security.authorization.AuthenticationStatus;
3030
import org.phoebus.security.store.SecureStore;
31-
import org.phoebus.security.tokens.AuthenticationScope;
3231
import org.phoebus.security.tokens.ScopedAuthenticationToken;
3332
import org.phoebus.util.http.HttpRequestMultipartBody;
3433
import org.phoebus.util.http.QueryParamsHelper;
@@ -131,14 +130,13 @@ public static Builder builder() {
131130
* Disallow instantiation.
132131
*/
133132
private OlogHttpClient(String userName, String password) {
134-
if(Preferences.connectTimeout > 0){
133+
if (Preferences.connectTimeout > 0) {
135134
httpClient = HttpClient.newBuilder()
136135
.cookieHandler(new CookieManager(null, CookiePolicy.ACCEPT_ALL))
137136
.followRedirects(HttpClient.Redirect.ALWAYS)
138137
.connectTimeout(Duration.ofMillis(Preferences.connectTimeout))
139138
.build();
140-
}
141-
else{
139+
} else {
142140
httpClient = HttpClient.newBuilder()
143141
.cookieHandler(new CookieManager(null, CookiePolicy.ACCEPT_ALL))
144142
.followRedirects(HttpClient.Redirect.ALWAYS)
@@ -361,20 +359,30 @@ public void groupLogEntries(List<Long> logEntryIds) throws LogbookException {
361359
*
362360
* @param userName Username, must not be <code>null</code>.
363361
* @param password Password, must not be <code>null</code>.
364-
* @throws Exception if the login fails, e.g. bad credentials or service off-line.
362+
* @return An {@link AuthenticationStatus} to indicate the outcome of the login attempt.
365363
*/
366-
public void authenticate(String userName, String password) throws Exception {
364+
public AuthenticationStatus authenticate(String userName, String password) {
367365
String stringBuilder = Preferences.olog_url +
368366
"/login";
369-
HttpRequest request = HttpRequest.newBuilder()
370-
.uri(URI.create(stringBuilder))
371-
.header("Content-Type", CONTENT_TYPE_JSON)
372-
.POST(HttpRequest.BodyPublishers.ofString(OBJECT_MAPPER.writeValueAsString(new LoginCredentials(userName, password))))
373-
.build();
374-
HttpResponse<String> response = httpClient.send(request, HttpResponse.BodyHandlers.ofString());
375-
if (response.statusCode() == 401) {
376-
throw new ServiceAuthenticationException("Failed to login: user not authorized");
367+
try {
368+
HttpRequest request = HttpRequest.newBuilder()
369+
.uri(URI.create(stringBuilder))
370+
.header("Content-Type", CONTENT_TYPE_JSON)
371+
.POST(HttpRequest.BodyPublishers.ofString(OBJECT_MAPPER.writeValueAsString(new LoginCredentials(userName, password))))
372+
.build();
373+
HttpResponse<String> response = httpClient.send(request, HttpResponse.BodyHandlers.ofString());
374+
if (response.statusCode() == 401) {
375+
LOGGER.log(Level.WARNING, "User not authenticated with logbook service");
376+
return AuthenticationStatus.BAD_CREDENTIALS;
377+
}
378+
} catch (ConnectException e) {
379+
LOGGER.log(Level.WARNING, "Cannot connect to logbook service");
380+
return AuthenticationStatus.SERVICE_OFFLINE;
381+
} catch (Exception e) {
382+
LOGGER.log(Level.WARNING, "Unable to send authentication request to service, reason unknown");
383+
return AuthenticationStatus.UNKNOWN_ERROR;
377384
}
385+
return AuthenticationStatus.AUTHENTICATED;
378386
}
379387

380388
/**
@@ -469,7 +477,7 @@ public InputStream getAttachment(Long logId, String attachmentName) {
469477
.GET()
470478
.build();
471479
HttpResponse<InputStream> response = httpClient.send(request, HttpResponse.BodyHandlers.ofInputStream());
472-
if(response.statusCode() >= 300) {
480+
if (response.statusCode() >= 300) {
473481
LOGGER.log(Level.WARNING, "failed to obtain attachment: " + new String(response.body().readAllBytes()));
474482
return null;
475483
}
@@ -556,7 +564,7 @@ public LogTemplate saveTemplate(LogTemplate template) throws LogbookException {
556564
}
557565

558566
@Override
559-
public Collection<LogEntryLevel> listLevels(){
567+
public Collection<LogEntryLevel> listLevels() {
560568
HttpRequest request = HttpRequest.newBuilder()
561569
.uri(URI.create(Preferences.olog_url + "/levels"))
562570
.GET()

‎app/save-and-restore/app/src/main/java/org/phoebus/applications/saveandrestore/authentication/SaveAndRestoreAuthenticationProvider.java‎

Lines changed: 1 addition & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -21,45 +21,23 @@
2121

2222
import org.phoebus.applications.saveandrestore.ui.SaveAndRestoreService;
2323
import org.phoebus.security.authorization.AuthenticationStatus;
24-
import org.phoebus.security.authorization.ServiceAuthenticationException;
2524
import org.phoebus.security.authorization.ServiceAuthenticationProvider;
2625
import org.phoebus.security.tokens.AuthenticationScope;
2726

28-
import java.net.ConnectException;
29-
import java.util.logging.Level;
30-
import java.util.logging.Logger;
31-
3227
/**
3328
* Authentication provider for the save-and-restore service.
3429
*/
3530
public class SaveAndRestoreAuthenticationProvider implements ServiceAuthenticationProvider {
3631

3732
private final AuthenticationScope saveAndRestoreAuthenticationScope;
38-
private final Logger logger = Logger.getLogger(SaveAndRestoreAuthenticationProvider.class.getName());
3933

4034
public SaveAndRestoreAuthenticationProvider() {
4135
saveAndRestoreAuthenticationScope = new SaveAndRestoreAuthenticationScope();
4236
}
4337

4438
@Override
4539
public AuthenticationStatus authenticate(String username, String password) {
46-
SaveAndRestoreService saveAndRestoreService = SaveAndRestoreService.getInstance();
47-
try {
48-
saveAndRestoreService.authenticate(username, password);
49-
logger.log(Level.INFO, "User " + username + " successfully signed in");
50-
return AuthenticationStatus.AUTHENTICATED;
51-
} catch (ConnectException e) {
52-
logger.log(Level.WARNING, "Unable to connect to save&restore service");
53-
return AuthenticationStatus.SERVICE_OFFLINE;
54-
} catch (ServiceAuthenticationException e) {
55-
logger.log(Level.WARNING, "User " + username + " not authenticated");
56-
return AuthenticationStatus.BAD_CREDENTIALS;
57-
} catch (Exception e) {
58-
// NOTE!!! Exception message and/or stack trace could contain request URL and consequently
59-
// user's password, so do not log or propagate it.
60-
logger.log(Level.WARNING, "Failed to authenticate user " + username + " with save&restore service, reason unknown");
61-
return AuthenticationStatus.UNKNOWN_ERROR;
62-
}
40+
return SaveAndRestoreService.getInstance().authenticate(username, password);
6341
}
6442

6543
@Override

‎app/save-and-restore/app/src/main/java/org/phoebus/applications/saveandrestore/client/SaveAndRestoreClient.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import org.phoebus.applications.saveandrestore.model.TagData;
3333
import org.phoebus.applications.saveandrestore.model.search.Filter;
3434
import org.phoebus.applications.saveandrestore.model.search.SearchResult;
35+
import org.phoebus.security.authorization.AuthenticationStatus;
3536

3637
import javax.ws.rs.core.MultivaluedMap;
3738
import java.util.List;
@@ -247,9 +248,9 @@ public interface SaveAndRestoreClient {
247248
*
248249
* @param userName User's account name
249250
* @param password User's password
250-
* @throws Exception e.g. if service is off-line or if user credentials are rejected.
251+
* @return An {@link AuthenticationStatus} to indicate the outcome of the login attempt.
251252
*/
252-
void authenticate(String userName, String password) throws Exception;
253+
AuthenticationStatus authenticate(String userName, String password);
253254

254255
/**
255256
* Requests service to restore the specified {@link SnapshotItem}s

‎app/save-and-restore/app/src/main/java/org/phoebus/applications/saveandrestore/client/SaveAndRestoreClientImpl.java‎

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,13 @@
2626
import org.phoebus.applications.saveandrestore.model.TagData;
2727
import org.phoebus.applications.saveandrestore.model.search.Filter;
2828
import org.phoebus.applications.saveandrestore.model.search.SearchResult;
29-
import org.phoebus.security.authorization.ServiceAuthenticationException;
29+
import org.phoebus.security.authorization.AuthenticationStatus;
3030
import org.phoebus.security.store.SecureStore;
3131
import org.phoebus.security.tokens.ScopedAuthenticationToken;
3232
import org.phoebus.util.http.QueryParamsHelper;
3333

3434
import javax.ws.rs.core.MultivaluedMap;
35+
import java.net.ConnectException;
3536
import java.net.CookieHandler;
3637
import java.net.CookieManager;
3738
import java.net.CookiePolicy;
@@ -576,18 +577,28 @@ public List<Node> deleteTag(TagData tagData) {
576577
* @return {@inheritDoc}
577578
*/
578579
@Override
579-
public void authenticate(String userName, String password) throws Exception {
580+
public AuthenticationStatus authenticate(String userName, String password) {
580581
String stringBuilder = Preferences.jmasarServiceUrl +
581582
"/login";
582-
HttpRequest request = HttpRequest.newBuilder()
583-
.uri(URI.create(stringBuilder))
584-
.header("Content-Type", CONTENT_TYPE_JSON)
585-
.POST(HttpRequest.BodyPublishers.ofString(OBJECT_MAPPER.writeValueAsString(new LoginCredentials(userName, password))))
586-
.build();
587-
HttpResponse<String> response = CLIENT.send(request, HttpResponse.BodyHandlers.ofString());
588-
if (response.statusCode() == 401) {
589-
throw new ServiceAuthenticationException("User not authenticated with save&restore service");
583+
try {
584+
HttpRequest request = HttpRequest.newBuilder()
585+
.uri(URI.create(stringBuilder))
586+
.header("Content-Type", CONTENT_TYPE_JSON)
587+
.POST(HttpRequest.BodyPublishers.ofString(OBJECT_MAPPER.writeValueAsString(new LoginCredentials(userName, password))))
588+
.build();
589+
HttpResponse<String> response = CLIENT.send(request, HttpResponse.BodyHandlers.ofString());
590+
if (response.statusCode() == 401) {
591+
LOGGER.log(Level.WARNING, "User not authenticated with save&restore service");
592+
return AuthenticationStatus.BAD_CREDENTIALS;
593+
}
594+
} catch (ConnectException e) {
595+
LOGGER.log(Level.WARNING, "Cannot connect to save&restore service");
596+
return AuthenticationStatus.SERVICE_OFFLINE;
597+
} catch (Exception e) {
598+
LOGGER.log(Level.WARNING, "Unable to send authentication request to service, reason unknown");
599+
return AuthenticationStatus.UNKNOWN_ERROR;
590600
}
601+
return AuthenticationStatus.AUTHENTICATED;
591602
}
592603

593604
/**

‎app/save-and-restore/app/src/main/java/org/phoebus/applications/saveandrestore/ui/SaveAndRestoreService.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
import org.phoebus.pv.PV;
4040
import org.phoebus.pv.PVPool;
4141
import org.phoebus.saveandrestore.util.VNoData;
42+
import org.phoebus.security.authorization.AuthenticationStatus;
4243
import org.phoebus.util.time.TimestampFormats;
4344

4445
import javax.ws.rs.core.MultivaluedMap;
@@ -319,10 +320,10 @@ public List<Node> deleteTag(TagData tagData) throws Exception {
319320
*
320321
* @param userName User's account name
321322
* @param password User's password
322-
* @throws Exception if authentication fails, e.g. service off-line or invalid credentials
323+
* @return An {@link AuthenticationStatus} to indicate the outcome of the login attempt.
323324
*/
324-
public void authenticate(String userName, String password) throws Exception {
325-
saveAndRestoreClient.authenticate(userName, password);
325+
public AuthenticationStatus authenticate(String userName, String password) {
326+
return saveAndRestoreClient.authenticate(userName, password);
326327
}
327328

328329
/**

0 commit comments

Comments
 (0)