Skip to content
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.

Sign up for GitHub

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

Bugfix/apikey header capitalization #447

Merged
merged 6 commits into from
Nov 3, 2023
Merged

Bugfix/apikey header capitalization #447

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
eb6c121
Cleanup web.xml.
MikeNeilson Oct 30, 2023
4cf28d3
Using .path() included things like a Timeseries ID which was too long…
MikeNeilson Oct 31, 2023
dfd8a89
Update to allow case insensitive auth type and rendering in swagger c…
MikeNeilson Oct 31, 2023
626d730
Update swagger ui web jar.
MikeNeilson Oct 31, 2023
b900ed9
Remove exploratory code that wasn't required.
MikeNeilson Oct 31, 2023
5fb14da
Allow for AuthDAO and KeyAccessManager to function correctly in multi…
MikeNeilson Nov 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions cwms-data-api/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,12 +149,10 @@ dependencies {

// testImplementation group: 'commons-beanutils', name: 'commons-beanutils', version: '1.9.4'

webjars('org.webjars:swagger-ui:4.18.2') {
webjars('org.webjars:swagger-ui:5.9.0') {
transitive = false
}



}

configurations.all {
Expand Down
27 changes: 18 additions & 9 deletions cwms-data-api/src/main/java/cwms/cda/ApiServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,13 @@ public void init() {
case 403: re = new CdaError("Not Authorized",true); break;
default: re = new CdaError("Unknown auth error.");
}
logger.atInfo().withCause(e).log(e.getMessage());

if (logger.atFine().isEnabled()) {
logger.atFine().withCause(e).log(e.getMessage());
} else {
logger.atInfo().log(e.getMessage());
}

ctx.status(e.getAuthFailCode()).json(re);
})
.exception(Exception.class, (e, ctx) -> {
Expand All @@ -292,17 +298,17 @@ public void init() {
})

.routes(this::configureRoutes)
.javalinServlet();
.javalinServlet();
}

private CdaAccessManager buildAccessManager(String provider) {
try {
AccessManagers ams = new AccessManagers();
try {
AccessManagers ams = new AccessManagers();
return ams.get(provider);
} catch (ServiceNotFoundException err) {
throw new RuntimeException("Unable to initialize access manager",err);
}

}

protected void configureRoutes() {
Expand Down Expand Up @@ -434,9 +440,8 @@ private void getOpenApiOptions(JavalinConfig config) {
req.addList(manager.getName());
secReqs.add(req);
}

});

config.accessManager(am);

OpenApiOptions ops =
Expand All @@ -461,13 +466,17 @@ private void getOpenApiOptions(JavalinConfig config) {
})
.activateAnnotationScanningFor("cwms.cda.api");
config.registerPlugin(new OpenApiPlugin(ops));

}

