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

feat(identities): add a state to identities #1312

Merged
merged 4 commits into from
May 23, 2021

Conversation

thomasruiz
Copy link
Contributor

@thomasruiz thomasruiz commented May 4, 2021

Related issue

#598

Proposed changes

Add a disabled_at field in the identity model. If it is set, the account is disabled. Block session creation when the account is disabled.

Checklist

  • I have read the contributing guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further comments

This is draft just to make sure the implementation is correct. Tests and documentation are very much needed, but before going further, I'd rather have a validation from the maintainers.

@CLAassistant
Copy link

CLAassistant commented May 4, 2021

CLA assistant check
All committers have signed the CLA.

@thomasruiz thomasruiz changed the title feat(identities): add a state to entities feat(identities): add a state to identities May 4, 2021
@aeneasr
Copy link
Member

aeneasr commented May 5, 2021

Thank you for the contribution! I think what I meant in the original issue was more along the lines of adding a State parameter that is an enum of the different states. That way, we don't have to rely on nil values but instead have a clear cut singular state object :)

@thomasruiz
Copy link
Contributor Author

Would there be another state besides active and disabled? Having a date to know when the user was disabled is a good thing, it allows automatic cleanup and other things. We could have a state field and a state_changed_at but... I don't see any other state rather that active and disabled. :p

@zepatrik
Copy link
Member

zepatrik commented May 5, 2021

invited might be a possible candidate later on (e.g. to implement #595). I am pretty sure there will be some more coming up 😉

@thomasruiz thomasruiz force-pushed the feat/disable-account branch 2 times, most recently from c99087d to a4d43d6 Compare May 5, 2021 18:20
@thomasruiz
Copy link
Contributor Author

Alright, I see the point. This should be more what you guys had in mind. It still needs tests, and I'm thinking about adding a PATCH method to the identities controller. What do you think?

@aeneasr
Copy link
Member

aeneasr commented May 11, 2021

Sorry @thomasruiz for dropping the ball on this. Would you still be open to work on this or have you proceeded to other work?

@thomasruiz thomasruiz force-pushed the feat/disable-account branch from a4d43d6 to b8c5a14 Compare May 11, 2021 14:47
@thomasruiz
Copy link
Contributor Author

@aeneasr I updated most of the tests but there is still a problem: I don't see how to update the openapi specs to regenerate the SDKs. I added state and state_changed_at to the response of the identities API.

@thomasruiz thomasruiz force-pushed the feat/disable-account branch from b8c5a14 to 182cecc Compare May 11, 2021 14:59
@aeneasr
Copy link
Member

aeneasr commented May 12, 2021

To regenerate the SDK run make sdk in the root directory :)

@thomasruiz thomasruiz force-pushed the feat/disable-account branch 2 times, most recently from 736a88a to 50b301e Compare May 12, 2021 13:50
identity/identity.go Outdated Show resolved Hide resolved
@thomasruiz thomasruiz force-pushed the feat/disable-account branch from 50b301e to 6a29f7f Compare May 14, 2021 13:47
@aeneasr
Copy link
Member

aeneasr commented May 14, 2021

I think this is ready for review right? :) If so can you please mark it as ready for review? Thank you! :)

@thomasruiz thomasruiz marked this pull request as ready for review May 14, 2021 14:18
@thomasruiz
Copy link
Contributor Author

I think this is ready for review right? :) If so can you please mark it as ready for review? Thank you! :)

There you go! I hope I didn't mess anything up :)

identity/identity.go Outdated Show resolved Hide resolved
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Awesome! Just have two small things but this is basically good to merge! Could you also add a small section on the new identity state to the docs? :)

identity/identity.go Outdated Show resolved Hide resolved
identity/identity.go Outdated Show resolved Hide resolved
identity/identity.go Outdated Show resolved Hide resolved
identity/identity.go Outdated Show resolved Hide resolved
@thomasruiz thomasruiz force-pushed the feat/disable-account branch from 6a29f7f to 2336eec Compare May 19, 2021 10:00
@aeneasr
Copy link
Member

aeneasr commented May 21, 2021

🙄 circle ci has an outage..

@thomasruiz
Copy link
Contributor Author

A

@aeneasr
Copy link
Member

aeneasr commented May 22, 2021

🤣

@aeneasr
Copy link
Member

aeneasr commented May 23, 2021

Thank you so much, a job well done! :)

@aeneasr aeneasr merged commit d22954e into ory:master May 23, 2021
harnash pushed a commit to Wikia/kratos that referenced this pull request May 24, 2021
Closes ory#598

Co-authored-by: hackerman <3372410+aeneasr@users.noreply.github.com>
harnash pushed a commit to Wikia/kratos that referenced this pull request May 24, 2021
Closes ory#598

Co-authored-by: hackerman <3372410+aeneasr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants