-
Notifications
You must be signed in to change notification settings - Fork 17
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
Update users at runtime #196
Conversation
@@ -68,3 +68,5 @@ This defines two users: "bob" and "may". Bob has permission to create Windows, L | |||
Authentication is performed using standard Basic access authentication. The authentication data can be sent as an `Authorization` request header, but that is inconvenient when using the command line tools (bob.jar) or the Defold editor. The username and password can be sent as part of the build server URL set in the Preferences window of the editor and using the `--build-server` option to bob.jar: | |||
|
|||
java -jar bob.jar --build-server https://bob:super5ecret@myextender.com | |||
|
|||
It is also possible to specify a username and password in the environment variables DM_EXTENDER_USERNAME and DM_EXTENDER_PASSWORD. |
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.
This was added in a previous PR for those uncomfortable with adding the username and password in the URL itself. We forgot to add documentation for this though.
@@ -1,3 +1,7 @@ | |||
[submodule "server/scripts/shared"] | |||
path = server/scripts/shared | |||
url = git@github.com:defold/aws-build-tools.git | |||
|
|||
[submodule "server/scripts/task-definitions"] |
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.
The AWS task definitions are here. These now contain application.yml overrides with paths to resources we do not wish to share openly.
} | ||
return users; | ||
} | ||
private InMemoryUserDetailsManager userDetailsManager = new InMemoryUserDetailsManager(); |
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.
We start with an empty user details manager. It will get updated before the first build via the UserUpdateService.
auth.userDetailsService(userDetailsManager); | ||
} | ||
|
||
@Bean |
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.
Expose the user details manager as a Spring Bean so that we can access it from the UserUpdateService
|
||
private long lastUpdateTimestamp = 0; | ||
|
||
private Properties loadUsers() { |
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.
Load the users from the user resource (file or url)
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.
Good comment that could be in the code :)
may = top5ecret,ROLE_MACOS,enabled | ||
extender: | ||
authentication: | ||
users: https://www.mysite.com/extender-users |
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.
Perhaps make it clearer that it expects a text file?
In our task definition we use an explicit url to a users.txt.
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.
Well it doesn't actually have to be a file. It does an HTTP GET so anything that serves a response in the correct format is fine. It could be a REST API that grabs the users from a DB.
This can perhaps be clarified though.
The same by passing an environment variable when launching Docker: | ||
|
||
``` | ||
docker run ... -e extender.authentication.users=https://www.mysite.com/extender-users extender/extender; |
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 don't see a change for run-local.sh?
How does this work with the current workflow? (I.e. when I build for Switch)
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'll move the extender.authentication.platforms config to the task definition.
|
||
private long lastUpdateTimestamp = 0; | ||
|
||
private Properties loadUsers() { |
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.
Good comment that could be in the code :)
List<String> userSettings = Arrays.asList(users.getProperty(username).split(",")); | ||
final String password = userSettings.get(0); | ||
final boolean disabled = userSettings.get(userSettings.size() - 1).equals("disabled"); | ||
final String[] authorities = userSettings.subList(0, userSettings.size() - 1).toArray(new String[0]); |
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.
Doesn't this add the password too?
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.
What do you mean by this? Yes, we update/create users with a set of authorities/roles and a password.
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.
My question is, is "authorities[0] == password" ?
Or am I reading it wrong?
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.
Oh, good catch! Yes indeed, the password was included as one of the authorities.
return users; | ||
} | ||
|
||
// user = password,ROLE_ANDROID,ROLE_IOS,ROLE_HTML5,ROLE_WINDOWS,ROLE_LINUX,ROLE_MACOS,ROLE_SWITCH,disabled |
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.
Is this a "standard" format?
If we later need to add/remove info to this (not sure what, but let's say email), will it be easy to do? Will it be easy to remove fields?
Not sure we need to, just making sure the format is as solid we can make it/want it.
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.
Yeah, so the InMemoryUserDetailsManager accepts a Properties instance as one of its constructors:
I can't seem to find the example of how it is used anymore.
BUT since we aren't creating the InMemoryUserDetailsManager with a Properties instance in the constructor we're actually free to change the format if we like.
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.
👍
It's good that we can change it.
Workflow question: Will it be easy?
I'm thinking of the update workflow here.
Let's say we update the parsing function, since we want to add/remove a field.
We then need to build stage server, and point it to a separate users.txt.
During these two weeks (the beta period), we need to update two separate users.txt to keep them working for our users.
Then, after two weeks, we can merge the stage/users.txt to the prod/users.txt.
I'm not going to push for any changes at this point though.since this is yet theoretical and not an actual use case yet.
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.
Yeah, I get what you are saying. What we do know is that password comes first, then a list of authorities/roles all prefixed with ROLE_ and finally a value indicating if the user is enabled or not. We're not locked into a format which we cannot change at this moment, but I agree that it makes sense to pick a more extensible format.
server/src/main/java/com/defold/extender/services/UserUpdateService.java
Outdated
Show resolved
Hide resolved
List<String> userSettings = Arrays.asList(users.getProperty(username).split(",")); | ||
final String password = userSettings.get(0); | ||
final boolean disabled = userSettings.get(userSettings.size() - 1).equals("disabled"); | ||
final String[] authorities = userSettings.subList(0, userSettings.size() - 1).toArray(new String[0]); |
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.
My question is, is "authorities[0] == password" ?
Or am I reading it wrong?
return users; | ||
} | ||
|
||
// user = password,ROLE_ANDROID,ROLE_IOS,ROLE_HTML5,ROLE_WINDOWS,ROLE_LINUX,ROLE_MACOS,ROLE_SWITCH,disabled |
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.
👍
It's good that we can change it.
Workflow question: Will it be easy?
I'm thinking of the update workflow here.
Let's say we update the parsing function, since we want to add/remove a field.
We then need to build stage server, and point it to a separate users.txt.
During these two weeks (the beta period), we need to update two separate users.txt to keep them working for our users.
Then, after two weeks, we can merge the stage/users.txt to the prod/users.txt.
I'm not going to push for any changes at this point though.since this is yet theoretical and not an actual use case yet.
No description provided.