•  
  • last updated a few seconds ago
Constraints
Constraints: committers
 
Constraints: files
Constraints: dates
  • More
  • CR-8181
  • summarized and closed
Just for closure: Neil and I discussed the issue in the sprint meeting, and decided that there was nothing to do at this juncture.

Just for closure: Neil and I discussed the issue in the sprint meeting, and decided that there was nothing to do at this juncture.

Yes - will change.

Yes - will change.

  • More
  • CR-8206
  • summarized and closed
Unit test in question has been made locale aware.

Unit test in question has been made locale aware.

Would be good one day to have a unit tests framework that load the local to english by default and set a default debug config too.

Would be good one day to have a unit tests framework that load the local to english by default and set a default debug config too.

look good for me too

look good for me too

lgtm

lgtm

It would probably be better to have this strings as constants, but I'm not overly concerned about that for unit tests.

It would probably be better to have this strings as constants, but I'm not overly concerned about that for unit tests.

OPENAM-6837 Unit test fix
OPENAM-6837 Unit test fix
I agree, though I'm not sure it is a feasible change at this point. The STS being peripheral as it is, and me being remote, I don't get the benefit of Phill handling the rest-endpoint integration. ...

I agree, though I'm not sure it is a feasible change at this point. The STS being peripheral as it is, and me being remote, I don't get the benefit of Phill handling the rest-endpoint integration. I know that David Luna wanted to migrate the sts publish, token, and instant-endpoint CREST endpoints to RestEndpoints, as part of the simple validate and cancel work, which is probably where they belong, though I did not have time to complete that work. I suspect that may be necessary to get the benefit of the realm router. Though see the STSPublishServiceHttpRouteProvider - a ResourceRouter gets injected there, not sure if it is the RealmRouter. I suspect that this is not the case - i.e. at some point, new instantiations of each of the 'common' rest services were created in each realm, in accordance to a dynamic realm routing example in CREST, though this was quite some time ago, and the approach has been modified since.

Note also that the cardinality of sts-related services is different than most (all?) other OpenAM services: you can create as many different sts instances as you like in each realm, and they are all routed by the single sts rest endpoint CREST router(see RestSTSServiceHttpRouteProvider). In this sense, sts instances are like authN modules, in that you can create many different service instances in each realm, and the sts rest endpoint CREST router is the sts equivalent to the jaas runtime that 'routes' to individual authN modules. And each sts instance consumes a global token service (again which does not have realm-specific instances, but rather a cache for sts instances which have asked for a token). And the publish service is like the token service - there is a single instance, not an instance per realm (assuming the 'standard' OpenAM rest endpoint procedure is to have a service instantiation in each realm, routed-to by a realm-specific router).

So I'm not quite sure how to proceed. I don't doubt that the ultimate answer is to get the sts more in congruence with the more standard model, but I'm not sure now is the time (though it could be). At some point, I was moving in this direction, but ran into issues with the router not being dynamic enough to handle the sts use-case of un-publishing instances (the 'standard' OpenAM model being to add services in each realm, but not remove them). So again, I'm a bit at a loss. What do you think?

Sure

Sure

OK. Appreciate all your efforts and comments. They've been invaluable. When I think I'm ready to merge on to trunk, would you mind both taking a quick look at the refactoring of the thread pooling?

OK. Appreciate all your efforts and comments. They've been invaluable. When I think I'm ready to merge on to trunk, would you mind both taking a quick look at the refactoring of the thread pooling?

The usual way that OpenAM endpoints include a realm is via the RealmRouter so that you have urls like /json/{realm}/sts or whatever. Can we use that same approach here? It seems odd to have this be...

The usual way that OpenAM endpoints include a realm is via the RealmRouter so that you have urls like /json/{realm}/sts or whatever. Can we use that same approach here? It seems odd to have this be queried by realm differently to all the other endpoints - and it would also remove the need for that massive QueryFilterVisitor just to read one query parameter.

Still, not again... haven't refactored that yes.

Still, not again... haven't refactored that yes.

Currently however, it does have the effect of stopping the radius server as it means we drop out of the Listener's run() method which allows the Radius server to finish.

Currently however, it does have the effect of stopping the radius server as it means we drop out of the Listener's run() method which allows the Radius server to finish.

I'm about to refactor the thread pooling in this class (AME-8315) - I put this here to signal the intention, which is to shutdown the Radius server cleanly. I didn't want to spend time thinking abo...

I'm about to refactor the thread pooling in this class (AME-8315) - I put this here to signal the intention, which is to shutdown the Radius server cleanly. I didn't want to spend time thinking about how to do that before doing the refactoring as it would likely be time wasted.

If you want/need to handle paging and sorting of results, then Rob has some common code for that.

If you want/need to handle paging and sorting of results, then Rob has some common code for that.

Include the stack trace of the root cause too?

Include the stack trace of the root cause too?

Good point. This is a singleton, but only because Guice is configured to provide it as such. Will remove the static nature of the fields.

Good point. This is a singleton, but only because Guice is configured to provide it as such. Will remove the static nature of the fields.

Agreed - I'm done now. Jamie - I trust you to complete any identified issues, we don't have to see any more revisions.

Agreed - I'm done now. Jamie - I trust you to complete any identified issues, we don't have to see any more revisions.

Indentation seems a little messed up here

Indentation seems a little messed up here

Fixed.

Fixed.

Stray semi-colon

Stray semi-colon

More blushes here http://sources.forgerock.org/static/mt0445/2static/images/wiki/icons/emoticons/wink.gif

More blushes here

<blushes>

<blushes>