Openconnect #35
Openconnect #35
Conversation
|
Looks ok to me, I've left a few comments about code style and error handling. |
| if(values.containsKey(attribute)){ | ||
| return (String) values.get(attribute); | ||
| } else { | ||
| logger.error("Missing attribute " + attribute + " for user " + | ||
| values.entrySet().stream().map(e-> e.getKey() +":"+e.getValue()) | ||
| .collect(Collectors.joining(";"))); | ||
|
|
||
| throw new RuntimeException("Missing attribute " + attribute + " for user "); | ||
| } |
There was a problem hiding this comment.
| if(values.containsKey(attribute)){ | |
| return (String) values.get(attribute); | |
| } else { | |
| logger.error("Missing attribute " + attribute + " for user " + | |
| values.entrySet().stream().map(e-> e.getKey() +":"+e.getValue()) | |
| .collect(Collectors.joining(";"))); | |
| throw new RuntimeException("Missing attribute " + attribute + " for user "); | |
| } | |
| var value = (String) values.get(attribute); | |
| if (value == null) { | |
| logger.error("Missing attribute {} for user {}", attribute, values); | |
| throw new RuntimeException("Missing attribute " + attribute + " for user"); | |
| } | |
| return value; |
There was a problem hiding this comment.
I removed the log altogether because there already is a log after.
not particularly a fan of extracting a variable and checking directly for null but if you really want I can do it.
| usersRecord | ||
| ); | ||
|
|
||
| } catch( Exception e){ |
There was a problem hiding this comment.
don't try/catch the exception, you already logged everything required in getMandatory
There was a problem hiding this comment.
there is other potential errors, I removed the log from getMandatory to not have a duplication of logs.
There was a problem hiding this comment.
There is indeed other potential error. Spring will handle all of them just fine, leave exception handling to the framework, it will log what's required and display to the user what's required.
|
|
||
| @Service | ||
| @RequiredArgsConstructor | ||
| public class SecurityUserService extends DefaultOAuth2UserService { | ||
|
|
||
| private final UserRepository userRepository; | ||
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(SecurityUserService.class); | ||
|
|
||
| String getMandatory(Map<String, Object> values, String attribute){ |
There was a problem hiding this comment.
| String getMandatory(Map<String, Object> values, String attribute){ | |
| private static String getMandatory(Map<String, Object> values, String attribute) { |
|
|
||
| @Service | ||
| @RequiredArgsConstructor | ||
| public class SecurityUserService extends DefaultOAuth2UserService { | ||
|
|
||
| private final UserRepository userRepository; | ||
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(SecurityUserService.class); |
There was a problem hiding this comment.
We use lombok to create loggers so there is no way to miss copy/pasta errors.
Example:
src/main/resources/application.yml
Outdated
|
|
||
|
|
||
| server: | ||
| forward-headers-strategy: framework |
There was a problem hiding this comment.
should use native unless we have a specific need, also, we already have server values at the top of the file, please combine them.
There was a problem hiding this comment.
combined. I have no way of testing native because I don't know what the tomcat config is in production.
src/main/resources/application.yml
Outdated
| token-uri: "https://members.lghs.be/oauth/token" | ||
| user-info-uri: "https://members.lghs.be/api/me" | ||
| client-id: "accounting" | ||
| client-secret: "f539b20e-048a-4d81-90bf-238477fb3ad6" |
There was a problem hiding this comment.
Please never include secrets, even fake ones in configs. You never know who will copy paste and it makes debugging harder.
If you need those secrets, drop an application.yml in your work folder and gitignore it (I personally always run project with a CWD outside the git repository to avoid committing secrets by mistake).
There was a problem hiding this comment.
I'm not fan of having additional configuration to do before launching a development environment. There is a lot of step before being able to launch the app and this can only introduce error between dev-env/prod or between two developer.
The other MR aims to have a init script, so that you can launch the init script, then docker-compose up and the dev-env is running without any other step (maybe add a line in the host file because I'm not fan of having root privileges to the scripts).
That being said, I understand the issue with having them in the app config file. I replaced them with env variables and defined them directly in the docker-compose.yaml file
|
This will require a change of the production config that's stored here: https://github.com/LgHS/infra/blob/main/playbooks/accounting.lghs.be/templates/application.yml.j2 |
frol2103
force-pushed
the
openconnect
branch
3 times, most recently
from
4dccacb
to
981a6e1
Compare
|
Ok thanks. We are thinking to do another quick change to allow to use historical uuid from the keycloak. I'll do another merge request for that. |
modification of app and configuration to use openconnect.
Have been tested with lghs keycloak.
Will be easier to read when the 2 other PR have been merged