Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(queries): login policy projection #2401

Merged
merged 211 commits into from Oct 19, 2021
Merged

fix(queries): login policy projection #2401

merged 211 commits into from Oct 19, 2021

Conversation

adlerhurst
Copy link
Member

merge after #2383

@adlerhurst adlerhurst assigned hifabienne and unassigned adlerhurst Oct 18, 2021
@adlerhurst adlerhurst marked this pull request as ready for review October 18, 2021 08:36
hifabienne
hifabienne previously approved these changes Oct 18, 2021
if err != nil {
return nil, err
}
return &mgmt_pb.GetLoginPolicyResponse{Policy: policy_grpc.ModelLoginPolicyToPb(policy), IsDefault: policy.Default}, nil
//TODO: why do we provice IsDefault twice?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is default should only be shown once. I think this happend on last api refactoring, because of the sub protos.
I think we should deprecate it in the api. So we can remove it in the future

Sequence: policy.Sequence,
CreationDate: timestamp_pb.New(policy.CreationDate),
ChangeDate: timestamp_pb.New(policy.ChangeDate),
ResourceOwner: policy.OrgID, //TODO: should resource owner always be the org from ctx?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean with this comment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have no idea. deleted the comment

Comment on lines 15 to 22
ForceMFA bool
HidePasswordReset bool
PasswordlessType PasswordlessType
SecondFactors []SecondFactorType
MultiFactors []MultiFactorType
SecondFactors []domain.SecondFactorType
MultiFactors []domain.MultiFactorType
Default bool

CreationDate time.Time
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where do we need this view model? wouldn't it make sense to use the domain model?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's needed in the AuthRequest-struct. I would not remove it now as not all objects in AuthRequest are changed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense

internal/query/policy_login.go Show resolved Hide resolved
)
if err != nil {
if errs.Is(err, sql.ErrNoRows) {
return nil, errors.ThrowNotFound(err, "QUERY-yPqIZ", "errors.login_policy.not_found")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message is wrong. should start with upper case

internal/query/policy_login.go Outdated Show resolved Hide resolved
)
if err != nil {
if errs.Is(err, sql.ErrNoRows) {
return nil, errors.ThrowNotFound(err, "QUERY-yPqIZ", "errors.login_policy.not_found")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error message

@@ -0,0 +1 @@
ALTER TABLE zitadel.projections.orgs RENAME COLUMN domain TO primary_domain;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this on this branch?

@@ -0,0 +1,13 @@
CREATE TABLE zitadel.projections.org_domains (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this on this branch?

@@ -0,0 +1,20 @@
CREATE TABLE zitadel.projections.login_policies (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename version

error messages,
migrations
Copy link
Member

@hifabienne hifabienne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

adlerhurst and others added 2 commits October 18, 2021 17:12
Co-authored-by: Fabi <38692350+fgerschwiler@users.noreply.github.com>
hifabienne
hifabienne previously approved these changes Oct 18, 2021
@hifabienne hifabienne enabled auto-merge (squash) October 19, 2021 07:58
@hifabienne hifabienne merged commit 1c26587 into main Oct 19, 2021
@hifabienne hifabienne deleted the login-policy-projection branch October 19, 2021 08:10
@github-actions
Copy link

🎉 This PR is included in version 1.47.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants