planetfederal / geoserver-exts

Other
31 stars 40 forks source link

Allow api key override from environment #37

Closed rmarianski closed 10 years ago

rmarianski commented 10 years ago

Allow the api key that the mapmeter extension uses to be overridden by the environment, either through an environment variable or from a servlet context parameter. This also turns off the api key management ui in the geoserver admin interface, and instead displays the active api key in use for the instance and its source (saying either from env var or servlet context)

rmarianski commented 10 years ago

@jdeolive - can you review?

jdeolive commented 10 years ago

Looked it over, and I think the lookup preference you have implemented doesn't quite match the one that is usually used, which is:

  1. web.xml
  2. system property
  3. env var

Btw, this lookup logic is already available through the GeoServerExtensions.getProperty() method. Not sure if you explicitly avoided it or not.

rmarianski commented 10 years ago

Hhmm, I'd expect it to be env -> sys prop -> web.xml, but it's much better to be consistent rather than come up with a different scheme. I didn't realize that GeoServerExtensions had a utility method for this, so I'll just delegate to that. Thanks.

rmarianski commented 10 years ago

In the event of an override, the UI is currently displaying where that override came from: "override provenance". Changing the implementation to delegate to the GeoServerExtension utility method would mean that we wouldn't be able to show this. I don't think this is a requirement though and I just threw it in there because I had access to it. I'll remove it for now because it'll simplify the code a bit. We can always revert back and just change the ordering if we need to show it.

rmarianski commented 10 years ago

Btw, when I tested locally, the lookup ordering was:

  1. system property
  2. web.xml
  3. env var

That seems consistent with:

https://github.com/geoserver/geoserver/blob/e4677162e15384cf925afc340efd3e12b7b40c51/src/platform/src/main/java/org/geoserver/platform/GeoServerExtensions.java#L375-L403

rmarianski commented 10 years ago

@jdeolive - looks ok to merge?

jdeolive commented 10 years ago

Yup, looks good Rob.

rmarianski commented 10 years ago

Thanks Justin.

Merged to 2.2.x, 2.3.x, and master.