Introduce concept of Silos#747
Conversation
Add silos, which will isolate organizations, and provide a namespace for
users and groups.
This required adding Silo id to Actor, so users that have authenticated
now have an associated Silo id that can be used to restrict organization
lookup.
Silos can be created, read, and deleted. modification is a TODO.
A few tests have been modified to use `authn_as` because an earlier
version of this branch added OpContext to every endpoint, but that was
reverted because the blast radius of the PR would have been too large.
What remains are a few modified tests that make authenticated calls.
When all endpoints are protected and each datastore function has an
OpContext, Silo can be looked up on Actor. For now, there are places
hard coding as the built-in Silo.
Still TODO:
- authz for silos and silo users
- some testing is dependent on ^
- PUT /silos/{name}
- building on top of silos
davepacheco
left a comment
There was a problem hiding this comment.
Thanks again for taking this on! This really wound up needing to touch code all over Nexus, which makes sense. There are lots of places where you plugged right into some existing convention. I hope it wasn't too hard to figure out what was going on. Nice work.
|
The |
get rid of get_silo_id_from_silo_user_id, use silo_user_fetch instead.
|
K, ran out of time today to rebase - follow-up commits ready for re-review though |
|
Should I hold off on a rebase until you folks are done re-reviewing? |
| .await | ||
| .map_err(|e| match e { | ||
| AsyncInsertError::CollectionNotFound => Error::ObjectNotFound { | ||
| type_name: ResourceType::Silo, |
There was a problem hiding this comment.
Yes, but you're creating the Organization inside a Silo that I think you must be logged into.
| } | ||
| } | ||
|
|
||
| // Return this instead, by performing an additional SELECT to get silo id |
There was a problem hiding this comment.
I think it's useful that there be a low-level layer that represents exactly what's in the database, which is what db::model is today (though it doesn't have to be). I think we're both saying there's room for another layer atop that with more usable/useful abstractions. We could make that an explicit layer ("data"? ugh the naming is hard) but that feels too bureaucratic to me. I think it makes sense to put such abstractions in the subsystems they're associated with, which is why I suggested authn here. But maybe it should just be authn::ConsoleSession? The idea is a reader would understand: db::model::X is a low-level database interface but some_subsystem::X is probably what I want to be using, if it exists.
|
I think that's all of the follow-up addressed, except for the decision about Let me know if you folks would like me to rebase and have you review that instead? |
|
It would be nice to see it rebased. |
|
@jmpesp if you're planning to rebase + force push rather than merge, I wonder if it's worth doing that in a separate PR (and close this one)? My experience is that GitHub incremental review is basically destroyed after a force push, and even trying to view the old discussions becomes difficult to impossible once those commits are removed from the PR. |
This is my experience too, unfortunately. Closing and opening a new PR now. |
Add silos, which will isolate organizations, and provide a namespace for
users and groups.
This required adding Silo id to Actor, so users that have authenticated
now have an associated Silo id that can be used to restrict organization
lookup.
Silos can be created, read, and deleted. modification is a TODO.
A few tests have been modified to use
authn_asbecause an earlierversion of this branch added OpContext to every endpoint, but that was
reverted because the blast radius of the PR would have been too large.
What remains are a few modified tests that make authenticated calls.
When all endpoints are protected and each datastore function has an
OpContext, Silo can be looked up on Actor. For now, there are places
hard coding as the built-in Silo.
Still TODO: