-
Notifications
You must be signed in to change notification settings - Fork 0
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
Enhancement/allow multiple users #24
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general, but there are a few things that probably can still be improved.
@@ -41,14 +41,14 @@ Not to be confused with the credentials the **Auth Server** used to authenticate | |||
|
|||
== Configuration | |||
|
|||
The user information and authorities returned by the service can be configured: | |||
The user information and authorities returned by the service are configured in the service config. Multiple users can eb configured as shown in the following example. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: eb
instead of be
|
||
[source,yaml] | ||
---- | ||
smartcosmos: | ||
security: | ||
user: | ||
name: user | ||
users[user]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it possible to do something like this?
users:
- username: user
password: password
...
- username: user2
password: password2
...
In Ansible (which also uses YML) this syntax works perfectly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes but that is ugly to me and is not a map, I want a map. I want to lookup by key not search a list with contains
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, that makes perfect sense!
|
||
@SpringBootApplication | ||
@EnableConfigurationProperties({ UserAuthenticationProperties.class }) | ||
@EnableAutoConfiguration | ||
@Import(UserDetailsDevelopmentConfiguration.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably don't need this if the @ComponentScan
can locate your @Configuration
class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been having problems with autowiring. I am trying to get tests going that do not require loading the entire works and all the web mvn components. It's still not correct
What changes were proposed in this pull request?
Allow the configuration of multiple users in the user details configuration.
How is this patch documented?
README.adoc, javadoc and configuration file example
How was this patch tested?
Postman and existing tests
Depends On
SMARTRACTECHNOLOGY/smartcosmos-cluster-config#23