Conversation
ndp-opendap
requested review from
dh-opendap,
hannahilea and
jgallagher59701
| } | ||
|
|
||
| value = request.getHeader(USER_AGENT_KEY); | ||
| if( d_clientUserAgent == null || !d_clientUserAgent.equals(value) ) { |
There was a problem hiding this comment.
Is it usual that authentication requires a valid User-Agent value? Ah, I see it is, at least according to Google, that makes sense:
However, in practical terms, many servers and web applications expect the User-Agent header to be present and may behave differently or even reject requests that do not include it.
And there's probably no way to test this other than let the code sit in UAT for a while and see what shakes loose.
| * @return The enum value for the units | ||
| */ | ||
| TimeUnits stringToTimeUnits(String timeUnitsStr) { | ||
| if(timeUnitsStr!=null){ |
There was a problem hiding this comment.
for each of these, is it worth supporting the user inadvertently using singular rather than plural units? or is that unlikely/deemed user error? especially since the fallback is to return as if the unit was seconds....
// <MaxSessionLife units="weeks">2</MaxSessionLife>
vs
// <MaxSessionLife units="week">1</MaxSessionLife>
There was a problem hiding this comment.
I was debating startsWith("w") etc...
There was a problem hiding this comment.
if(timeUnitsStr.toLowerCase().startsWith("min")){
There was a problem hiding this comment.
Something has been nagging me about this and I looked a little closer:
I don't think implementing: TimeUnits stringToTimeUnits(String timeUnitsStr) is the way when one can:
TimeUnits units = TimeUnits.valueOf(unitsStr.toUpperCase());It also enforces correct spelling, and it throws IllegalArgumentException if the string doesn't map to a value.
Which doesn't make it lenient as you suggested, but it does utilize more built in machinery.
There was a problem hiding this comment.
yep, that sounds good to me---especially if this is something that would be exposed to a user who encountered it.
...this would be someone standing up a hyrax server for the first time, right? so they'd definitely see the message and the failure and be able to do something about it?
There was a problem hiding this comment.
It would show up in the tomcat logs. And many users would struggle to figure it out.
I am thinking maybe lenient is better.
Consider:
TimeUnits stringToTimeUnits(String timeUnitsStr) {
if(timeUnitsStr!=null){
if(timeUnitsStr.toLowerCase().startsWith("mil")){
return TimeUnits.MILLISECONDS;
}
if(timeUnitsStr.toLowerCase().startsWith("s")){
return TimeUnits.SECONDS;
}
if(timeUnitsStr.toLowerCase().startsWith("min")){
return TimeUnits.MINUTES;
}
if(timeUnitsStr.toLowerCase().startsWith("h")){
return TimeUnits.HOURS;
}
if(timeUnitsStr.toLowerCase().startsWith("d")){
return TimeUnits.DAYS;
}
if(timeUnitsStr.toLowerCase().startsWith("w")){
return TimeUnits.WEEKS;
}
}
return TimeUnits.SECONDS;
}There was a problem hiding this comment.
yeah, that looks good to me---esp if there's a default option that at the very least logs a warning that we've fallen back to the default case
| } | ||
| if(timeUnitsStr.equalsIgnoreCase("weeks")){ | ||
| return TimeUnits.WEEKS; | ||
| } |
There was a problem hiding this comment.
worth adding a warning here (to debug/log) with the unrecognized unit?
| if(sessionIsExpired(session)){ | ||
| log.debug("Invalidating expired session: {}", session.getId()); | ||
| session.invalidate(); | ||
| session = request.getSession(true); |
There was a problem hiding this comment.
is there ever a way we could get caught in a loop through recursion here? if the maxSessionTimeSeconds is really small (or takes roughly the same amount of time as it takes to create a new session via request.getSession(true)...)
There was a problem hiding this comment.
Yes, the user could contrive to deploy with some tiny max session time and break their deployment.
There was a problem hiding this comment.
I should add that the clients would prolly end up with a "too may redirects" error
| log = LoggerFactory.getLogger(this.getClass()); | ||
| setAuthContext(DEFAULT_AUTH_CONTEXT); | ||
| setDescription("The NASA Earthdata Login (formerly known as URS)"); | ||
| setDescription("NASA Earthdata Login"); |
| * @return An EarthDataLoginAccessToken object built from the response from the EDL SSO. | ||
| * @throws IOException | ||
| */ | ||
| public EarthDataLoginAccessToken exchangeAuthCodeForEdlToken(String code, String returnToUrl) throws IOException { |
There was a problem hiding this comment.
...this refactor (pulling this set of code into a new exchangeAuthCodeForEdlToken function) is orthogonal to the task at hand, right? Or am I misunderstanding and there's a specific reason to do it as part of this PR? If possible, I'd prefer to do it as a separate follow-up PR, esp since we already know we have some EDL auth cleanup/refactoring to do in the near future.
There was a problem hiding this comment.
I know this is a bad thing I did, but I couldn't follow the mess of complexity. And I doubt we will ever get back here to do any meaningful cleanup refactoring . We should discuss in person I think.
| sb.append(indent).append("\"").append(getClass().getName()).append("\" : {"); | ||
|
|
||
| String l2i = l1i +indent_inc; | ||
| String jsonObjName = getClass().getName().replace(".","_"); |
There was a problem hiding this comment.
love 2 hand-build json string :D
There was a problem hiding this comment.
Yeah - this is addressed in a my ndp/profile-serialization branch. There's a bunch of work there and a lot of change. More merge challenges for sure.
…ault is utilized.
|
3 participants
All the things: