Add user to orgs by default#371
Conversation
This commit creates an org for each user that is created. This org is the default org to which ad hoc scans will be attached.
This commit started it's life as a relatively small feature adding a user to an org by default to ensure that users has a usable account after logging in, but ended up with some serious mission creep. Writing tests for this feature uncovered a lack of isolation between tests, and tests that mostly failed with `KeyError` or `'NoneType' object is not subscriptable` errors, as well as test data with clashing ids. This change had some far reaching consequences and I basically kept reworking the tests until the isolation errors went away and I could tell if the "user orgs" feature had actually broken stuff. If it did, I fixed that too.
| elif user_claims: | ||
| user_id = user_claims[0]['user_id'] | ||
| with app.app_context(): | ||
| # XXX: affiliations should have been eager loaded with joinedload |
There was a problem hiding this comment.
The user_id is being pulled from the jwt, and the reason why there is another query to user_affiliations is to confirm that the users access is up to date and has not been modified since initial login
There was a problem hiding this comment.
If we did a joinedload and loaded both the user and their affiliations in one shot at the beginning of the request that would be good. But I guess you're getting at the idea that another thread/query could change the state in the db between that inital load and when this gets executed. I'll have to think about that.
| payload = { | ||
| "exp": datetime.datetime.utcnow() | ||
| + datetime.timedelta(days=0, seconds=1800), | ||
| + datetime.timedelta(days=0, seconds=1800), # XXX: too short |
There was a problem hiding this comment.
When this was initially created Po approved the 30min time limit
There was a problem hiding this comment.
Ha! We'll have to figure that out. I was finding that was so short that it was actually invalidating my jwts while I was debugging stuff!
There was a problem hiding this comment.
Oh I know that feeling 😂
| } | ||
| user_roles.append(temp_dict) | ||
| else: | ||
| # XXX: Roles is [] || [""] || [{}]? |
There was a problem hiding this comment.
I forget the exact reason why its set like this
There was a problem hiding this comment.
No worries! Little breadcrumbs leading us back to things to simplify.
tparrott-cse
left a comment
tparrott-cse
left a comment
There was a problem hiding this comment.
LGTM -- with the understanding we should come back through this code base and look for optimizations such as your joinedload suggestions.
3 participants
This commit started it's life as a relatively small feature adding a user to an
org by default to ensure that users has a usable account after logging in, but
ended up with some serious mission creep.
Writing tests for this feature uncovered a lack of isolation between
tests, and tests that mostly failed with
KeyErroror'NoneType' object is not subscriptableerrors, as well as test data withclashing ids.
This change had some far reaching consequences and I basically kept reworking
the tests until the isolation errors went away and I could tell if the "user
orgs" feature had actually broken stuff. If it did, I fixed that too.