Apache Shiro integration

[ganisimov]

No description provided.

[chrisknoll]

Whoever does the merge for this, please ensure that either this PR is squelched down to a single commit, or use the squelch and merge option via the 'merge pull request' option in github.

[chrisknoll]

Hi, @ganisimov ,
I think part of this pull request should also incorporate the security identity to the person saving the different entities in the system (we have concept set, cohort definitions, IR analysis, estimation, etc) that have a created_by and modified_by value to store the user identity that performed the create/update. These fields are varchar(255) so it should be big enough to store the user id value in it.

I've seen something in spring where you can use an annotation (and I'm sorry in the past 5 minutes I couldn't find the annotation documentation on this) but annotating a field in the Entity as @username or something like that will automatically populate the user name in the field when an entity is saved to the repository. The alternative is to go to the save() calls in the code and specifically set the value to the user identity doing the modification.

Either way this is done, we should make that part of this pull request, because when we put this out in a release, we want these 'created_by' and 'modified_by' fields to be updated with the user context that performed the action.

Also, apologies if this is already happening in code, I couldn't find it in the changed files, and I can't look at the raw database that hixbeta is pointing to (I checked on 01, but I think it's now using Amazon services for this database, but I'm not sure).

[ganisimov]

@chrisknoll ,

User's login is saved now in created_by/modified_by fields. If method is not protected and user is not logged in then value 'anonymous' is saved instead of login.

[chrisknoll]

All right, I see it and I like it. separate question: in the event that webAPI with shiro gets released but the Atlas UI is behind, is that to say that all interactions with the webservice will be invoked as an 'anonymous' user, and we could have a release of webapi where anonymous users have full control by default (for backwards compatablity) and then later once atlas comes up to speed, we can then implement specific role security on the WebAPI end points?

The reason for this thought is that I'm trying to imagine what a seamless transition is from the current implementation to one with a security layer, and one way of doing that is to default to giving anonymous full rights (as it is right now, but everyone is known as 'system'), and then each site can modify this default permission to grant to specific users to lock it down.

If this is something that we'd like to adopt, then one other point i'd make is to create a migration script that updates all 'system' values in created_by and modified_by to 'anonymous' since that's basically how people have been working with WebAPI up to the point of implementing shiro.

[ganisimov]

@chrisknoll ,
There is a simple way to switch security features off. You just need to move @Primary annotation from AtlasSecurity class to DisabledSecurity class. This is described here.

In this case any user can access any method. Also if DisabledSecurity is active one, then author of changes will be saved as 'system' as it used to be, so that you won't notice any changes in behaivour.

When you'll be ready to use new features, move the annotation back to AtlasSecurity class.
If you use AtlasSecurity, then user must be authenticated and authorized to access protected endpoints. Note that not all endpoints are protected (you may protect endpoints as it's described here).

Also AtlasSecurity saves user's login into created_by/modified_by fields in case if user is authenticated, and 'anonymous' value in case if not.

I think renaming of 'system' to 'anonymous' should be applyed when you'll be ready to use security features, because DisabledSecurity use 'system' value. Another way to address this is to use 'system' instead of 'anonymous' with both implementations of Security. But I like 'anonymous' more, because it better reflects the situation.

So, how do you feel, should I add the migration, or eliminate use of 'anonymous', or keep current version?

[chrisknoll]

Actually I like to use the notion of 'anonymous' in either disabled or unauthenticated AtlasSecurity. System sounds like a backend process did the change. If security is disabled, everyone is anonymous. So i'd leave it anonymous.

[chrisknoll]

Re: what to do with existing values, I think it makes sense to have a migration script that updates any modified_by/created_by from 'system'=> 'anonymous'.

Would be something like

update {ohdsi_schema}.cohort_definition set created_by = 'anonymous', modified_by = 'anonymous' where created_by = 'system' or modified_by = 'system'

Just need that for the different tables. I can produce that script if you like and if we're in agreement that anonymous should be the identity in both disabled and enabled atlas security contexts.

[ganisimov]

Thanks for allaying my doubts. My initial thought was that 'DisabledSecurity' should mimic old behavior. I added migration and updated the app on hixbeta.

[chrisknoll]

Yeah normally I'd be in favor of leaving it the same for backwards compatibility, but in this case it was a column that wasn't really used so there's no backwards dependency on that value and the original value of 'system' was just a quick default value that I thought made sense at the time. But this branch introduces authentication, even if it's disabled, unauthenticated users are anonymous so this just makes more sense. Later on we might have some backend processes that will have a 'system' identity (for doing scheduled cleanup or something) and at that time we might have a real identity of 'system' introduced into WebAPI, and i wouldn't want all the anonymous users appearing as 'system' in the app. So I think this is the right way to go.

[ganisimov]

Forget to mention that last commit was amended to remove the change where I put 'system' instead of 'anonymous' in 'DisabledSecurity'. So that 'DisabledSecurity' uses 'anonymous'.

[anthonysena]

Hi Gennadiy - it looks like the Shiro enhancements require we use SSL and I was curious how you configured this in your development environment? Apologies if this is noted somewhere and I just missed it. Thanks!

[chrisknoll]

@ganisimov , I'm sorry I missed this in the review, but could you please add the prefix: '${ohdsiSchema}.' (removing ') to each of the tables you are creating in your migration scripts as well as the UPDATE migraiton that is updating the created_by, modified_by? We have to explicitly state the schema we're managing in the migration scripts, and the ${ohdsiSchema} is configured in application.properties to be set to the appropriate schema.

[ganisimov]

@chrisknoll
I added the prefix. Just curious, why this is required? I'm asking because my understanding was that its enough to adjust 'flyway.schemas' in the POM to make Flyway use first schema in this list as default one.

@anthonysena
I'll add discription of how you can configure Tomcat to use SSL and will post the link to the wiki here.

[ganisimov]

@anthonysena
Here is a link to section in the wiki - http://www.ohdsi.org/web/wiki/doku.php?id=development:security#ssl_configuration_in_tomcat

[anthonysena]

@ganisimov Thanks for the instructions - they worked well. I've amended them a bit to include a new page for using OpenSSL to self-sign certificates (which is helpful when doing development locally). I still need to do more testing but I am now able to run this version of WebAPI locally with and without SSL. One item that we'll need: you'll need to rebase this branch against the current WebAPI master branch so it will be easier to merge.

One item that @chrisknoll and I were talking about was changing the pom.xml settings so that we can externalize certain security settings. For example:

<security.origin>http://localhost:8080</security.origin>

Generally we set these using a placeholder so that we can specify instance specific settings in another file (settings.xml) without impacting the pom.xml. For example, on my local instance I had to change this setting from http://localhost:8080 to http://localhost since I run my web applications on port 80.

One more item that I wanted to note is that when running WebAPI locally with SSL, you need to update the ./src/main/resources/application.properties to the following:

# EMBEDDED SERVER CONFIGURATION (ServerProperties)
#server.port=8080 # No SSL port configuration
server.port = 8443
server.ssl.key-store = classpath:keystore.jks
server.ssl.key-store-password = <super secret password>
server.ssl.key-password = <super secret password>

The configuration above requires specification of the keystore.jks created per the wiki article along with the password used when creating the key store. Also on the Window's side, I had to install my local CA certificate in the Trusted Root Certificate Authority (requires a reboot). Alternatively, you can use chrome://flags to disable security warnings for localhost (but this is not recommended).

@chrisknoll I'm not sure if we can externalize these types of application.properties or if these should just be left in the file commented out as this type of configuration is only required when running locally.

[chrisknoll]

Thanks for digging into this @anthonysena , I completely agree with moving the values from application.properties into a placeholder in pom.xml which is then overridden using settings.xml. Especially the keystore passwords. The other thing that looks like we need to review is the key-store property is set to a classpath, making it look like you'll find the store somewhere in the classpath of webapi. not sure if that's what we intend there. @ganisimov , do you have any insight as to setting the value of server.ssl.key-store to some physical location?

[ganisimov]

@anthonysena, thank you!
I extended POM with embeded server properties. Now you can define them in POM or in local settings.xml.

In terms of externalizing security settings, you can update your settings.xml file with definition of profile which you can use when building the app to override values in POM and in application.properties. Here is an example

