Replies: 5 comments 4 replies
-
Generally this only really matters during authorizing a write/change operation, though it could for read in the future. |
Beta Was this translation helpful? Give feedback.
-
I don't like the idea of there being multiple places to check for the session office id. It would only complicate debugging issues. It seems we need to make a decision between encapsulating the CWMS schema session office or exposing that as part of this API as well. Putting it in the header exposes it, but it also lets us test for where it is actually used. If we want to tackle removing unintentional session id usages within the CWMS schema we can write tests leaving the header out. We wouldn't be able to do the same thing by leaving the DTO office out. |
Beta Was this translation helpful? Give feedback.
-
The office could also be associated with an api-key. I'm not sure how this would work with all the various ways we're considering doing authentication but the creation of a user key is analogous to a new user session. So if the office is supposed to exist in the session it seems like it would be associated with the key. Not sure how the user specifies what office they want a key for - is that a drop down on the login dialog? Is there a different login end-point for each office? An extra parameter to the login process? idk how it would work. Seems like it should at least be considered. |
Beta Was this translation helpful? Give feedback.
-
... it is most unfortunate that our baseline roles are Office based. Like the manager checks for "has the CWMS User role" but that role is per office. However, I think I can get away with "has role in anyoffice" for the initial check. |
Beta Was this translation helpful? Give feedback.
-
Whelp, I've hit a, fortunately somewhat minor, snag. The idea of handling the office at the DTO (and in the case of some deletes parameter) level does work and the code change for that wasn't super extreme. However, the function that actually set the user for the session fails because the "office" from the previous session is either active and the next user might not have it or the next user doesn't have and it has to be set first. The good news is, relatively simple fix. Going to change that function (or add one) that also takes the office so it can just set it correctly. Bad news is, that fix has to be done at the database. On the otherhand, it means we can just delete the whole "DataSourcePreparer" concept. (Note that I'm out to murder it, specifically it did what we needed at the time, but if we can simplify we should.) So this effort is going to include a little more change than I wanted. Not just the DTO changes themselves, but also rearranging the the auth code. Unfortunately for testing all needs to be done at once, but fortunately we have that set of tests. |
Beta Was this translation helpful? Give feedback.
-
There was a side conversation in a Pull Request that I thought suited to put here for future ideas/brainstorming.
Here is a paste of some bits I pulled from the PR, but you can read it in it's entirety (here)
PR tidbits
Alternatively an additional explicit header, like X-CWMS-Auth-Office, could be set on each call that requires it separate from the query parameter.
Here are some naïve thoughts I had about automagically determining the office
I think the X-CWMS-Auth-Office is a great idea as it serves the auth need and allows setting the office globally (which could remove the need for the office param)
Whenever a user choses an office param it should override the set header for the proceeding data fetch (but not the auth)
Some other ideas on trying to determine office without user/dev inputs
In no particular order
Overrides (header/param) would overwrite all of the options in 3
Beta Was this translation helpful? Give feedback.
All reactions