private static void setSecurityRequirements(String key, PathItem path,List<SecurityRequirement> secReqs) {
/* clear the lock icon from the GET handlers to reduce user confusion */
logger.atFinest().log("setting security constraints for " + key);
setSecurity(path.getGet(), new ArrayList<>());
if (key.contains("/auth/")) {
setSecurity(path.getGet(), secReqs);
} else {
setSecurity(path.getGet(), new ArrayList<>());
}
setSecurity(path.getDelete(),secReqs);
setSecurity(path.getPost(), secReqs);
setSecurity(path.getPut(), secReqs);
Expand Down
79 changes: 51 additions & 28 deletions cwms-data-api/src/main/java/cwms/cda/data/dao/AuthDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.Optional;
import java.util.Set;
import java.util.TimeZone;
import java.util.concurrent.TimeUnit;
import java.util.logging.Level;

import javax.sql.DataSource;
Expand Down Expand Up @@ -51,7 +52,7 @@ public class AuthDao extends Dao<DataApiPrincipal>{
+ "23.03.16 or later to handle authorization operations.";
public static final String DATA_API_PRINCIPAL = "DataApiPrincipal";
// At this level we just care that the user has permissions in *any* office
private final String RETRIEVE_GROUPS_OF_USER;
private static String RETRIEVE_GROUPS_OF_USER = null;

private static final String SET_API_USER_DIRECT = "begin "
+ "cwms_env.set_session_user_direct(upper(?));"
Expand All @@ -69,28 +70,31 @@ public class AuthDao extends Dao<DataApiPrincipal>{
public static final String GET_SINGLE_KEY = "select userid,key_name,created,expires from cwms_20.at_api_keys where UPPER(userid) = UPPER(?) and key_name = ?";
public static final String ONLY_OWN_KEY_MESSAGE = "You may not create API keys for any user other than your own.";

private boolean hasCwmsEnvMultiOfficeAuthFix;
private String connectionUser;
private String defaultOffice;
private static boolean hasCwmsEnvMultiOfficeAuthFix = false;
private static String connectionUser = null;
private static String defaultOffice = null;
Comment on lines +74 to +75
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making these static and conditionally setting them in the constructor weirds me out. I can't say exactly why but it feels like asking for trouble. Like if the first request made is for an un-checked get end-point and the request includes office_id will it cause the static value to get assigned to whatever the first request provided? At the very minimum it seems like assigning to AuthDao.defaultOffice shouldn't happen if the set_session_user_direct failed - right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This always uses the office id of the context "e.g. cwms -> hq, spk -> spk" not the context of data.

For the 2nd part I think it just won't get used, but I don't think the additional logic is required just for that state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the short version is that I'm just trying to avoid creating a new instance of this object on every request since it will be used on a LOT of requests.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. the part about the context makes more sense. Should AuthDao have a static getInstance() and just use it singleton style?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered the use of a singleton, that's going to need more investigation. Since the clientInfo is different for every request i needs the new context on each run so I think for now we do new to just run new on every request since their could be multiple threads. Or synchronize on the object which seems like a terrible idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, after realizing it needs the javalin Context information of the current request so that setClientInfo is correct for tracing instead of a pure singleton I made it a singleton per thread.


public AuthDao(DSLContext dsl, String defaultOffice) {
super(dsl);
if (getDbVersion() < Dao.CWMS_23_03_16) {
throw new RuntimeException(SCHEMA_TOO_OLD);
}
this.defaultOffice = defaultOffice;
try {
connectionUser = dsl.connectionResult(c->c.getMetaData().getUserName());
dsl.execute("BEGIN cwms_env.set_session_user_direct(?,?)", connectionUser,defaultOffice);
hasCwmsEnvMultiOfficeAuthFix = true;
} catch (DataAccessException ex) {
if( ex.getLocalizedMessage()
.toLowerCase()
.contains("wrong number or types of arguments in call")) {
hasCwmsEnvMultiOfficeAuthFix = false;

if (AuthDao.defaultOffice == null) {
AuthDao.defaultOffice = defaultOffice;
try {
connectionUser = dsl.connectionResult(c->c.getMetaData().getUserName());
dsl.execute("BEGIN cwms_env.set_session_user_direct(?,?)", connectionUser,defaultOffice);
hasCwmsEnvMultiOfficeAuthFix = true;
} catch (DataAccessException ex) {
if( ex.getLocalizedMessage()
.toLowerCase()
.contains("wrong number or types of arguments in call")) {
hasCwmsEnvMultiOfficeAuthFix = false;
}
}
AuthDao.RETRIEVE_GROUPS_OF_USER = ResourceHelper.getResourceAsString("/cwms/data/sql/user_groups.sql",this.getClass());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to do this here vs at the variable declaration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm quite sure there was when I initial did the work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, that's not a bad idea. Let me chew on that one a bit. It is supposed to be usable by all the possible Auth mechanisms.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that response was to the comment above but I did go ahead and just do this.

}
this.RETRIEVE_GROUPS_OF_USER = ResourceHelper.getResourceAsString("/cwms/data/sql/user_groups.sql",this.getClass());
}

@Override
Expand Down Expand Up @@ -140,21 +144,30 @@ private void setSessionForAuthCheck(Connection conn) throws SQLException {
}

private String checkKey(String key) throws CwmsAuthException {
return dsl.connectionResult(c-> {
setSessionForAuthCheck(c);
try (PreparedStatement checkForKey = c.prepareStatement(CHECK_API_KEY);) {
checkForKey.setString(1,key);
try (ResultSet rs = checkForKey.executeQuery()) {
if (rs.next()) {
return rs.getString(1);
} else {
throw new CwmsAuthException("No user for key");
try {
return dsl.connectionResult(c-> {
setSessionForAuthCheck(c);
try (PreparedStatement checkForKey = c.prepareStatement(CHECK_API_KEY);) {
checkForKey.setString(1,key);
try (ResultSet rs = checkForKey.executeQuery()) {
if (rs.next()) {
return rs.getString(1);
} else {
throw new CwmsAuthException("No user for key");
}
}
} catch (SQLException ex) {
throw new CwmsAuthException("Failed API key check",ex);
}
} catch (SQLException ex) {
throw new CwmsAuthException("Failed API key check",ex);
});
} catch (DataAccessException ex) {
Throwable t = ex.getCause();
if (t instanceof CwmsAuthException) {
throw (CwmsAuthException)t;
} else {
throw ex;
}
});
}
}

/**
Expand Down Expand Up @@ -193,7 +206,9 @@ private Set<RouteRole> getRolesForUser(String user) {
public static void prepareContextWithUser(Context ctx, DataApiPrincipal p) throws SQLException {
Objects.requireNonNull(ctx, "A valid Javalin Context must be provided to this call.");
Objects.requireNonNull(p, "A valid data api principal must be provided to this call.");
logger.atInfo().log("Validated Api Key for user=%s", p.getName());
logger.atInfo()
.atMostEvery(5,TimeUnit.SECONDS)
.log("Validated Api Key for user=%s", p.getName());
DataSource dataSource = ctx.attribute(ApiServlet.DATA_SOURCE);
ConnectionPreparer userPreparer = new DirectUserPreparer(p.getName());
ctx.attribute(DATA_API_PRINCIPAL,p);
Expand Down Expand Up @@ -377,4 +392,12 @@ public DataApiPrincipal getDataApiPrincipal(Context ctx)
{
return ctx.attribute(DATA_API_PRINCIPAL);
}

/**
* Used to avoid constant instancing of the AuthDao objects
* @param dslContext
*/
public void resetContext(DSLContext dslContext) {
this.dsl = dslContext;
}
}
5 changes: 3 additions & 2 deletions cwms-data-api/src/main/java/cwms/cda/data/dao/JooqDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,8 +105,9 @@ public static DSLContext getDslContext(Context ctx) {

private static Connection setClientInfo(Context ctx, Connection connection) {
try {
//logger.atInfo().log("Path : " + ctx.url());
connection.setClientInfo("OCSID.ECID", ApiServlet.APPLICATION_TITLE + " " + ApiServlet.VERSION);
connection.setClientInfo("OCSID.MODULE", ctx.path());
connection.setClientInfo("OCSID.MODULE", ctx.endpointHandlerPath());
connection.setClientInfo("OCSID.ACTION", ctx.method());
connection.setClientInfo("OCSID.CLIENTID", ctx.url().replace(ctx.path(), "") + ctx.contextPath());
} catch (SQLClientInfoException ex) {
Expand All @@ -122,7 +123,7 @@ public static DSLContext getDslContext(Connection database, String officeId) {
CWMS_ENV_PACKAGE.call_SET_SESSION_OFFICE_ID(dsl.configuration(), officeId);

return dsl;
}
}

@Override
public List<T> getAll(Optional<String> limitToOffice) {
Expand Down
8 changes: 6 additions & 2 deletions cwms-data-api/src/main/java/cwms/cda/security/KeyAccessManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public class KeyAccessManager extends CdaAccessManager {

@Override
public void manage(Handler handler, Context ctx, Set<RouteRole> routeRoles) throws Exception {
init(ctx);
init(ctx);
String key = getApiKey(ctx);
DataApiPrincipal p = authDao.getByApiKey(key);
AuthDao.isAuthorized(ctx, p, routeRoles);
Expand All @@ -33,6 +33,8 @@ public void manage(Handler handler, Context ctx, Set<RouteRole> routeRoles) thro
private void init(Context ctx) {
if (authDao == null) {
authDao = new AuthDao(JooqDao.getDslContext(ctx),ctx.attribute(ApiServlet.OFFICE_ID));
} else {
authDao.resetContext(JooqDao.getDslContext(ctx));
}
}

Expand All @@ -53,8 +55,10 @@ private String getApiKey(Context ctx) {
@Override
public SecurityScheme getScheme() {
return new SecurityScheme()
.scheme("apikey")
.type(Type.APIKEY)
.in(In.HEADER)
.description("Key value as generated from the /auth/keys endpoint. NOTE: you MUST manually prefix your key with 'apikey ' (without the single quotes).")
.name("Authorization");
}

Expand All @@ -69,6 +73,6 @@ public boolean canAuth(Context ctx, Set<RouteRole> roles) {
if (header == null) {
return false;
}
return header.trim().startsWith("apikey");
return header.trim().toLowerCase().startsWith("apikey");
}
}
23 changes: 0 additions & 23 deletions cwms-data-api/src/main/webapp/WEB-INF/web.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,29 +18,6 @@
<url-pattern>/status/prometheus</url-pattern>
</servlet-mapping>

<!--
<login-config>
<auth-method>BASIC</auth-method>
<realm-name></realm-name>
</login-config>

<security-constraint>
<web-resource-collection>
<web-resource-name>CWMS DATA</web-resource-name>
<url-pattern>/*</url-pattern>
<http-method>POST</http-method>
<http-method>PUT</http-method>
<http-method>DELETE</http-method>
</web-resource-collection>
<auth-constraint>
<role-name>CWMS Users</role-name>
</auth-constraint>
</security-constraint>

<security-role>
<role-name>CWMS Users</role-name>
</security-role>
-->
<filter>
<filter-name>CorsFilter</filter-name>
<filter-class>org.apache.catalina.filters.CorsFilter</filter-class>
Expand Down
Loading