<settings xmlns="http://maven.apache.org/SETTINGS/1.0.0"
          xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
          xsi:schemaLocation="http://maven.apache.org/SETTINGS/1.0.0 http://maven.apache.org/xsd/settings-1.0.0.xsd">
  <profiles>
    <profile>
        <id>env-webapi-local</id>
          <properties>
            <datasource.driverClassName>org.postgresql.Driver</datasource.driverClassName>
            <datasource.url>jdbc:postgresql://localhost:5432/OHDSI</datasource.url>
            <datasource.username>ohdsi_app_user</datasource.username>
            <datasource.password>app1</datasource.password>
            <datasource.dialect>postgresql</datasource.dialect>
            <datasource.dialect.source>postgresql</datasource.dialect.source>
            <datasource.ohdsi.schema>webapi</datasource.ohdsi.schema>
            <flyway.datasource.driverClassName>${datasource.driverClassName}</flyway.datasource.driverClassName>
            <flyway.datasource.url>${datasource.url}</flyway.datasource.url>
            <flyway.datasource.username>ohdsi_admin_user</flyway.datasource.username>
            <flyway.datasource.password>admin1</flyway.datasource.password>
            <flyway.locations>classpath:db/migration/postgresql</flyway.locations>

            <domain.ui>http://localhost</domain.ui>
            <security.ssl.enabled>true</security.ssl.enabled>
            <security.token.expiration>43200</security.token.expiration>
            <security.origin>${domain.ui}</security.origin>
            <security.oauth.callback.ui>${domain.ui}/Atlas/#/welcome</security.oauth.callback.ui>
            <security.oauth.callback.api>http://localhost:8084/WebAPI/user/oauth/callback</security.oauth.callback.api>
            <security.oauth.google.apiKey>my-key</security.oauth.google.apiKey>
            <security.oauth.google.apiSecret>my-secret</security.oauth.facebook.apiKey>
            <security.oauth.facebook.apiKey>my-key</security.oauth.facebook.apiKey>
            <security.oauth.facebook.apiSecret>my-key</security.oauth.facebook.apiSecret>

            <server.port>8443</server.port>
            <server.ssl.key-store>${user.home}/.keystore</server.ssl.key-store>
            <server.ssl.key-store-password>keystorepass</server.ssl.key-store-password>
            <server.ssl.key-password>keypass</server.ssl.key-password>

          </properties>
    </profile>
  </profiles>
</settings>

Now you can use id of profile (env-webapi-local) to build the app with these settings.
Please let me know, if this is not what you asked for or you have any questions.

This branch is already on top of OHDSI's master, so you won't come up with conflicts.

[ganisimov]

Sorry, looks like i pressed wrong button and closed the PR. It is reopened now.

[chrisknoll]

@ganisimov , I'm getting a build error at this line:
TokenManager.java line 36:
userToKeyMap.replace(subject, key);

The error is that the replace is an unknown token like you can't call replace() on a Map.

The replace function was introduced in JDK1.8. Are we targeting 1.8? I thought we had to limit to JDK 1.7?

[chrisknoll]

Follow up: I'm seeing this in POM.xml:

      <plugin>
        <groupId>org.apache.maven.plugins</groupId>
        <artifactId>maven-compiler-plugin</artifactId>
        <version>2.5.1</version>
        <inherited>true</inherited>
        <configuration>
          <source>1.7</source>
          <target>1.7</target>
        </configuration>
      </plugin>

I'm a real novice at maven, but it looks like this would instruct the compiler to target 1.7 JDK. But @anthonysena is not seeing compile errors on his dev env because he actually has the 1.8 JDK installed. So I think we need to enforce JDK 1.7 target, and I'm not sure why the POM.xml is not doing it.

[chrisknoll]

Ok, I have more information:

The IDE I'm using is NetBeans 8.0.2. This version of the IDE has the JDK 1.7 as a default which is why I got the compiler errors.

I downloaded and installed NetBeans 8.2 which sets the JDK 1.8 as a default option to see if I could get it to alert of non 1.8 problems. What I did was I added a 'Java Platform' in Tools->Java Platforms, and then added a new platform referencing my JDK 1.7 path (C:\projgram files\java\jdk1.7.0_51). Then I went to properties of the WebAPI project, and went to Build->Compile screen, and selected the 1.7 JDK from the dropdown. After that, it kicked off a rebuild and I saw the compile errors.

So, long story short, we should all make sure that the compiler in the IDE is using a 1.7 JDK to ensure that no 1.8 features are being called. There's also a way to specify a 1.7 compiler via 'toolchain' which is described here, but it is more invovled to set up than just ensuring that your IDE is using the proper JDK;
http://maven.apache.org/plugins/maven-compiler-plugin/examples/compile-using-different-jdk.html

[chrisknoll]

@ganisimov , Sorry for the endless questions:

