Add `cb logout` command. #49

abrightwell · 2022-06-15T15:52:35Z

These changes add a new command and action called logout. The purpose
of these are to allow a user to clear all credentials from their system
which has the effect of logging them out of the system. To do this, we
remove both the stored credentials/API key. As well, we remove the
cached token regardless of whether or not it is expired.

In the case that none of the above items exist on the system, the effect
is the same as a no-op.

Mentioned in #47.

abrightwell · 2022-06-15T15:57:22Z

src/cb/cacheable.cr

@@ -22,15 +22,21 @@ module CB::Cacheable
    end

    def fetch?(key)
+      token = fetch key


I felt like perhaps this was an acceptable addition. It seems useful to have both versions. Though, I had also considered adding an expired? function in a previous PR. And that came up here again as perhaps something that is worthwhile and perhaps it is in the bigger picture. However, I figured this particular addition was just as useful for other purposes as well. So opted to follow-up with the other in a separate PR later if it became evident that its addition would truly be beneficial.

I think in a earlier thing (not sure if ever committed) I had separate thing if it was expired vs missing. But then it just had the same logic, so it didn't matter the difference between the two cases. At that time at least.

The only thing with this change is that when there are pairs of methods in crystal, one with a ? the convention I think is that the non question mark never returns a nilable object. This new non question mark one does.

For the token delete, if the original fetch? would return nil, we would never use that token, so it's already as good as deleted in a sense. There may be a malformed file there, but cb wont use it.

So it might be okay to leave this method as it was? I'm not entirely sure. I think since the delete has a nil check already it's safe. The only difference would be an expired token will sit around on the file system, but that's ok?

So it might be okay to leave this method as it was? I'm not entirely sure. I think since the delete has a nil check already it's safe. The only difference would be an expired token will sit around on the file system, but that's ok?

So this is the impetus for my change here. If it's 'expired' then it doesn't matter, so effectively the same thing. But I was conflicted on whether or not it should be removed regardless. I'm not sure I have a strong opinion there, but it seemed reasonable to think all access related items would be removed. Another conflict I had was whether or not we just have a 'clear cache' kind of approach to it. 🤷 I don't want to over complicate it, but yeah... that's how I arrived at this implementation.

abrightwell · 2022-06-15T16:01:31Z

src/cb/token.cr

@@ -41,6 +41,11 @@ struct CB::Token
    fetch? host
  end

+  def self.delete(host)
+    token = fetch host
+    token.delete unless token.nil?


I kind of feel like Token#delete and also Creds#delete should perhaps be more fault tolerant in the long run. However, I figured since it's not nil then it 'should' exist and should be safe. That doesn't mean I don't still wish it to be more fault tolerant, but I also don't want to explicitly silence any exceptions... not yet at least without good reason.

Hmmm, what if the the class method delete on these removes the file if it exists, and if the file doesn't exist don’t do anything. Then we wouldn't need to even fetch them in the first place to logout?

will · 2022-06-15T16:41:55Z

src/cb/creds.cr

+
+  def self.delete(host)
+    creds = for_host host
+    creds.delete unless creds.nil?


Just a few different ways to do this

creds.delete unless creds.nil? creds.delete if creds creds.try &.delete for_host(host).try &.delete # and drop the creds = line above

Any of them are fine, and it doesn't really matter, just wanted to point out the ways. I tend to like unless more when it is making a nil? go away rather than add it, like if blah.nil? to unless blah, but I'm sure i'm not consistent about that.

I think I like the last one most. I didn't like having to assign a var, but keep forgetting about that form.

will · 2022-06-15T16:52:04Z

src/cb/cacheable.cr

@@ -22,15 +22,21 @@ module CB::Cacheable
    end

    def fetch?(key)
+      token = fetch key


I think in a earlier thing (not sure if ever committed) I had separate thing if it was expired vs missing. But then it just had the same logic, so it didn't matter the difference between the two cases. At that time at least.

The only thing with this change is that when there are pairs of methods in crystal, one with a ? the convention I think is that the non question mark never returns a nilable object. This new non question mark one does.

For the token delete, if the original fetch? would return nil, we would never use that token, so it's already as good as deleted in a sense. There may be a malformed file there, but cb wont use it.

So it might be okay to leave this method as it was? I'm not entirely sure. I think since the delete has a nil check already it's safe. The only difference would be an expired token will sit around on the file system, but that's ok?

These changes add a new command and action called `logout`. The purpose of these are to allow a user to clear all credentials from their system which has the effect of logging them out of the system. To do this, we remove both the stored credentials/API key. As well, we remove the cached token regardless of whether or not it is expired. In the case that none of the above items exist on the system, the effect is the same as a no-op.

abrightwell · 2022-06-15T22:14:56Z

Thanks!

abrightwell requested a review from will June 15, 2022 15:54

abrightwell force-pushed the abrightwell/logout-action branch from f104d35 to 987dff2 Compare June 15, 2022 15:58

abrightwell commented Jun 15, 2022

View reviewed changes

abrightwell force-pushed the abrightwell/logout-action branch from 987dff2 to 3f89c68 Compare June 15, 2022 16:22

will reviewed Jun 15, 2022

View reviewed changes

abrightwell force-pushed the abrightwell/logout-action branch from 3f89c68 to 47dc5cf Compare June 15, 2022 19:37

abrightwell requested a review from will June 15, 2022 19:39

abrightwell force-pushed the abrightwell/logout-action branch from 47dc5cf to bf02208 Compare June 15, 2022 19:40

will approved these changes Jun 15, 2022

View reviewed changes

abrightwell merged commit ce6ca18 into main Jun 15, 2022

abrightwell deleted the abrightwell/logout-action branch June 15, 2022 22:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `cb logout` command. #49

Add `cb logout` command. #49

abrightwell commented Jun 15, 2022 •

edited

Loading

abrightwell Jun 15, 2022

will Jun 15, 2022

abrightwell Jun 15, 2022

abrightwell Jun 15, 2022

will Jun 15, 2022

will Jun 15, 2022

abrightwell Jun 15, 2022

will Jun 15, 2022

abrightwell commented Jun 15, 2022

Add cb logout command. #49

Add cb logout command. #49

Conversation

abrightwell commented Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abrightwell commented Jun 15, 2022

Add `cb logout` command. #49

Add `cb logout` command. #49

abrightwell commented Jun 15, 2022 •

edited

Loading