Added authorization operations

[skavanagh]

Added authorization operations and ability to get connection by authorization token.

[skavanagh]

I cleaned up the formatting diffs on pull request. Sorry about that!

[adietish]

oh, pretty awesome, looks like a nice enhancement!
Please allow me a few more days for review it, I'm currently I'm currently code-freezing in JBoss Tools currently.

[skavanagh]

@adietish - Do you think the authorization should default to what is used to create a connection??

so for example if I did...

IOpenShiftConnection connection = new OpenShiftConnectionFactory().getAuthTokenConnection(IUser.ID, myAuthToken);
connection.getUser().getAuthorization().destroy();

it would remove the authorization I made the connection with. That is not how it works now. Currently it would create a new authorization and destroy that one. Which I don't think is ideal.

I think if I pass the token in the APIResource constructor

https://github.com/skavanagh/openshift-java-client/blob/master/src/main/java/com/openshift/internal/client/APIResource.java#L68

Then in the getAuthorization method I can get the authorization if the token isn't null and create a new one if it is.

Thanks!

[skavanagh]

I updated the PR to default the authorization to the one used to create the connection - Thanks!

[adietish]

I'm not familiar with the authorizations yet. Can you please enlight me (or point me to some doc): what are scopes for?

[skavanagh]

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."

[adietish]

yeah sounds like a nice addition!

[adietish]

Btw. I published 2.6.0.Final in the last few days so we're safe back in the 2.7.0-SNAPSHOT phase.
I suggest that we keep this focused on a first implementation, proceed to merging quickly and do further enhancements if needed. Works for you?

[skavanagh]

Merging quickly works for me! I added the expires to the create token operations

skavanagh@45cbda3

but I'll wait and do a separate PR for LIST_AUTHORIZATIONS
public Collection getAuthorizations();
public IAuthorization getAuthorization(String id);

[adietish]

works for me.

[adietish]

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?
Furthermore I'm wondering what's happening with expired authorizations. We currently rely on the token to know if the re's an authorization. What about the case where the authorization expired?

[skavanagh]

I agree. Ok, I can make LIST_AUTHORIZATIONS apart of this PR then!

public Collection getAuthorizations();
public IAuthorization getAuthorization(String id);

Expired authorizations shouldn't be returned from the LIST_AUTHORIZATIONS operation.
Also, "expires_in_seconds" looks beneficial to make apart of the authorization object too!!

http://openshift.github.io/documentation/rest_api/rest-api-1-6.html#authorization

[adietish]

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?

[skavanagh]

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...

LoginController.java >> 

IOpenShiftConnection connection = new OpenShiftConnectionFactory().getConnection(OpenShiftUtils.CLIENT_NAME, username, password);

IUser user=connection.getUser()

//this could be session cookie, save the token to the DB or whatever
request.getSession().setAttribute("token",  user.createAuthorization("Application XYZ's Authorization", "session", 60*60*2).getToken());
.....


GetSomeOpenShiftInfoController.java >>

IOpenShiftConnection connection = new OpenShiftConnectionFactory().getConnecton(OpenShiftUtils.CLIENT_NAME, request.getSession().getAttribute("token"));

//do some stuff
IDomain domain=connection.getUser().getDomain();
.....


LogoutController.java >>

IOpenShiftConnection connection = new OpenShiftConnectionFactory().getConnecton(OpenShiftUtils.CLIENT_NAME, request.getSession().getAttribute("token"));

//destroy authorization
IDomain domain=connection.getUser().getAuthorization().destroy();
.....

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!!

[adietish]

you're more than welcome! thanks for all the work :)

Your code excerpt is enlightening, helps understanding.
I think that we should not restrict functionality offered be the REST service. If it's offered we should have it too. In other words if it made sense to the developers of the REST interface to allow one to list and use any auth we should do that too.
My current most concern is the piece of code where we currently either create a new authorization or show an existing one:

APIResource:

public IAuthorization getAuthorization() throws OpenShiftException {
    if (authorization == null) {
        // TODO: if the given token is expired we get an exception here
        this.authorization = getOrCreateAuthorization(token);
    }
    return this.authorization;
}

protected AuthorizationResource getOrCreateAuthorization(String token) {
if (token == null) {
return createAuthorization(null, IOpenShiftJsonConstants.PROPERTY_SESSION, IAuthorization.NO_EXPIRES_IN);
} else {
return new AuthorizationResource(
this, new ShowAuthorizationRequest().execute(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.
I thought that we should get your great contribution in very quickly and mostly untouched API-wise so that you can switch your application back to the "official" build. Then we can go on and improve where required.

[skavanagh]

I agree, I'll work on the get and list this weekend!

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?).

That token should always be the one used to create the connection.
The getAuthorization() either creates a new authorization, returns an authorization that has already been created, or defaults it to the one used when creating the connection (we actually don't have a get with a token or id yet).

It is so if I have

IOpenShiftConnection connection = new OpenShiftConnectionFactory().getConnecton(OpenShiftUtils.CLIENT_NAME, myToken);

then

connection.getUser().getAuthorization()

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

public Collection getAuthorizations();
public IAuthorization getAuthorization(String id);

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

public IAuthorization getAuthorization() throws OpenShiftException {
    if (token != null) {
        this.authorization = new AuthorizationResource(this, new ShowAuthorizationRequest().execute(token));
    }
    return this.authorization;
}

It would probability be more straight forward that way anyway.

[skavanagh]

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.

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. :)

[adietish]

No problem at all :)
I already added an integration test, fully sufficient to add the discussed tests to that class: AuthorizationIntegrationTest

[adietish]

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.
Just thinking loud: Looks like separating the 3 available authorization schemes into separate strategies (behind the scenes) would help to get the logic(s) more straight (we'd of course happily accept any furhter patches ;) )

• username/pw
• key (authIV, authKey)
• token

[adietish]

I rebased, did minor cleanups and added integration tests here: #151

[adietish]

#151 build run was successfull and your changes merged in :)

[adietish]

I deployed openshift-java-client 2.7.0-SNAPSHOT to our maven repo

[skavanagh]

Sweet!! I can be legit now!!