I'm looking at the values you are putting in settings.xml:

            <domain.ui>http://localhost</domain.ui>
            <security.ssl.enabled>true</security.ssl.enabled>
            <security.token.expiration>43200</security.token.expiration>
            <security.origin>${domain.ui}</security.origin>
            <security.oauth.callback.ui>${domain.ui}/Atlas/#/welcome</security.oauth.callback.ui>
            <security.oauth.callback.api>http://localhost:8084/WebAPI/user/oauth/callback</security.oauth.callback.api>
            <security.oauth.google.apiKey>my-key</security.oauth.google.apiKey>
            <security.oauth.google.apiSecret>my-secret</security.oauth.facebook.apiKey>
            <security.oauth.facebook.apiKey>my-key</security.oauth.facebook.apiKey>
            <security.oauth.facebook.apiSecret>my-key</security.oauth.facebook.apiSecret>

Few questions:
1: Do we need to know the ${domain.ui} before hand? I only say this because sometimes we build a war, test it locally and then deploy that war up to the server, but it looks like if i want to test it locally, and then deploy to our intern environment (such as hixbeta.jnj.com) I'll need to reset that value in settings.xml and deploy the war file without validating it working. (I'm assuming that if the ${domain.ui} isn't set correctly something with the SSL calls will fail).

1. In the security.oauth.callback.api property, you reference a url localhost:8084/WebAPI but the ssl port you are specifying 8443, so will that callback accessing port 8084 even work? I thought that when you start the Tomcat using SSL it doesn't open up a HTTP connector unless you specify it programatically, although i did see this:
   http://stackoverflow.com/questions/36490709/spring-boot-enable-https-for-embedded-tomcat
   and it has a application properties value: server.http.port which it seems if you set to a port value it will open up a HTTP under that port and HTTPS will be opened under the server.port one. I need to do more experimentation to understand what the real behavior is.

[ganisimov]

Hi @chrisknoll ,
Good catch, I'll re-build WebAPI using JDK 1.7 and fix errors. Thank you for detailed explanation of how to use JDK 1.7 in NetBeans!

Regarding example of settings.xml, looks like I brought more confusion then a help. You're write, this configuration will break OAuth capabilities, because it references wrong url. My mistake.

In terms of setting ${domain.ui}, I think this is normal to adjust envirionment depended variables within 'settings.xml'. Any way, you need to change connection strings and so on. I think the best way to handle this is to use mechanism of profiles. You can define certain values in different profiles and then refernce them by IDs. In my previous example the ID was 'env-webapi-local'. And you can define another profile for hixbeta environment, for example 'env-webapi-hixbeta', which contains another values for the properties.

If you build with NetBeans, to apply profile's values just go Run --> Set Project Configuration --> env-webapi-local (or 'env-webapi-hixbeta').

Even if you made mistake when you deployed first time, you can fix it in your 'env-webapi-hixbeta' profile to avoid it next time. You can read more about profiles in 'settings.xml' here.

${domain.ui} is used here like a local variable to set two things, because they're naturally the same. This just helps to make sure that these properties don't conflict to each other.

First is 'security.origin' property. This defines the value for HTTP header 'Access-Control-Allow-Origin'. If this value is not the same as origin of calling application, then browser will reject this request due to same origin request policy. It affects AJAX calls from within a browser. Wrong value will fail an initialization of ATLAS and you will see a message like 'Initialization failed' or smth like this write after attempt to start it.

The second property is 'security.oauth.callback.ui', and here ${domain.ui} is used as application root for endpoint to redirect after OAuth authentication. In this case wrong value will break OAuth. After you log in with OAuth provider, WebAPI will redirect you to URL defined in this property.

[chrisknoll]

Thank you @ganisimov for the explanation, I'll set up additional profiles to address your points above. Thank you.

[ganisimov]

@chrisknoll , I fixed the error and redeployed in hixbeta.

[chrisknoll]

I don't think this should be added to the main pom.xml: the posgresql driver is available via maven repo so there's no burden to register the driver. However, adding the Microsoft driver to the root pom means people need to manually register the artifact in their local maven repo.

Instead we put this dependency in the mssql profile within the pom, and if people build using the mssql profile, then it will require the dependency.

[ganisimov]

@chrisknoll
Apologies for the delay. I've been trying to reproduce the error related to hibernate dialect. I received similar error using database with compatibility level 'SQL Server 2005 (90)' on SQL Server 2012 with MS JDBC driver version 6.0 (JDBC version 4). Note that here are two versions: 6.0 - is version of driver and 4 - is version of JDBC API.
Microsoft recently made their JDBC driver available on the Maven Central Repository. Use of this driver eliminates the error in my local environment. Please, give it a try. I added corresponding dependency and removed dependencies which point to local repository. There are two versions of this driver - jre7 and jre8. I added jre8 assuming that we will target JDK 1.8.
If this won't help, my suggestion is to add setting in POM to specify certain dialect and if it's set use it to adjust 'hibernate.dialect' property.
Also I fixed missing prefixes. Thank you for finding this!

[chrisknoll]

@ganisimov : Hope everything well with you.

I've been working with @anthonysena about getting the branch working locally, but on top of that seeing what happens when we point a 'non-security' Atlas version (like what is currently on master in OHDSI/Atlas) and pointing it at a secured WebAPI just to see what'll happen.

The first thing we thought we saw was that in the new version of Atlas, there were HTTP headers being set up on the client side that we thought was causing errors when invoking URLs that had a security restriction on. Can you confirm that the authorization headers are required when making WebAPI calls under shiro? What we'd prefer is that if there is no authorization headers provided, the user context that is assumed is 'anonymous' and then anyone invoking the WebAPI call will be authorized as an 'anonymous' user vs. a logged in user (and everyone using the old Atlas version against the Shiro-WebAPI will just intract with the WebAPI calls as 'anonymous'. This lets us do things like 'Let anonymous do anything they want' (which is what we'd want to do on a demo site).

The second thing we saw was that there's some HTTP VERB mismatches between what the current ATLAS expects for create/update vs. what the new shiro WebAPI expects when create/update. Me and @anthonysena did some research on which verbs should be used for which CRUD operations, and one article pointed that C = POST and U = PUT. However, I found a more in-depth discussion about the verbs in this article: http://restcookbook.com/HTTP%20Methods/put-vs-post/. In my opinion., the statement about PUT being 'idempotent' vs POST not being 'idempotent' is the key here. Being 'idempotent' in WebAPI means that sending the data to the server does not add new entities to the database. So if you PUT some data to /cohortdefinitions/1 then you are updating the resource foudn at /cohortdefinitons/1. If you PUT some data to /cohortdefinitions/ (like we do today when we create a cohort definition), by nature of the PUT url going to /cohortdefinitions/ (no ID here), it means that we want to create a new entity. But per the discussion in the above article, creating the new entity (which will have a resource URI of /cohortdefinition/{newId} means that this should have been sent up as a POST (because if you take the same data and PUT it, the first url would be /cohortdefinition/{newId}. sending it again would be /cohortdefinition/{newId + 1} and again it would be /cohortdefinition/{newId+2}...and so on. The fact that the PUT is resulting in different resource URIs being changed makes it not idempotent and therefore the PUTs that create should be 'POST"s.

Now, I think this is what you exactly have for concept set creation and maybe even cohort definition creation....but the problem is that we'd like it to be consistent between the Atlas on MASTER with the Atlas on the shiro branch (so that it's backwards compatable). To this end, I would like to get agreement that creates should be POSTs (because makes a new URI to access the entity) and updates should be PUTS (because no new resource URI is created). And once we have this concensous, I'd like to apply an update to our current NON-shiro ATLAS and non-SHIRO WebAPI to bring these rules in line, and then when we point the non-shiro ATLAS to a shiro-atlas there's no misunderstanding between the HTTP verbs that are being used for create/update in WebAPI.

