[AC-2933] [Fix] Support for bitwarden.eu cloud
#57
Conversation
- Parse `serverUrl` as an `URL` - Use `serverUrl.host` as host value - Support `bitwarden.com` and `bitwarden.eu` as cloud instances
|
No New Or Fixed Issues Found |
|
Thanks for the contribution! This seems straight forward to me. I'm passing it over to our QA team and we'll get it merged once that has passed. |
|
Internal tracking link: https://bitwarden.atlassian.net/browse/AC-2933 |
|
|
There was a problem hiding this comment.
Thank you @jpclipffel for this addition!
I have raised few issues with your change, please have a look.
| const apiUrl = isBitwardenCloud ? `https://api.${serverUrl.host}` : serverUrl + "/api/"; | ||
| const identityUrl = isBitwardenCloud ? `https://identity.${serverUrl.host}` : serverUrl + "/identity/"; |
There was a problem hiding this comment.
A pathname for self-hosted server url is optional.
This might result in double forward slash character //, example server url https://HOST would result in https://HOST//api, which python app does not normalize and is technically different href (i.e. https://api.bitwarden.com/public/events works but https://api.bitwarden.com//public/events return 404)
This happens because serverUrl is URL type, which concatenated always have the forward slash character / at the beginning.
For record, this was wrong before, since we did not normalize the paths, but now it's even worse when pathname is not provided.
A quick fix would be to change the serverUrl back to rawServerUrl, but i think proper fix would be to:
| const apiUrl = isBitwardenCloud ? `https://api.${serverUrl.host}` : serverUrl + "/api/"; | |
| const identityUrl = isBitwardenCloud ? `https://identity.${serverUrl.host}` : serverUrl + "/identity/"; | |
| const selfHostedServerUrl = serverUrl.origin + (serverUrl.pathname === "/" ? '' : serverUrl.pathname); | |
| const apiUrl = isBitwardenCloud ? `https://api.${serverUrl.host}` : selfHostedServerUrl + "/api/"; | |
| const identityUrl = isBitwardenCloud ? `https://identity.${serverUrl.host}` : selfHostedServerUrl + "/identity/"; |
| let { clientId, clientSecret, index, rawServerUrl, startDate, ...properties } = setup_options; | ||
|
|
||
| // Parse server URL | ||
| const serverUrl = new URL(rawServerUrl); |
There was a problem hiding this comment.
This is a regression, URL requires the protocol to be present, either http: or https: and throws exception when none is provided, where it was working fine before (we added it in python app backend when missing).
| const serverUrl = new URL(rawServerUrl); | |
| const serverUrlContainsProtocol = rawServerUrl.startsWith("http://") || rawServerUrl.startsWith("https://"); | |
| const serverUrl = new URL(serverUrlContainsProtocol ? rawServerUrl : "https://" + rawServerUrl); |
eliykat
changed the title
bitwarden.eu cloudbitwarden.eu cloud
|
@jpclipffel Would you be able to resolve the comments on this PR? If not, I'd like to close it. We have our own backlog item to address this, but it hasn't bubbled to the top yet and I'm not sure exactly when it will. |
🎟️ Tracking
#56
📔 Objective
Support for non-US cloud instances (e.g.
bitwarden.eu).Details:
serverUrlas anURLserverUrl.hostas host valuebitwarden.comandbitwarden.euas cloud instances⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes