auth: Remove Upsert/Delete from SAMLService interface#18634
Conversation
|
@ravicious @smallinsky This PR will need to go in before I can merge https://github.com/gravitational/teleport.e/pull/597 and #18285 |
camscale
force-pushed
the
camh/narrow-saml-interface
branch
from
3bb515c to
071c21e
Compare
|
I've added @ravicious and @smallinsky as reviewers as this is part of the PR series that they have already reviewed - this is a last minute change that I could not make work within the other PRs (see linked issue for PRs). |
There was a problem hiding this comment.
Please wrap auth.ErrRequiresEnterprise here and anywhere else where you're checking for an enterprise license.
There was a problem hiding this comment.
Good catch. That's because I factored the error to the top-level, there's no (useful) trace information, so I should now wrap at the point the error is returned? Otherwise if I had return trace.AccessDenied(errMsg), that would wrap correctly?
Also, were you also asking me to rename the var? I'll rename to ErrSAMLRequiresEnterprise since the error message contains SAML (and will conflict with OIDC when I do that next).
There was a problem hiding this comment.
No, not asking you to rename.
Basically I want errors.Is(err, ErrRequiresEnterprise) to return true for whatever err it is you return. This way we have a consistent way of checking whether an error is due to requiring enterprise without resorting to string comparisons.
So something like fmt.Errorf or trace.Wrap should do the trick.
There was a problem hiding this comment.
ack. sorry I went looking for ErrRequiresEnterprise but didn't find it - hence my confusion - I must have mistyped. Will update.
|
@zmb3 PTAL |
Remove the Upsert and Delete methods from the SAMLService interface. It was intended for these to be part of the SAML "plugin", however they are needed for the operator tests to be able to create and delete the SAML connectors. The methods are back to being implemented directly in the `auth.Server`. This does not cause any real issues with the rest in the enterprise repo as the upsert/delete logic is just manipulating the local backend with no actual SAML logic in it. The `Get*` methods had already been kept in the `auth.Server` for the same reason. This narrows the `SAMLService` interface to just creating the SAML auth request and validating the response that comes back from the SAML identity provider. This is the core of the SAML-specific logic.
Rename error variable to be more explicit, and ensure it is wrapped when returned from a function. Addresses: https://github.com/gravitational/teleport/pull/18634/files#r1029449563
camscale
force-pushed
the
camh/narrow-saml-interface
branch
from
0ea5f94 to
550a864
Compare
4 participants
Remove the Upsert and Delete methods from the SAMLService interface. It
was intended for these to be part of the SAML "plugin", however they are
needed for the operator tests to be able to create and delete the SAML
connectors.
The methods are back to being implemented directly in the
auth.Server.This does not cause any real issues with the rest in the enterprise repo
as the upsert/delete logic is just manipulating the local backend with
no actual SAML logic in it. The
Get*methods had already been kept inthe
auth.Serverfor the same reason.This narrows the
SAMLServiceinterface to just creating the SAML authrequest and validating the response that comes back from the SAML
identity provider. This is the core of the SAML-specific logic.
Issue: https://github.com/gravitational/teleport.e/issues/525