If you are in agreement, I'll open up a new issue on Atlas and WebAPI to fix the HTTP Verbs for all CRUD operations and then you'll need to merge those changes to master into your branch (which may lead to conflicts on your side, but it's just something that you'll just need to fix once, but it will give us the backwards compatibility we need to have old-Atlas work with shiro-WebAPI.

Let me know. Thanks.

-Chris

[chrisknoll]

Incidentally, you might be asking yourself "If POSTS = C and PUT = U in WebAPI, why isn't that just the simple rule?

Imagine the case where we're working on a API where you're submitting some content that part of the content submission is to fill in a unique identifier that will be used to locate the content at a later time.

So, you would be creating content, but in this case because the unique identifier is being specified during creation, it is idempotent in the sense that if you submit to the resource multiple times, the URI to the resource isn't changed. So:

PUT: /api/content/my-winter-content <-- results in a new record
PUT: /api/content/my-winter-content <-- existing record is updated
PUT: /api/content/my-winter-content <-- existing record is updated.

Contrast what we do today in webAPI:
PUT: /webapi/cohortdefinition/ <--- results in new URI being created /webapi/cohortdefinition/newId
PUT: /webapi/cohortdefinition/ <--- results in new URI being created /webapi/cohortdefinition/newId+1
PUT: /webapi/cohortdefinition/ <--- results in new URI being created /webapi/cohortdefinition/newId+2

So really, for WebAPI creates, these should all have been posts, because the client who is sending the data isn't specifying the final URI to find the resource at. I think this will be the case for any C- operation in WebAPI, so it is a simple rule to say 'C = POST' but I just wanted to put down the reasoning behind why this should be, but in the futre if we have a WebAPI where the caller can specify the resource URI to be created (like in my content example above), the C and the U would both be PUT because the all the calls refer to the same resource to update, and is therefore is idempotent.

-Chris

[ganisimov]

Hi @chrisknoll!

To 'Let anonymous do anything they want', all you need is just to build WebAPI using 'DisabledSecurity' as its described here . Please let me know if you have any questions about it.
When 'DisabledSecurity' is on, all urls are not protected and any user can access any resource.
With 'AtlasSecurity', some urls are protected (not all of them). This means that user should be authenticated and authorized to access protected resources. Authentication of requests is achived by inspecting Authorization header.
So you need to set a valid Authorization header only to access protected urls. If url isn't protected, you don't need Authorization header.

In terms of HTTP VERB mismatches, I don't see any problems with aligning these rules. Shiro branch affects only few lines of existent WebAPI code, so it would be easy to incorporate changes on WebAPI side.
The only thing I'd like to mention here is that there should be different methods for 'create' and 'update' logic. This is important, because you may want to apply different security rules for 'create' and 'update' methods. This can be done by defining certain rules for certain combination of method's URL and HTTP VERB.
For example, in old WebAPI (which is in master branch) 'ConceptSetService' exposes one method for both 'create' and 'update'. But requirements stated that only permitted users should have ability to create Concept Set and only author of Concept Set should have ability to update it. So that I had to decouple this method into two separate methods in shiro branch.

[leeevans]

Hi @ganisimov , I manage the build process for deploying the OHDSI WebAPI & apps to the OHDSI cloud sandbox and the associated "Broadsea" Docker containers.

Does the Shiro framework support enabling or disabling security at run-time? Can you expose a single run-time environment variable to set the DisabledSecurity option on or off as an alternative, or in addition to the compile time approach you linked to?

Benefits of a run-time DisabledSecurity environment variable:

• Supports enabling/disabling security for debugging of deployed WebAPI security access/config issues
• Supports a single build process, eliminating the overhead of maintaining separate build chains for secure/non-secure WebAPI builds

[chrisknoll]

@leeevans : we're working on an update that will make the security mode a setting in the config, meaning you can specify the security settings in the build's settings.xml configuration. However, this can't be changed at runtime since this is set at build time not runtime.

[chrisknoll]

@ganisimov ,
Thanks for the info, looks like disable security is the way we'll need to operate.

I'll open up a separate PR for adjusting the HTTP verbs for CRUD operations. For cohort definitions the create URL was PUT: /cohortdefinitions/ (no ID specified, PUT verb), and update was PUT: /cohortdefinitions/{id} (ID specified, PUT verb). So we did have 2 different methods in that case, but wrong verb for create (POST: /cohortdefinitions/). For concept sets, I guess they had 1 URL and made it a conditional in the call (like if id = null then create else update). I can make this change in master so that you have different methods for the different permissions.

After this is put in, I'll ask you to merge from master into your branch, and you can resolve conflicts.

[leeevans]

@chrisknoll I'm a noob regarding Shiro but what about implementing the option of a simple shiro.ini file to allow run-time config and we can add a simple 'anonymous' user in the shiro.ini file for the public OHDSI cloud?

https://shiro.apache.org/tutorial.html

Factory<SecurityManager> factory = new IniSecurityManagerFactory("classpath:shiro.ini");

[chrisknoll]

@leeevans, I'm not sure about it. Looks like the example .ini file has users defined in it too. Does that mean that file is the datastore for user identities? This branch stores that stuff in a DB....so not exactly sure what to target to fit your needs, but for purposes of getting the shiro branch integrated into master, I'm just trying to resolve the various build issues and configuration issues to make life easier for getting new developers set up (such as relaxing the need for SSL, backwards compatablity with non-secured versions of Atlas, JDK 1.8 implications, etc), and making sure I can get it working locally. Beyond that I didn't have plans to extend anything done here to expand the scope of the branch. But if others have interest in doing so, they should feel welcome to explore it!

Thanks!

-Chris

[leeevans]

@chrisknoll I believe a small set of static users/passwords/roles can optionally be defined in the shiro.ini file as an alternative data source for shiro authentication/authorization. There seems to be a lot of different configuration options available in the shiro.ini file.

I've added a separate enhancement issue "enabling shiro runtime config with shiro.ini" as I don't want to distract from this pull request.

[ganisimov]

Hi @leeevans , @chrisknoll

I added 'security.disabled' setting into POM and application.properties to define which implementation of 'Security' to use. If the 'security.disabled' is set to true, 'DisabledSecurity' is used, otherwise - 'AtlasSecurity' is used.

You can define it in POM (or settings.xml) before building the app (it will be propagated into application.properies) or you can change this property after deployment in {AppPath}\WEB-INF\classes\application.properies directly. To apply changes application needs to be restarted ('Reload' button in 'Tomcat Web Application Manager').

@leeevans , does it solve your needs?

If you just add user with shiro.ini or other way, you still need to authenticate/authorize requests. To switch security off on Shiro level, you need to create config which doesn't set any security related filters instead. And there is some other stuff to handle, but this are details.

So that, you need two configs - one for disabled security and one for enabled. This is actually done by using two implementations of 'Security' class, which mostly is a kind of wrapper for Java-style Shiro configuration, but also gives one entry point for all security related tasks. So I'd like to stick to it, because in the future we may need some other security related features, which Shiro doesn't handle.

[chrisknoll]

@ganisimov : you must have read me and @anthonysena's mind. We came up with the same local solution to put it in a pom.xml/settings.xml file to enable security. But we called it 'security.enabled' instead of 'security.disabled' because I think the default config will be disabled. @anthonysena is creating pull requset for this.

Another thing I'm looking at is disabling the SSL requirement for local development mode. @anthonysena again has saved the day by looking into what settings in POM/settings.xml are cleared (set empty) to make the tomcat instances start up in non-SSL mode (this is for embedded testing) so we got local starting as non-ssl (and saving us the trouble to do keystore/OPENSSL stuff) and it working, but only after we disabled the 'require SSL' filter chains that are being applied in Shiro. So, the change here is to add a flag for enabling ssl and then don't apply those require SSL filters if ssl-enabled = false.

@anthonysena will push up the branch and PR and if you could review that'd be great.

Last qustion about your deplyoment to hixbeta: How did you configure the SSL properties? Was it done A) as a setting in the server.xml file directly in tomcat on the hix beta server or B) tomcat/spring boot is smart enough to configure the WAR deployment to tomcat with the proper SSL profile and it will automatically set up the right SSL channels when you deploy the .WAR to tomcat using the /manager app.

