Origen-SDK / o2

MIT License
4 stars 0 forks source link

Mailer ldap users session and more #134

Closed coreyeng closed 3 years ago

coreyeng commented 3 years ago

Hello,

After spending the last few PRs saying I would be do more bite-size chunks, I've proceeded to throw that out the window and do another large-scale one. Believe it or not, this wasn't on purpose. I started by adding a mailer and got it going by just hard-coding values, but in order to do much outside of hard-coding, I needed to expand users. After expanding users a bit, I wanted to fix up the storing and retrieving of passwords, so the sessions store came about. Then, I wanted to verify the password was correct before sending it to the mailer, and so the ldap popped up. So, this PR includes:

For everything except the mailer though, this is a good "save point" as the basics are there and the underlying utilities have been updated quite a bit.

Instead of going through the usages here, I updated the docs directly. I still can add more and provide clarifications, but this should give a decent starting point. Here, I'll go into more of the design aspects instead.

General

All of these utilities are accessible as rust-side statics, like the DUT, Tester, etc., so they are available on both sides. Because of this, all the configuration is done in the config files. This is a bit tricky because we're trying to pass in a lot of configuration stuff, but I think I got it as good as we can get. Unfortunately, I could not find a way to extend Configs to allow for custom structs (requires extending private modules, which Rust doesn't allow) so I had to make-do with just using maps (or tables, as TOML calls them) to pass in structures. I actually don't think that's too bad by itself, but a potential problem is the entire table gets overwritten by a higher-priority config. We could pretty easily add a table-merge, which I think we'll want eventually, I just haven't added it yet.

I don't think there's anything too controversial here. There' s not much in the way of actual changes - mostly just adding new stuff. I expect there to be some updates and requests but not much here should step on anything existing.

The one exception is how the workspace and git driver were using Users - which was going through the Git config to retrieve user information. I was assuming though that that was more a stop-gap since users wasn't very robust at the time and ldap integration was non-existent. @ginty, correct me if I'm wrong here though. If you think this is a problem and you'd like the git config methods to hang around, I think we can do that with datasets, and have git be the default. Actually, I can do that anyway if you'd like.

Metadata (Again)

There's now two metadatas running around, which is confusing, and I'm going to look at merging these into one. The older metadata was added here and just stored a reference to a Python object. The advantage of that is it acts like you'd expect a Python object to; that is, passed by reference and can be literally anything but with the caveat of it can only be used by Python and if the backend needed it for some reason, the frontend had to know about it and provide a way to send it back.

The second metadata actually came about from the session store, where everything had to be serializable. However, with some type-awareness in PyAPI, we can actually store objects usable by Rust, meaning that primitives stored in the session are available both in the frontend and backend. I have some tests which should show this in the backend and in the frontend.

The caveat here is the object needs to be a recognizable type, but it allows some semblance between the frontend and backend w.r.t. arbitrary data. Anything not stored as a Rust primitive, in this case, is serialized. This is (as far as I can tell) 99% transparent to the frontend - you'd likely have to go out of your way or be doing something beyond "our normal use case" to see it. An example that comes to mind is numpy integer types, which when stored will always resolve as int(numpy_object) -> BigInt (Rust/Session) -> int (Python). Basically, if you give the session store a numpy.short, you'll get an int back. We could pretty easily add an argument to store the actual type or a user-string along with it, allowing PyAPI to cast to the actual type before returning, or having the user do it, but for now I'm dubbing this a corner case, putting it on the "nice to have" list and moving on. :p

Because Users is meant to be used on both sides, the DataStores use this latter metadata.

The tests are kind of a chicken-and-egg problem right now and, for the time being, I just moved the Rust tests to after the Python tests, which would only pass if the Python app is built. I'll look at mixing this around. I'd like to go Rust Tests -> Python Tests -> Rust Test that require Python App, but my initial forays into configuring cargo tests were kinda messy. Will look at it some more though.

AutoAPI

@ginty, I did move the git-URL-ed autoAPI into the origen app itself but I didn't check it in yet. The original autoAPI is under the Apache license and Origen is under MIT. I'm 99.9% sure this will be fine, but I couldn't find anything definitive that says essentially re-licensing an Apache license with MIT is okay. If this ends up being alright, I do have a small README that'll go in that directory that gives the original author credit and links the original project. Other than trying to build website though, we can leave this out for now. It works in my environment so I can check it in if you think there's no issue. If there is, we can look at making it a development-only dependency while I prioritize trying to get it checked into the master branch. I need to document the added features and fix whatever tests I broke (and need to do so on Linux, even though I'm developing on Windows), which just hasn't been a priority.

Conclusion

I think this is ready for merging. Its not perfect and I have a local to-do list I can share if desired, but I think everything here is a solid starting point. Only item I know for sure that needs resolving is the auto-API and, if its okay to re-license, then that's already done locally. Will just need to check it in.

ginty commented 3 years ago

Hi @coreyeng, sounds good though I haven't digested it all yet.

Wrt to the user resolution, I think it should similar to O1 where if you say something like CurrentUser.email, then it will try various ways to find an email - e.g. Already set on the current user, Git, LDAP, etc.

So yes, probably I pulled it from Git initially since that was the only way, but I think Git should still be in the mix going forward - though I don't mind what priority it has in the lookup chain.

coreyeng commented 3 years ago

Hi @ginty, I thought that searching for an email (or anything) in that way might be a bit dangerous from how I have the datasets working now. Say you have an ldap and a git dataset, with ldap as the default, and ldap only sets the username but not the email. I only have a single default dataset, so:

origen.current_user().username #=> username from LDAP
origen.current_user().email #=> None (LDAP doesn't provide one)

Having it go:

origen.current_user().username #=> username from LDAP
origen.current_user().email #=> email from Git since LDAP doesn't provide one

To me, this was a bit confusing. I'd rather you need to either explicitly set the emails or explicitly state where you want it to come from:

# This will also work for things that use the email in the backend
origen.current_user().datasets["ldap"].email = origen.current_user().datasets["git"].email

origen.current_user().username #=> username from LDAP
origen.current_user().email #=> email from LDAP previously set
username = origen.current_user().username #=> username from LDAP
email = origen.current_user().email or origen.current_user().datasets["git"].email #=> email from git if LDAP doesn't provide one

# use username and email here

This'll have some problems for Rust-implemented stuff though that doesn't have an associated Python app that can mess about with this "email relationship" first. Maybe a middle ground is a "default chain" instead of a singleton?:

# current way, only allow a single default
users__default_dataset = "LDAP"

# Could be updated to fall back in a defined order
users__default_dataset = ["LDAP", "Git"]
coreyeng commented 3 years ago

Hello,

I made some updates based on the feedback from last Thursday. Not much to say in terms of implementation as the docs were updated to reflect the changes - specifically the hierarchy parts of the datasets section and the password caching section

Two notes on external libraries though: