Refuse to delete non-empty IdPs (412 precondition failed) #875
Conversation
fisx
changed the title
|
I think I'm done with this. |
|
can be merged by reviewer |
|
(we may want to add a shell script that deletes non-empty idps by first deleting all the users inside. it's dangerous, but i have a feeling we're going to use it a couple of times before we've figured out how to do all of this in team-settings.) |
|
before merging this to dev, make sure QA and clients are aware of the change and happy with it. |
| team = idp ^. SAML.idpExtraInfo | ||
|
|
||
| -- fail if idp is not empty | ||
| idpIsEmpty <- wrapMonadClient $ isNothing <$> Data.getSAMLAnyUserByIssuer issuer |
There was a problem hiding this comment.
Why is there a map here, when we are getting a single user?
There was a problem hiding this comment.
Why is there a map here, when we are getting a single user?
The return value of Data.getSAMLAnyUserByIssuer is m (Maybe UserId) for some functor m. <$> maps into any functor, so in this case it's not [], but m. Inside the m, we have a maybe value, which is transformed into a boolean by isNothing.
Is that a good answer? And am I answering the right question?
| env <- ask | ||
| (firstOwner, tid, idp) <- registerTestIdP | ||
| ssoOwner <- mkSsoOwner firstOwner tid idp | ||
| callIdpDelete' (env ^. teSpar) (Just ssoOwner) (idp ^. idpId) | ||
| `shouldRespondWith` \resp -> statusCode resp < 300 | ||
| `shouldRespondWith` checkErr (== 412) "idp-has-bound-users" |
There was a problem hiding this comment.
these error code changes confuse me, as i don't see 404 or 412 in the changed code.
| callIdpGet' (env ^. teSpar) (Just ssoOwner) (idp ^. idpId) | ||
| `shouldRespondWith` checkErr (== 404) "not-found" | ||
| `shouldRespondWith` \resp -> statusCode resp < 300 |
There was a problem hiding this comment.
I don't have an explanation for this change, though. This is indeed odd.
There was a problem hiding this comment.
ah, no, should have read the test: if spar refuses to delete the idp, it won't be deleted, so getting it will still work. :)
| @@ -212,6 +213,14 @@ insertSAMLUser (SAML.UserRef tenant subject) uid = retry x5 . write ins $ params | |||
| ins :: PrepQuery W (SAML.Issuer, SAML.NameID, UserId) () | |||
| ins = "INSERT INTO user (issuer, sso_id, uid) VALUES (?, ?, ?)" | |||
|
|
|||
| -- | We only need to know if it's none or more, so this function returns the first one. | |||
| getSAMLAnyUserByIssuer :: (HasCallStack, MonadClient m) => SAML.Issuer -> m (Maybe UserId) | |||
| getSAMLAnyUserByIssuer issuer = runIdentity <$$> | |||
There was a problem hiding this comment.
@fisx please change this C* query to use LIMIT 1 otherwise you risk returning a potentially unbound nr. of users.
There was a problem hiding this comment.
good catch, glad you've paid attention! thanks :)
Fixes https://github.com/zinfra/backend-issues/issues/754