-Chris

[ganisimov]

@chrisknoll ,
I think 'security.disabled' better reflects what it does, but actually it doesn't meter. Do you want me to reset the latest commit? Or I can just rename the property.

You can switch SSL off by setting 'security.ssl.enabled' property in POM (or local settings.xml) to false. If it's set to true, application allows only SSL connection.
All security related settings are listed here.

To make Tomcat working with SSL, you need to define a connector in server.xml in addition to settings in POM file. You may find an example here:

  <Connector 
    port="8443" 
    protocol="org.apache.coyote.http11.Http11NioProtocol"
    maxThreads="200"
    scheme="https" 
    secure="true" 
    SSLEnabled="true"
    keyAlias="webapi"
    keystoreFile="C:\path\to\my\keystore.jks" 
    keystorePass="{Your keystore password}"
    clientAuth="false" 
    sslProtocol="TLS"/>

So here are two levels of adjustments for SSL - 'security.ssl.enabled' and 'security.ssl.port' in POM (application level) and SSL Connector in server.xml (server level).

[anthonysena]

@ganisimov I've forked your fork of WebAPI and will create a Pull Request to your repository with the changes that @chrisknoll described. In that PR, I'll include the change to the security.ssl.enabled flag name so don't worry about that for now.

[anthonysena]

@ganisimov I've created a PR to your Shiro fork of WebAPI for your review. The idea with this pull request is two-fold. First, we want to establish a set of default settings for WebAPI where SSL and security are disabled. Secondly, we want to ensure SSL and Security can be enabled via configuration changes without the need to touch the Java code. I believe this PR accomplishes those goals. There are a few items that I want to note here which we'll need to include in the wiki:

To enable Security: By default, the pom.xml specifies that security is disabled:

<security.enabled>false</security.enabled>

This can be overridden by including the same property in settings.xml (under settings -> profiles -> profile -> properties):

<security.enabled>true</security.enabled>

To enable SSL: By default, the src/main/resources/application.properties file has these settings commented out:

server.ssl.key-store = ${server.ssl.key-store}
server.ssl.key-store-password = ${server.ssl.key-store-password}
server.ssl.key-password = ${server.ssl.key-password}

If you want to use SSL in your local development environment, these need to be activated by removing the '#' comment. We tried to leave these settings active and disable SSL but unfortunately, that does not appear to be possible. When running embedded tomcat with these settings active (even without values set), tomcat will start and automatically use SSL. Also, the following settings need to be present in your settings.xml (under settings -> profiles -> profile -> properties) to use SSL locally:

<server.port>8443</server.port>
<security.ssl.enabled>true</security.ssl.enabled>
<server.ssl.key-store>classpath:keystore.jks</server.ssl.key-store>
<server.ssl.key-store-password>supersecret</server.ssl.key-store-password>
<server.ssl.key-password>superdupersecret</server.ssl.key-password>

As you mentioned, the good news is that this is only required for local development as these settings can be controlled via Tomcat's server.xml file.

[chrisknoll]

@ganisimov: I've pushed changes to master to get all the HTTP verbs in line. It shows a conflict in this branch now because of that. This might be a good opportunity to create a branch off of the current head of your fork branch and rebase that branch down to a single commit, rebase it to master, and then submit a new PR based on the squelched commits (and based off of current master). I don't think we're going to need any further changes to this branch since I think we've got it running locally.

If you are inclined, if you rebase and squelch this branch into a new PR, then I'll take the new 'clean' branch as a final version for inclusion into the OHDSI master repo.

[chrisknoll]

@anthonysena : I found this in the spring boot conversation about enabling/disablign ssl:
spring-projects/spring-boot#2241

Seems like if we set server.ssl.enabled = false, that should disable SSL, and therefore we won't need to remove those values from the application properties file. Did you want to give that a shot?

[ganisimov]

@anthonysena, I added a comment to your changes. The idea is that SSL filter can be disabled by calling its setEnabled method (see getSslFilter() function in AtlasSecurity class). So that you don't need to remove this filter from the filter chain to disable SSL on application level.

This behavior is already implemented. All you need to disable SSL on application level is set security.ssl.enable POM/Settings property to false as I mentioned in my previous comment. This way I disabled SSL in my local environment.

Please let me know if it doesn't work for you.

[anthonysena]

Thanks @ganisimov and @chrisknoll for the input into these changes - I've submitted a new PR to address these concerns. To summarize:

• The security.disabled setting has been renamed to security.enabled. I've added a few logging statements to the ShiroConfiguration.java class to output the type of security module that has been loaded when the WAR starts up.
• @chrisknoll: you were correct - using server.ssl.enabled prevents us from having to touch the application.properties file! So, I've mapped the {$security.ssl.enabled} value to both the sslEnabled property in AtlasSecurity and the server.ssl.enabled property in application.properties. The result is that you can simply set this security.ssl.enabled to true/false to toggle SSL.
• I've removed the security.ssl.port setting as it was redundant and not honored by embedded tomcat. Instead, the sslPort property in AtlasSecurity will use the value set by the server.port setting.
• Also, I've set the default values in pom.xml such that security and ssl will be disabled by default.

Hoping that this should be the last set of changes and then we can focus on bringing this into WebAPI.

[chrisknoll]

Sounds good, thanks @anthonysena . Once this gets updated in this PR, I can pull this PR in to a staging branch in the OHDSI repo to squelch down the commits and rebase it to the latest master. I can do a final run-through with those settings (and I'd like to be able to test deploy this up to hixbeta (on the '01 instance, not the place where it's already set up) to ensure that we can deploy properly.

Then once it's ready we'll figure out when it should be pushed onto master. I think there's a few more end-of-year enhancements that I'd like to get in as well and make a formal release of the as-of-now unreleased updates that are on master but haven't been pushed out to the community.

[ganisimov]

@anthonysena , thank you! Everything looks good to me. I merged your changes and created another pull request, which has no conflicts with master and taking into account that HTTP verbs were changed.