-
Notifications
You must be signed in to change notification settings - Fork 66
Added authorization operations #146
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
Changes from all commits
e1b75bf
7c57ba0
d50233c
4b25c4d
7ce63e7
1e109e8
45cbda3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,4 +4,6 @@ | |
target | ||
bin | ||
.metadata | ||
**/integrationTest.properties | ||
.idea | ||
*.iml | ||
**/integrationTest.properties |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
/******************************************************************************* | ||
* Copyright (c) 2014 Red Hat, Inc. | ||
* Distributed under license by Red Hat, Inc. All rights reserved. | ||
* This program is made available under the terms of the | ||
* Eclipse Public License v1.0 which accompanies this distribution, | ||
* and is available at http://www.eclipse.org/legal/epl-v10.html | ||
* | ||
* Contributors: | ||
* Sean Kavanagh - initial API and implementation | ||
******************************************************************************/ | ||
package com.openshift.client; | ||
|
||
/** | ||
* Operations to manage and view authorization resources | ||
* | ||
* @link http://openshift.github.io/documentation/rest_api/rest-api-1-6.html#authorization | ||
*/ | ||
public interface IAuthorization extends IOpenShiftResource { | ||
|
||
|
||
/** | ||
* authorization id | ||
* | ||
* @return | ||
*/ | ||
public String getId(); | ||
|
||
/** | ||
* authorization note | ||
* | ||
* @return | ||
*/ | ||
public String getNote(); | ||
|
||
|
||
/** | ||
* authorization scopes | ||
* | ||
* @return | ||
*/ | ||
public String getScopes(); | ||
|
||
/** | ||
* authorization token | ||
* | ||
* @return | ||
*/ | ||
public String getToken(); | ||
|
||
|
||
/** | ||
* Destroys this authorization | ||
* | ||
* @throws OpenShiftException | ||
*/ | ||
public void destroy() throws OpenShiftException; | ||
|
||
/** | ||
* Refresh the authorization but reloading its content from OpenShift. | ||
* | ||
* @throws OpenShiftException | ||
*/ | ||
public void refresh() throws OpenShiftException; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,40 @@ public interface IUser extends IOpenShiftResource { | |
|
||
public List<IOpenShiftSSHKey> getSSHKeys() throws OpenShiftException; | ||
|
||
/** | ||
* Returns current authorization. Creates new authorization for user if none exists. | ||
* Authorization is set by default when token is used to create API connection. | ||
* | ||
* @return authorization | ||
* @throws OpenShiftException | ||
*/ | ||
public IAuthorization getAuthorization() throws OpenShiftException; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's another point imho: we should not rely on a single token, we should lookup the backend to see if we have pre-existing authorizations. Currently we only lookup by token but there might be other ones. Agree? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. Ok, I can make LIST_AUTHORIZATIONS apart of this PR then! public Collection getAuthorizations(); Expired authorizations shouldn't be returned from the LIST_AUTHORIZATIONS operation. http://openshift.github.io/documentation/rest_api/rest-api-1-6.html#authorization There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cool :) wait a sec, I already added expires_in in IAuthorization and added integration tests. The merge build is currently running at: #151 I suggest that you can added the above on top in a new PR once you got the new HEAD? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So in my mind an authorization is more to give a particular application permission to use the APIs. This is how I'm currently using it...
so I'm saving the authorization token to the session in order to obtain a connection later in the application without having to login. I guess I could write the above to get a list of existing authorization that is associated with the account, pick one, and persist it to the session. .. but then I would feel like my application is piggybacking off of some other application's authorization. I think a desktop application that integrates with OpenShift could work the same way. Think if you had an eclipse plugin that uses the APIs and didn't want the user to login every time eclipse launches. You would login once, save the token to a configuration file, so the next time eclipse launches you can get the connection without a username and password. I think it would be a little devious for the plugin to use an authorization token that is set for some other application. .. but on the other hand, I could see someone who has written an app that just manages authorizations (just like ssh keys) so I think there is a need for having the LIST_AUTHORIZATIONS operations. Thanks for your patience with this stuff!! You guys have been great!! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you're more than welcome! thanks for all the work :) Your code excerpt is enlightening, helps understanding. APIResource:
protected AuthorizationResource getOrCreateAuthorization(String token) { My concern here is what happens if the token is given but it's outdated? I did not test it but I think that we run into an exception (404?). I'm not convinced, it could very much be that the initial connection would fail anyhow. Maybe it shows that we really should do an integration test covering this scenario. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, I'll work on the get and list this weekend!
That token should always be the one used to create the connection. It is so if I have
then
is the authorization that corresponds to myToken. And when you authenticate via a token you will only have access to that token's authorization anyways. so you wouldn't be able to list or get any other authorization. curl -k https://openshift.redhat.com/broker/rest/user/authorizations -H "Authorization: Bearer 9e01b053026036a40f14b22f53a2e86447703533cd1b86db9d2e8546b519e7da" messages":[{"exit_code":1,"field":null,"index":null,"severity":"error","text":"This action is not allowed with your current authorization."}],"status":"forbidden" and that is b/c you wouldn't want an authorization with read access to use the api's to obtain an authorization with write access. so the new methods for getting and listing authorizations
should only return something when you call them with a connection that is created with a username and password I wouldn't be opposed to dropping the creation behind the scenes though and if it returns null, you can do a createAuthorization. so change it to something like this
It would probability be more straight forward that way anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ok I got it.. Sorry!! Your saying what if in the time between making the initial connection with the token and doing some other operation the token expires. I'll write a unit test for that. And we probably need to unit test the "forbidden" stuff too when we add the list and get operations. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No problem at all :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok got it, that was my 2nd thought as outlined above, yeah I thought most likely you couldnt get into the situation of a 404 when we do a show_auth request.
|
||
|
||
/** | ||
* Creates and returns new authorization set for user | ||
* | ||
* @param note | ||
* A reminder description of what the authorization is for. | ||
* @param scopes | ||
* Scope of the authorization token to determine type of access. | ||
* @return authorization | ||
* @throws OpenShiftException | ||
*/ | ||
public IAuthorization createAuthorization(String note, String scopes) throws OpenShiftException; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a useful comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed skavanagh@7c57ba0 |
||
|
||
/** | ||
* Creates and returns new authorization set for user | ||
* @param note | ||
* A reminder description of what the authorization is for. | ||
* @param scopes | ||
* Scope of the authorization token to determine type of access. | ||
* @param expiresIn | ||
* The number of seconds before this authorization expires. | ||
* @return authorization | ||
* @throws OpenShiftException | ||
*/ | ||
public IAuthorization createAuthorization(String note, String scopes, int expiresIn) throws OpenShiftException; | ||
|
||
/** | ||
* Deprecated, use {@link #addSSHKey(String, ISSHPublicKey)} | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,12 +107,16 @@ public IOpenShiftConnection getConnection(final String clientId, final String us | |
|
||
public IOpenShiftConnection getConnection(final String clientId, final String username, final String password, | ||
final String serverUrl, ISSLCertificateCallback sslCallback) throws OpenShiftException { | ||
return getConnection(clientId, username, password, null, null, serverUrl, sslCallback); | ||
return getConnection(clientId, username, password, null, null, null, serverUrl, sslCallback); | ||
} | ||
public IOpenShiftConnection getConnection(final String clientId, final String token, | ||
final String serverUrl, ISSLCertificateCallback sslCallback) throws OpenShiftException { | ||
return getConnection(clientId, null, null, null, null, token, serverUrl, sslCallback); | ||
} | ||
|
||
public IOpenShiftConnection getConnection(final String clientId, final String username, final String password, | ||
final String authKey, final String authIV, final String serverUrl) throws OpenShiftException { | ||
return getConnection(clientId, username, password, null, null, serverUrl, null); | ||
return getConnection(clientId, username, password, null, null, null, serverUrl, null); | ||
} | ||
|
||
/** | ||
|
@@ -125,6 +129,8 @@ public IOpenShiftConnection getConnection(final String clientId, final String us | |
* user's login. | ||
* @param password | ||
* user's password. | ||
* @param token | ||
* authorization token. | ||
* @param serverUrl | ||
* the server url. | ||
* @return a valid connection | ||
|
@@ -133,7 +139,7 @@ public IOpenShiftConnection getConnection(final String clientId, final String us | |
* @throws OpenShiftException | ||
*/ | ||
public IOpenShiftConnection getConnection(final String clientId, final String username, final String password, | ||
final String authKey, final String authIV, final String serverUrl, | ||
final String authKey, final String authIV, final String token, final String serverUrl, | ||
final ISSLCertificateCallback sslCertificateCallback) throws OpenShiftException { | ||
if (configuration == null) { | ||
try { | ||
|
@@ -144,31 +150,77 @@ public IOpenShiftConnection getConnection(final String clientId, final String us | |
} | ||
|
||
Assert.notNull(clientId); | ||
Assert.notNull(username); | ||
Assert.notNull(password); | ||
if (token == null || token.trim().length() == 0) { | ||
Assert.notNull(username); | ||
Assert.notNull(password); | ||
} | ||
Assert.notNull(serverUrl); | ||
|
||
try { | ||
IHttpClient httpClient = | ||
new UrlConnectionHttpClientBuilder() | ||
.setCredentials(username, password, authKey, authIV) | ||
.setCredentials(username, password, authKey, authIV, token) | ||
.setSSLCertificateCallback(sslCertificateCallback) | ||
.setConfigTimeout(configuration.getTimeout()) | ||
.client(); | ||
return getConnection(clientId, username, password, serverUrl, httpClient); | ||
return getConnection(clientId, username, password, token, serverUrl, httpClient); | ||
} catch (IOException e) { | ||
throw new OpenShiftException(e, "Failed to establish connection for user ''{0}}''", username); | ||
} | ||
} | ||
|
||
protected IOpenShiftConnection getConnection(final String clientId, final String username, final String password, | ||
protected IOpenShiftConnection getConnection(final String clientId, final String username, final String password, final String token, | ||
final String serverUrl, IHttpClient httpClient) throws OpenShiftException, IOException { | ||
Assert.notNull(clientId); | ||
Assert.notNull(serverUrl); | ||
Assert.notNull(httpClient); | ||
|
||
IRestService service = new RestService(serverUrl, clientId, new JsonMediaType(), | ||
IHttpClient.MEDIATYPE_APPLICATION_JSON, new OpenShiftJsonDTOFactory(), httpClient); | ||
return getConnection(service, username, password); | ||
return getConnection(service, username, password, token); | ||
} | ||
|
||
/** | ||
* Establish a connection with the clientId along with a user's authorization token | ||
* | ||
* @param clientId | ||
* http client id | ||
* @param token | ||
* authorization token. | ||
* @param serverUrl | ||
* the server url. | ||
* @return a valid connection | ||
* @throws FileNotFoundException | ||
* @throws IOException | ||
* @throws OpenShiftException | ||
*/ | ||
public IOpenShiftConnection getAuthTokenConnection(final String clientId,final String token, final String serverUrl) throws OpenShiftException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be overloaded by renaming to getConnection with the necessary arguments There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was hoping to be able to overload, but there is already a method there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @skavanagh @adietish Not really your issue, but this might be an excellent place to introduce a strategy of some kind to allow the use of a singular name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if you made serverUrl an actual java.net.URL?? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that is reasonable, but I'm certain that would break backwards compatibility. @adietish would have more in site as to what our restrictions are. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you go the serverUrl java.net.URL route, could we do something like this...
and say that the future release will only support token. ..but for now change it so that if the connection is null with a token, try it as a username and password. It's a lot of changes, but I think that could be doable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasnt very picky about compatibility so far to be honest, maybe I should have been ;) I dont think that turning the default to token authentication is a good thing. The new user (which maybe does a simple cmd-line?) would imho choose the simplest possible way which is via username/password and would not know how to get the token at first sight. If I understand it right you need a first connection which will deliver you (the User will actually) the token that you can then user in another connection: Why not do the overload using the Authorization which would help the user that would discover by using/inspecting API? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current impl of OpenShiftConnectionFactory has far too many overloads anyhow. I think we should turn to a builder pattern on the medium/longer run. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having it as a String is nice b/c after the token is created you can persist it easily in a user session, session cookie, datastore or whatever. What if we did something like this for now until you can refactor it? I got rid of the getAuthTokenConnection methods and added..
|
||
return getConnection(clientId, null, null, null, null, token, serverUrl, null); | ||
} | ||
/** | ||
* Establish a connection with the clientId along with a user's authorization | ||
* token. Server URL is retrieved from the local configuration file (in | ||
* see $USER_HOME/.openshift/express.conf) | ||
* | ||
* @param clientId | ||
* http client id | ||
* @param token | ||
* authorization token. | ||
* @return a valid connection | ||
* @throws FileNotFoundException | ||
* @throws IOException | ||
* @throws OpenShiftException | ||
*/ | ||
public IOpenShiftConnection getAuthTokenConnection(final String clientId, final String token) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here regarding overloading There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Already a method that beat me to it with this one as well There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As suggested above: what about using URI to overload? |
||
throws OpenShiftException { | ||
try { | ||
configuration = new OpenShiftConfiguration(); | ||
} catch (IOException e) { | ||
throw new OpenShiftException(e, "Failed to load OpenShift configuration file."); | ||
} | ||
return getConnection(clientId, null, null, null, null, token, configuration.getLibraServer(), null); | ||
} | ||
|
||
|
||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ | |
import com.openshift.client.IOpenShiftConnection; | ||
import com.openshift.client.IQuickstart; | ||
import com.openshift.client.IUser; | ||
import com.openshift.client.IAuthorization; | ||
import com.openshift.client.OpenShiftException; | ||
import com.openshift.client.cartridge.EmbeddableCartridge; | ||
import com.openshift.client.cartridge.ICartridge; | ||
|
@@ -37,6 +38,7 @@ | |
import com.openshift.internal.client.response.QuickstartDTO; | ||
import com.openshift.internal.client.response.QuickstartJsonDTOFactory; | ||
import com.openshift.internal.client.response.UserResourceDTO; | ||
import com.openshift.internal.client.response.AuthorizationResourceDTO; | ||
import com.openshift.internal.client.utils.Assert; | ||
import com.openshift.internal.client.utils.CollectionUtils; | ||
import com.openshift.internal.client.utils.IOpenShiftJsonConstants; | ||
|
@@ -53,20 +55,32 @@ public class APIResource extends AbstractOpenShiftResource implements IOpenShift | |
|
||
private final String login; | ||
private final String password; | ||
private final String token; | ||
private UserResource user; | ||
private AuthorizationResource authorization; | ||
//TODO: implement switch that allows to turn ssl checks on/off | ||
private boolean doSSLChecks = false; | ||
private List<IDomain> domains; | ||
private List<IStandaloneCartridge> standaloneCartridges; | ||
private List<IEmbeddableCartridge> embeddableCartridges; | ||
private Map<String, IQuickstart> quickstartsByName; | ||
private final ExecutorService executorService; | ||
|
||
protected APIResource(final String login, final String password, final IRestService service, | ||
|
||
protected APIResource(final String token, final IRestService service, | ||
final Map<String, Link> links) { | ||
super(service, links, null); | ||
this.login = null; | ||
this.password = null; | ||
this.token = token; | ||
this.executorService = Executors.newFixedThreadPool(10); | ||
} | ||
|
||
protected APIResource(final String login, final String password, final String token, final IRestService service, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overload the constructor such that it only takes a token but no username/password There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed skavanagh@d50233c |
||
final Map<String, Link> links) { | ||
super(service, links, null); | ||
this.login = login; | ||
this.password = password; | ||
this.token = token; | ||
this.executorService = Executors.newFixedThreadPool(10); | ||
} | ||
|
||
|
@@ -115,6 +129,38 @@ public IUser getUser() throws OpenShiftException { | |
} | ||
return this.user; | ||
} | ||
|
||
public IAuthorization createAuthorization(String note, String scopes) throws OpenShiftException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not familiar with the authorizations yet. Can you please enlight me (or point me to some doc): what are scopes for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's permissions for API calls. Under "Add new authorization" it has the scope description http://openshift.github.io/documentation/rest_api/rest-api-1-6.html#authorization and it can be any combination of those definitions. Also, "Valid options may be different on your actual implementation of OpenShift." |
||
if (authorization != null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it looks like we can only have 1 authorization at the time. Is this required or simply to make things simpler? |
||
authorization.destroy(); | ||
} | ||
this.authorization = new AuthorizationResource(this, new AddAuthorizationRequest().execute( | ||
new StringParameter(IOpenShiftJsonConstants.PROPERTY_NOTE, note), | ||
new StringParameter(IOpenShiftJsonConstants.PROPERTY_SCOPES, scopes))); | ||
return this.authorization; | ||
} | ||
|
||
public IAuthorization createAuthorization(String note, String scopes, int expiresIn) throws OpenShiftException { | ||
if (authorization != null) { | ||
authorization.destroy(); | ||
} | ||
this.authorization = new AuthorizationResource(this, new AddAuthorizationRequest().execute( | ||
new StringParameter(IOpenShiftJsonConstants.PROPERTY_NOTE, note), | ||
new StringParameter(IOpenShiftJsonConstants.PROPERTY_SCOPES, scopes), | ||
new StringParameter(IOpenShiftJsonConstants.PROPERTY_EXPIRES_IN, Integer.toString(expiresIn)))); | ||
return this.authorization; | ||
} | ||
|
||
public IAuthorization getAuthorization() throws OpenShiftException { | ||
if (authorization == null) { | ||
if(token == null) { | ||
this.authorization = new AuthorizationResource(this, new AddAuthorizationRequest().execute(new StringParameter(IOpenShiftJsonConstants.PROPERTY_SCOPES, IOpenShiftJsonConstants.PROPERTY_SESSION))); | ||
} else { | ||
this.authorization = new AuthorizationResource(this, new ShowAuthorizationRequest().execute(token)); | ||
} | ||
} | ||
return this.authorization; | ||
} | ||
|
||
@Override | ||
public List<IDomain> getDomains() throws OpenShiftException { | ||
|
@@ -346,4 +392,26 @@ protected List<QuickstartDTO> execute() throws OpenShiftException { | |
} | ||
} | ||
|
||
private class AddAuthorizationRequest extends ServiceRequest { | ||
|
||
private AddAuthorizationRequest() throws OpenShiftException { | ||
super("ADD_AUTHORIZATION"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. guessing by the link in the API resource one can have several authorizations at a time. Shouldnt we support/follow this pattern? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Authorizations are handled individually as far as CRUD goes. I think the only operation that returns multiple is LIST_AUTHORIZATIONS, which I didn't implement. Should we?? we could add something like this.. public Collection getAuthorizations(); then you could use getAuthorization to set the user authorization based one that is found in the collection |
||
} | ||
|
||
protected AuthorizationResourceDTO execute(Parameter... parameters) throws OpenShiftException { | ||
return super.execute(parameters); | ||
} | ||
} | ||
private class ShowAuthorizationRequest extends ServiceRequest { | ||
|
||
private ShowAuthorizationRequest() throws OpenShiftException { | ||
super("SHOW_AUTHORIZATION"); | ||
} | ||
|
||
protected AuthorizationResourceDTO execute(String id) throws OpenShiftException { | ||
List<Parameter> urlPathParameter = new Parameters().add("id", id).toList(); | ||
return (AuthorizationResourceDTO) super.execute(IHttpClient.NO_TIMEOUT, urlPathParameter, Collections.<Parameter>emptyList()); | ||
} | ||
} | ||
|
||
} |
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.
Add a comment identifying purpose. Consider adding an '@link' with ref to openshift.io documentation http://openshift.github.io/documentation/rest_api/rest-api-1-6.html#authorization
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.
Fixed skavanagh@7c57ba0