-
-
Notifications
You must be signed in to change notification settings - Fork 1
Add 2 environments variables to set the EU servers URLs #65
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
Conversation
0a08b69 to
e4e13cb
Compare
…n.eu & identiy.bitwarden.eu)
e4e13cb to
1fe6832
Compare
|
Thanks for your contribution @stache3000. What are these URLs? Is there some documentation available for these? |
Yes, you can consult the Bitwarden API docs here : https://bitwarden.com/help/public-api/ Regards, |
Fine. You are right, we could make this configurable, eg. allow to choose between |
purejava
left a comment
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 two endpoints are currently already defined by apiUrl and identityUrl.
To make these configurable, you should:
- read these form the env, instead of creating new ones from the env
- check them with
isEnvVarValid - in case they are not valid, fall back to
https://api.bitwarden.comandhttps://identity.bitwarden.com - best way to do this is to use an enum with the possible 4 URL values for api and identity, com and eu for each
| if (isEnvVarValid(envApiUrl) && isEnvVarValid(envIdentityUrl)) { | ||
| this.apiUrl = envApiUrl; | ||
| this.identityUrl = envIdentityUrl; | ||
| } |
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 are introducing the two new variables, but after reading them from the env, you do nothing with them / they do not have any effect on the application.
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.
They are an effects on the application.
if these news vars are valide , we set "this.apiUrl" & "this.identityUrl" with these new values (.eu) ,
so "this.bitwardenSettings" use these in following try/catch to create the "BitwardenClient".
If these news vars are invalide , we do nothing (fall back to .com)
Many others solutions exist, i have no preferences.
Without new vars (to remove envApiUrl & envIdentityUrl) we can also do this :
if (isEnvVarValid(System.getenv("BITWARDEN_API_URL")) && isEnvVarValid(System.getenv("BITWARDEN_IDENTITY_URL"))) {
this.apiUrl = System.getenv("BITWARDEN_API_URL");
this.identityUrl = System.getenv("BITWARDEN_IDENTITY_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.
Ok, I overlooked, that this.apiUrl and this.identityUrl get replaced.
The final request remains: as these 4 URLs are fixed values, there is no need to check either of the two received env URLs whether it's empty or not. Better check them against the 4 possible values contained in an enum.
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.
That's, what I mean:
public class BitwardenAccess implements KeychainAccessProvider {
private static final Logger LOG = LoggerFactory.getLogger(BitwardenAccess.class);
private final BitwardenSettings bitwardenSettings = new BitwardenSettings();
private BitwardenClient bitwardenClient;
private final String accessToken;
private UUID organizationId = null;
private final String stateFile;
private boolean isSupported = false;
private final String boID;
private final String apiUrl;
private final String identityUrl;
private final String APP_NAME = "Cryptomator";
public BitwardenAccess() {
this.accessToken = System.getenv("BITWARDEN_ACCESS_TOKEN");
this.boID = System.getenv("BITWARDEN_ORGANIZATION_ID");
this.stateFile = System.getenv("BITWARDEN_STATE_FILE");
var envApiUrl = System.getenv("BITWARDEN_API_URL");
var envIdentityUrl = System.getenv("BITWARDEN_IDENTITY_URL");
var endpoint = resolveEndpoint(envApiUrl, envIdentityUrl);
this.apiUrl = endpoint.apiUrl;
this.identityUrl = endpoint.identityUrl;
if (isEnvVarValid(accessToken) && isEnvVarValid(boID)) {
try {
this.organizationId = UUID.fromString(boID);
this.bitwardenSettings.setApiUrl(apiUrl);
this.bitwardenSettings.setIdentityUrl(identityUrl);
this.bitwardenClient = new BitwardenClient(bitwardenSettings);
this.bitwardenClient.auth().loginAccessToken(accessToken, stateFile);
this.isSupported = true;
} catch (BitwardenClientException | IllegalArgumentException e) {
LOG.error(e.toString(), e.getCause());
}
}
}
private BitwardenEndpoint resolveEndpoint(String apiUrlEnv, String identityUrlEnv) {
if (isEnvVarValid(apiUrlEnv) && isEnvVarValid(identityUrlEnv)) {
for (BitwardenEndpoint endpoint : BitwardenEndpoint.values()) {
if (endpoint.apiUrl.equals(apiUrlEnv) && endpoint.identityUrl.equals(identityUrlEnv)) {
return endpoint;
}
}
LOG.warn("Provided API/Identity URLs are invalid. Falling back to default (US).");
} else {
LOG.info("API/Identity URLs not set. Falling back to default (US).");
}
return BitwardenEndpoint.US;
}
private boolean isEnvVarValid(String var) {
return var != null && !var.isEmpty() && !var.isBlank();
}
enum BitwardenEndpoint {
US("https://api.bitwarden.com", "https://identity.bitwarden.com"),
EU("https://api.bitwarden.eu", "https://identity.bitwarden.eu");
private final String apiUrl;
private final String identityUrl;
BitwardenEndpoint(String apiUrl, String identityUrl) {
this.apiUrl = apiUrl;
this.identityUrl = identityUrl;
}
}
}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'm ok with this implementation.
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.
Fine. Could you take over the code into your PR?
|
Thanks for bringing this up and your PR @stache3000. Your contribution is published with release 1.0.1. |
Add 2 environments variables to set the EU servers URLs (api.bitwarden.eu & identiy.bitwarden.eu)